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

Ensure that dynamic lib merge targets are unique. #1483

Merged
merged 4 commits into from
Oct 9, 2023

Conversation

freakboy3742
Copy link
Member

@freakboy3742 freakboy3742 commented Oct 7, 2023

The candidate list for dylibs to be merged will not be unique; every dylib will be found twice. This may also be the cause of the CI failure.

Also ensures that the merge step only operates on single-platform wheels.

Fixes #1481

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@freakboy3742 freakboy3742 requested review from mhsmith and removed request for mhsmith October 7, 2023 15:42
@freakboy3742
Copy link
Member Author

@mhsmith Hold off on reviewing this one - I've found another edge case related to dylib merging as a result of CI on beeware/toga#1399.

@freakboy3742
Copy link
Member Author

Ok - I've now fixed the problem we are seeing in beeware/toga#1399's CI.

The problem is that pip will install platform specific binaries in preference to universal2 binaries. As a result, if a package publishes both x86_64 and universal2 binaries (which is a common configuration - fontTools is at least one example that does this), and you build your app on x86_64, the original app_packages install the single platform binary, discovers the need for an arm64 binary, which then installs a universal2 binary. When you try to merge the two, you get an error, because 2 of the libraries being merged have an x86_64 slice.

This fix does a thinning pass first, ensuring that all the libraries in the arm64 folder only contain arm64 components. This does result in some double handling - you get a universal binary, strip it, then make a universal binary... but there's no easy way I can think of to either (a) force the installation of a universal binary in the first place, or (b) make a decision to use existing the universal binary, rather than merge. The latter could potentially be done, but it's going to involve most of the same steps anyway (because you're going to have to inspect every binary to see if it's universal or not, then work out if using one of the binaries is an option, and if so, which one...), so I opted for the "strip then merge" approach.

Copy link
Member

@mhsmith mhsmith left a comment

Choose a reason for hiding this comment

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

That all makes sense, but why didn't it fail in #1475's pre-merge checks? Is it because it uses a thread pool, so the order of the lipo commands was non-deterministic?

@freakboy3742
Copy link
Member Author

That all makes sense, but why didn't it fail in #1475's pre-merge checks? Is it because it uses a thread pool, so the order of the lipo commands was non-deterministic?

That's my best guess as well. When I run locally, I haven't been able to reproduce this error; and #1475 clearly had at least one CI pass (the last one) where it wasn't an issue. I'm guessing that the slower x86_64 CI machines are more prone to this problem, which is why we're seeing it manifest on most(?) CI runs.

@freakboy3742 freakboy3742 merged commit ce17261 into beeware:main Oct 9, 2023
@freakboy3742 freakboy3742 deleted the unique-dylibs branch October 9, 2023 09:50
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.

"Build app (ppb, macos-latest)" CI job is broken
2 participants