-
Notifications
You must be signed in to change notification settings - Fork 27
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
Command to deploy macOS/iOS universal binaries #53
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution, it is great that you are kicking off this effort.
There are some preliminary comments to start iterating, please have a look, some of them are architectural and are important, others are suggestions to simplify the examples, etc.
other_args.append(arg) | ||
i += 1 | ||
for profile in profiles: | ||
run(['conan', 'install', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running conan recursively is not possible, please check: https://docs.conan.io/2/knowledge/guidelines.html
So this functionality must be built using the conan_api
, not calling run(conan
subprocess. It is possible that some higher level abstractions could be used in the API, but in any case it is better to have here a low level API usage than re-entrant behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See below. I can look into the 2.0 conan_api
but at least in 1 it seemed difficult to read a conanfile multiple times with different profiles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😉 should work the same but just for settings
'-pr:h', profile | ||
] + other_args) | ||
output_dir = os.path.join('full_deploy', 'host') | ||
for package in os.listdir(output_dir): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not fully clear what is the outcome of this.
If we install
a dependency graph, with several dependency, the idea is that we will be creating a fat lipoed binary for every dependency in the graph?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the README needs a better explanation of the purpose. The goal is to produce Conan built libraries that can be turned into universal libraries and used outside of Conan. This should be as simple as possible, without trying to make universal packages or modifying recipes or toolchains. The '--deploy=full_deploy' option seems to be the same idea for single architecture situations.
The other complex point that was missed in the discussions under some other threads: this isn't just about supporting two or more architectures, but really multiple profiles. For example, I'd say it's pretty common in Xcode on macOS to target os.version=11.0 for armv8 (first supported OS) and something like os.version=10.13 for x86_64.
Since Conan packages are by design built around a single arch, os.version, etc. how do we do this in a single conan process? The sequence of commands should look like:
conan install --deploy=full_deploy -pr:h x86_64 . # Creates full_deploy/host/zlib/1.2.13/Release/x86_64/lib/libz.a
conan install --deploy=full_deploy -pr:h armv8 . # Creates full_deploy/host/zlib/1.2.13/Release/armv8/lib/libz.a
lipo full_deploy/host/zlib/1.2.13/Release/x86_64/lib/libz.a full_deploy/host/zlib/1.2.13/Release/armv8/lib/libz.a -create -output full_deploy/host/zlib/1.2.13/Release/lib/libz.a
# copy include files, etc to universal path as well
Now you can create an Xcode project outside of Conan with multiple architectures and references to files under 'full_deploy'. The conanfile.py is really only necessary to declare the dependencies, and maybe have build() run xcodebuild on the pre-existing Xcode project.
Any thoughts about how that fits as a custom command, or does this need to be a wrapper script outside of Conan?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above is good when conan install --requires=zlib/1.2.13
, because it has no dependencies. But I am thinking about the case for example of conan install --requires=boost/1.80
, where boost depends on zlib and bzip2. So the full_deploy
will deploy the 3 libraries, boost, zlib, and bzip2. What would be the expectaction/desired behavior in this case? To create a fat library for the 3 of them? like one fat boost, another fat zlib and another bzip2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly. In the example project under 'xcode' I'm depending on "libtiff/4.5.0" and the deployment produces:
jbig
libdeflate
libjpeg
libtiff
libwebp
xz_utils
zlib
zstd
This is why posting an example somewhere with an Xcode project is going to be really helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about a different approach that involves a deployer instead of the custom command weirdness (accepting all install command line arguments, and calling conan recursively). It would involve using lipo to replace a single arch at a time. The user would then run:
conan install -d lipo -pr x86_64 -b missing . # Creates 'full_deploy' as single arch universal binaries
conan install -d lipo -pr armv8 -b missing . # Runs 'lipo -replace' on 'full_deploy' to add the armv8 arch
conan build . # or xcodebuild
Does this seem like a better approach? (Let me make sure it's possible before closing this PR). It would be less efficient with a large number of archs but even on iOS it's just a few.
paths = [os.path.join(a, *(src_components[len(top_components):])) for a in arch_folders] | ||
paths = [p for p in paths if os.path.isfile(p)] | ||
if len(paths) > 1: | ||
run(['lipo', '-output', dst, '-create'] + paths, check=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to run self.conanfile.run()
because there, multiple things like environment definitions can be injected. Maybe not strictly necessary at this point, but could make sense and be more aligned with other parts of the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course. This code started life as a wrapper outside of Conan. If this turns out to be useful, it might make sense to contribute it as conan.tools.apple.lipo
in Conan itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, how do I get a conanfile here? For a custom command it looks like I have to load it (and it might be .txt or .py). I'm leaning towards the deployer, but that only has a dependency graph. Can I get the conanfile from that?
from conan.tools.apple import XcodeBuild | ||
|
||
|
||
class UniversalRecipe(ConanFile): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this test might not be necessary, but instead a test that does 2 conan create
, then runs the lipo
command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it's not useful as a test, but I think at least the README should link to an example somewhat, which could just be a separate repo. Having an Xcode project makes this easier to try out, and makes it clear that Conan is not creating it.
@@ -0,0 +1,1020 @@ | |||
// !$*UTF8*$! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the xcode project is not necessary, just running the deployer and validating the final library is perfectly good too cover it with testing.
See #58 for a different way to accomplish this. If you like that approach better, we can close this PR. |
This is a very rough version of a command to deploy dependencies and run lipo to create universal binaries. This could certainly use feedback and testing on iOS builds.