Auto populate the namespace list to rename in desktop packaging #1000
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ces to rename. This prevents some namespaces from accidentally being omitted. Instead, the script will rename all namespaces except for std:: and __gnu_cxx.
Also add 'firestore' to top-level namespaces. Also upgrade demumble to 1.2.2 which handles more MSVC demangling cases.
This reverts commit d01a21e.
Fix windows BFD target.
This fixes grpc_resource_quota_arg_vtable::$_0::__invoke
Remove strict_cpp as it should no longer be required for Windows.
jonsimantov
commented
Jul 20, 2022
| ) | ||
| cache_param=--cache=${cache_file} | ||
|
|
||
| if [[ ${platform} == "windows" ]]; then |
There was a problem hiding this comment.
No longer needed with the newer Demumble version.
jonsimantov
commented
Jul 20, 2022
| @@ -544,6 +549,9 @@ jobs: | |||
| cd sdk-src | |||
| python scripts/gha/install_prereqs_desktop.py --ssl boringssl | |||
| cd .. | |||
| if [[ $(uname) == "Darwin"* ]]; then | |||
There was a problem hiding this comment.
Fixes an issue where Mac packaging was not done in parallel due to the "parallel" tool not being installed. Speeds up packaging on Mac.
DellaBitta
requested changes
Jul 20, 2022
| @@ -544,6 +549,9 @@ jobs: | |||
| cd sdk-src | |||
| python scripts/gha/install_prereqs_desktop.py --ssl boringssl | |||
| cd .. | |||
| if [[ $(uname) == "Darwin"* ]]; then | |||
DellaBitta
approved these changes
Jul 20, 2022
❌ Integration test FAILEDRequested by @jonsimantov on commit c046c8a
Add flaky tests to go/fpl-cpp-flake-tracker |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Currently, there is a large list of C++ namespaces that are renamed in the binary version of the desktop SDK. This allows the Firebase C++ SDK to embed its own C++ dependencies (e.g. absl, BoringSSL, gRPC, etc.) without affecting any other versions of those dependencies that may be present in the build (for example, if the developer's desktop app uses OpenSSL or a different version of gRPC).
Because that list of C++ namespaces is manually maintained in the desktop packaging script, it's possible that when dependencies changed (as grpc recently did), there can be new namespaces introduced in dependencies that are not added to that list. This would result in duplicate symbols when a developer ultimately links their desktop app—which, for example, already uses gRPC—with the Firebase C++ SDK, which embeds its own version of gRPC.
Fortunately, the script that performs this renaming has a mode where it will scan for all C++ namespaces and rename them all automatically, except the ones we specifically want to preserve (in this case, just "firebase::"). This will rename all namespaces inside the C++ libraries, except for certain system namespaces (currently "std" and "__gnu_cxx").
This PR enables this option. Ultimately the binary SDK created in this PR will need to be checked to make sure it does not have any extraneous frameworks. (This PR also adds such a check to the end of the packaging workflow.)
Finally, this PR brings back the merge_libraries_test script from the internal repo, which runs a battery of tests on merge_libraries, including new ones added to catch these very issues. The script is not in CI but can be run locally via scripts/merge_libraries_test.sh.
Testing
CPP binary SDK packaging runs
Type of Change
Place an
xthe applicable box:Notes
Release Notessection ofrelease_build_files/readme.md.