Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix C++20 compliance with application of upstream PR 965 #15

Merged
merged 1 commit into from
Oct 28, 2024

Conversation

mikehardy
Copy link

@mikehardy mikehardy commented Oct 26, 2024

These symbols are no longer valid on macOS and will fail builds if the NDEBUG symbol is !defined, as it is now in default build settings for all react-native projects since release of react-native 0.76

This same change was applied upstream but never released, and it appears it has not been applied here

google#965 (comment)

See: invertase/react-native-firebase#8082 (comment)

It would be fantastic to get a leveldb release with this fix, that firebase-ios-sdk adopted, so react-native-firebase was compatible with firebase-ios-sdk out of the box again with react-native 0.76

Thanks!

These symbols are no longer valid on macOS and will fail builds if the `NDEBUG` symbol is defined, as it is now in default build settings for all react-native projects since release of react-native 0.76

This same change was applied upstream but never released, and it appears it has not been applied here

google#965 (comment)
Copy link

@andrewheard andrewheard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. This seems like a safe change regardless of what C++ Language Dialect is used for Firestore (based on manual testing and looking at the header in libc++).

  • If compiling for C++17, or older, then std::memory_order_relaxed refers to memory_order_relaxed in the memory_order enum.
  • If compiling for C++20, or newer, then std::memory_order_relaxed refers to relaxed in the new memory_order enum class; this is implemented with a memory_order_relaxed constexpr:
    • inline constexpr auto memory_order_relaxed = memory_order::relaxed;
    • I'm guessing they did this for backwards compatibility.

Also, thanks @mikehardy!

@paulb777 paulb777 merged commit c6d3ab7 into firebase:firebase-release Oct 28, 2024
6 checks passed
@paulb777
Copy link
Member

Thanks @mikehardy! We're going to test overnight and if all goes well, we'll publish 1.22.6 tomorrow.

@AngeloAvv
Copy link

I'm experiencing the same problem with a Flutter build. Shall I report this problem somewhere? If yes, where's the best place to do it? Thanks!

fvm flutter build ios -t lib/main.dart --release --no-codesign --build-number $CI_PIPELINE_IID
Downloading ios tools...                                        
Downloading ios-profile tools...                                
Downloading ios-release tools...                                
Warning: Building for device with codesigning disabled. You will have to manually codesign before deploying to device.
Building xxx for device (ios-release)...
Running pod install...                                          
Running Xcode build...                                          
Xcode build done.                                           46.2s
Failed to build iOS app
Semantic Issue (Xcode): No member named 'memory_order_relaxed' in 'std::memory_order'; did you mean 'std::memory_order_relaxed'?
/Users/angeloavv/builds/h5vqU3qv/0/flutter/clients/xxx/ios/Pods/leveldb-library/util/env_posix.cc:839:33
Semantic Issue (Xcode): No member named 'memory_order_relaxed' in 'std::memory_order'; did you mean 'std::memory_order_relaxed'?
/Users/angeloavv/builds/h5vqU3qv/0/flutter/clients/xxx/ios/Pods/leveldb-library/util/env_posix.cc:856:34
Encountered error while building for device.

@mikehardy
Copy link
Author

@AngeloAvv I believe this will resolve for you with these sets of steps and no further issues created:
1- wait for notice that 1.22.6 version of leveldb-library is published (using github web UI to "watch" this repository, for "releases" should allow you to receive notice)
2- remove ios/Podfile.lock, run pod repo update, then cd ios folder run pod install and I think the new release will automatically install based on the ~> style cocoapods dependency specified in firebase-ios-sdk which should allow the new point release to match the version constraint
3- build as normal, should succeed

@paulb777
Copy link
Member


🎉 Congrats

🚀 leveldb-library (1.22.6) successfully published
📅 October 29th, 08:14
🌎 https://cocoapods.org/pods/leveldb-library
👍 Tell your friends!

@mikehardy
Copy link
Author

Confirmed a fresh pod install (that is, after removing any existing Podfile.lock) installs the latest version:

Installing leveldb-library (1.22.6)

...and it builds perfectly for me out of the box, thanks!

@mikehardy
Copy link
Author

@AngeloAvv I will guess you are now using cocoapods 1.16.0 and/or xcodeproj 1.26.0, newly released.
They have some problems for some reason and I can't build with them either, same error
Use cocoapods 1.15.2 and xcodeproj 1.25.0 gems to build
reference facebook/react-native#47237

@mikehardy
Copy link
Author

Hopefully everyone has moved to the latest release on this one - I had very-temporarily directed people to the commit on my branch as a workaround but I do not want anything like that to be long-lived so with apologies to anyone affected, I am deleting my branch now

Everyone should be relying on the official release now

@mikehardy mikehardy deleted the patch-1 branch October 31, 2024 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants