-
Notifications
You must be signed in to change notification settings - Fork 116
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
Use llvm-objcopy to package Windows binaries #1427
base: main
Are you sure you want to change the base?
Conversation
be881d3
to
46d8a05
Compare
@jonsimantov I hope I made the right changes based on your feedback - if not please let me know. Any chance you could trigger the CI to run with this branch? |
I'll go ahead and test the packaging process with these changes. To do that I believe I'll have to copy these changes into a branch within this repo. |
Invalid workflow file: .github/workflows/cpp-packaging.yml#L130 |
75e4ae8
to
ae87a35
Compare
Thanks for checking – can you try again with the latest changes now please? |
Fixed a missing quote, round 2: https://github.com/firebase/firebase-cpp-sdk/actions/runs/6190946332 |
Thanks @jonsimantov! I tried the build and it works great for me using the LLVM toolchain. One thing I noticed is that the firebase_cpp_sdk_windows.zip artifact itself contains another ZIP file, but I don’t think that stems from these changes? |
Unfortunately, the integration tests failed to build against the newly packaged SDK on Windows. I've attached the log file. |
Hmm ok, so the issue here is all these Is this also a new issue: |
@aganea would you be able to take a look at these errors? Unfortunately I’m a bit lost about how this change might be causing them. |
9c92fe6
to
84d6e76
Compare
I rebased the branch onto the latest main. @jonsimantov could you please run the tests once more to see if these linker errors are still happening? 🙏 |
It's still failing to link. Log attached: |
Thank you @jonsimantov! It seems like the packaged library is missing a bunch of symbols. Since the Here is the same log without timestamps as well as the one from the latest run off the main branch for comparison: The |
Description
Changes the packaging script to use LLVM binutils. This fixes object files corrupted by using
objcopy
by usingllvm-objcopy
instead. While the corrupted files were working fine when using the Visual Studio toolchain they were leading to errors when using the LLVM toolchain.See #793 (comment) for details.
Testing
Needs to be run as part of the GitHub actions packaging. I will test builds once they are available.
Type of Change
Place an
x
the applicable box:Notes
Release Notes
section ofrelease_build_files/readme.md
.