Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Open
triplef wants to merge 1 commit into firebase:main
base: main
Choose a base branch
Loading
from triplef:fix-win-objcopy

Conversation

Copy link

@triplef triplef commented Aug 16, 2023

Description

Changes the packaging script to use LLVM binutils. This fixes object files corrupted by using objcopy by using llvm-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:

  • Bug fix. Add the issue # below if applicable.
  • New feature. A non-breaking change which adds functionality.
  • Other, such as a build process or documentation change.

Notes

  • Bug fixes and feature changes require an update to the Release Notes section of release_build_files/readme.md.
  • Read the contribution guidelines CONTRIBUTING.md.
  • Changes to the public API require an internal API review. If you'd like to help us make Firebase APIs better, please propose your change in a feature request so that we can discuss it together.

Copy link
Author

triplef commented Aug 27, 2023

@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?

Copy link
Contributor

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.

Copy link
Contributor

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]) || ''

Copy link
Author

triplef commented Sep 14, 2023

Thanks for checking – can you try again with the latest changes now please?

Copy link
Contributor

triplef reacted with hooray emoji

Copy link
Contributor

triplef reacted with heart emoji

Copy link
Author

triplef commented Sep 15, 2023

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?

Copy link
Contributor

Unfortunately, the integration tests failed to build against the newly packaged SDK on Windows.

I've attached the log file.

8_Build integration tests.txt

Copy link
Author

triplef commented Sep 15, 2023
edited
Loading

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?

Copy link
Author

triplef commented Oct 9, 2023

@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.

Copy link
Author

triplef commented Jan 5, 2024

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? 🙏

Copy link
Contributor

It's still failing to link. Log attached:
8_Build integration tests.txt

Copy link
Author

triplef commented Jan 11, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
2 more reviewers

@jonsimantov jonsimantov jonsimantov left review comments

@aganea aganea aganea left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /