-
Notifications
You must be signed in to change notification settings - Fork 125
Use llvm-objcopy to package Windows binaries #1427
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
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
The workflow is not valid. .github/workflows/cpp-packaging.yml (Line: 130, Col: 16): Unexpected symbol: '['. Located at position 43 within expression: matrix.tools_platform == 'darwin' && join(['-', env.xcodeVersion]) || ''
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 error LNK2001: unresolved external symbol
errors, right? Any idea how they could be caused by this change?
Is this also a new issue: Performing Test CMAKE_HAVE_LIBC_PTHREAD - Failed
?
@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:
8_Build integration tests.txt
Thank you @jonsimantov! It seems like the packaged library is missing a bunch of symbols.
Since the -L
flag we’re adding in this patch is also making the packaging script use llvm-nm
and llvm-ar
(in addition to llvm-objcopy
), I’m wondering if those are somehow behaving differently to cause this. Do they require some extra flags maybe – any idea @aganea?
Here is the same log without timestamps as well as the one from the latest run off the main branch for comparison:
The Performing Test CMAKE_HAVE_LIBC_PTHREAD - Failed
is also present in the latest run and thus not a new issue.
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
.