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

Minimal proof of concept of universal binaries support for CMakeToolchain #15775

Merged
merged 19 commits into from
Mar 5, 2024

Conversation

czoido
Copy link
Contributor

@czoido czoido commented Feb 28, 2024

Changelog: Feature: Add basic support in CMakeToolchain for universal binaries.
Docs: conan-io/docs#3642

Copy link
Contributor

@jcar87 jcar87 left a comment

Choose a reason for hiding this comment

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

This looks great!

While we have got requests to support something like this, one detail that is often neglected is that this may not work at all . That is, if the build scripts have logic that is conditional on the target architecture (whether this is CMake, autotools, etc), it may not be possible to generate a universal binary in the same pass, or worse, it may generate one but one of the two is missing some crucial options. By this I mean any logic that is contingent on, say, whether CMAKE_CROSSCOMPILING is enabled, or having logic around CMAKE_SYSTEM_PROCESSOR - etc.

So while it "may be possible" for a binary to exist with arch=x86_64-armv8 - it may be that it is impossible to generate it in a single pass, and may require generating the files "independently" for each architecture and them "fusing" them together. Which makes me wonder if we should consider a way of creating a multi-arch package from two previous existing single-arch packages. Food for thought :D

@memsharded
Copy link
Member

So while it "may be possible" for a binary to exist with arch=x86_64-armv8 - it may be that it is impossible to generate it in a single pass, and may require generating the files "independently" for each architecture and them "fusing" them together.

I think this idea has potential, I'd probably try to prioritize this, as it would be a way more general solution, valid for any build system, not adding extra complexity on the recipe+build-system side. It sounds a bit complex with the current model but with some ideas and building blocks, like what it is done in compatibility, it might be doable.

@czoido
Copy link
Contributor Author

czoido commented Feb 28, 2024

This looks great!

While we have got requests to support something like this, one detail that is often neglected is that this may not work at all . That is, if the build scripts have logic that is conditional on the target architecture (whether this is CMake, autotools, etc), it may not be possible to generate a universal binary in the same pass, or worse, it may generate one but one of the two is missing some crucial options. By this I mean any logic that is contingent on, say, whether CMAKE_CROSSCOMPILING is enabled, or having logic around CMAKE_SYSTEM_PROCESSOR - etc.

So while it "may be possible" for a binary to exist with arch=x86_64-armv8 - it may be that it is impossible to generate it in a single pass, and may require generating the files "independently" for each architecture and them "fusing" them together. Which makes me wonder if we should consider a way of creating a multi-arch package from two previous existing single-arch packages. Food for thought :D

Yes, absolutely true, this will not work in some cases. For those cases maybe we should use something similar to this: conan-io/conan-extensions#116
Anyway, I think this could be useful in some cases.

@czoido czoido added this to the 2.2.0 milestone Feb 29, 2024
@czoido czoido marked this pull request as ready for review February 29, 2024 14:32
@czoido czoido closed this Feb 29, 2024
@czoido czoido reopened this Feb 29, 2024
conan/tools/cmake/toolchain/blocks.py Outdated Show resolved Hide resolved
conan/tools/apple/apple.py Outdated Show resolved Hide resolved
conan/tools/cmake/toolchain/blocks.py Outdated Show resolved Hide resolved
@memsharded memsharded self-assigned this Mar 4, 2024
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Looking good, thanks!

conan/internal/internal_tools.py Show resolved Hide resolved
@memsharded memsharded merged commit 4159cc3 into conan-io:develop2 Mar 5, 2024
2 checks passed
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.

3 participants