Android: add cross-compilation configuration file flag #3403
Conversation
finagolfin
requested review from
abertelrud,
friedbunny,
neonichu and
tomerd
as code owners
I think this is actually a bug and I was planning to fix it. Any binaries we are installing should be built during the second stage, anything build by CMake should only be there temporarily. |
OK, I'm referring specifically to these libraries that are only built by CMake right now. |
|
Yep, we're talking about the same thing. I realize that they are currently being built by CMake only. We found this issue independently of this PR and I am planning to fix it soon, that's why I wanted to bring it up here. Potentially, it would make sense to wait with this PR until that issue is fixed because then we don't need the second flag, if I understand this correctly. |
|
Yes, if two of those libraries were generated by the SPM build also (one of the PD libraries is built by SPM, but not installed), then the first flag added by this pull isn't needed. It is only needed because the build currently installs those CMake-generated libs. I can wait if you want to fix the build first. |
| if args.cross_compile_hosts: # Use XCBuild target directory when building for multiple arches. | ||
| args.target_dir = os.path.join(args.build_dir, "apple/Products") | ||
| if args.cross_compile_hosts: | ||
| if "macosx-arm64" in args.cross_compile_hosts: |
There was a problem hiding this comment.
I don't think that this is correct; there are a number of targets that may be used, can you just invert the condition here instead?
if args.cross_compile_hosts:
if re.match('android-', args.cross_compile_hosts):
args.target_dir = os.path.join(args.build_dir, get_build_targets(args, cross_compile=True))
else:
args.target_dir = os.path.join(args.build_dir, 'apple/Products')
Yes, it isn't great, but leaves the behaviour the same while adding support for your use case.
There was a problem hiding this comment.
No, this is right, see the similar logic at the end of the script. Basically, Mishal only added support for cross-compiling SPM for macOS arm64 and he explicitly spelled that out below, but didn't bother here: I went ahead and made it explicit here too, now that I added Android also.
Done, although this script already has a bunch of long lines that I didn't add.
I'm not sure what "infrastructure" refers to in this context. |
|
@swift-ci please test |
|
CI failures in the lldb tests on linux and building the compiler on macOS are unrelated to this pull. |
|
@swift-ci please smoke test |
|
@neonichu have your concerns here been addressed and this is ready to be merged? |
|
@buttaface please note we have branched 5.5 to release/5.5 so once this is merged we will need a corresponding PR if we need this to be included in 5.5 |
|
Sigh, I made one last formatting change without testing and Python's complaining about that on the CI: I will never understand where line breaks are allowed in Python. This pull works with the way the SPM build currently builds some release libraries with CMake and most everything else with the bootstrap SPM, but Boris wants to move the SPM build to a full SPM bootstrap. I'm fine with waiting for that change because it will allow me to simplify this pull and remove one of the bootstrap script flags I otherwise need, provided changing the build to SPM only doesn't take months or something. I will submit a pull for the 5.5 branch once this is in. |
|
@abertelrud, I understand that you're looking to fix #3431 now by merging the two PD libraries, any plans to merge that back to the 5.5 branch? If not, I'd like to get this pull into trunk and 5.5 now and I will rework these changes, that cross-compile the two PD libraries with CMake, in another pull to adapt to when you get that PD merge pull into trunk. If you plan to merge the two PD libraries in 5.5 too, then I will wait till you get that into trunk and rework this pull then. |
|
I put up #3464 now, though it's still a WIP. I'm not sure whether it's in scope for 5.5 but I think it would be good if the question there can be cleared up (about unsafe compiler flags in the manifest). I will wrap up the bootstrap script changes there today or tomorrow and then there should be a discussion about whether that change can go into 5.5. Which will then affect this PR. |
|
I don't have an opinion about whether this is merged first though — I think that the bootstrap script will need to be modified either way, and if this is merged first, I can easily adapt #3464. |
|
Alright, I will wait till a decision is made on whether #3464 should go into 5.5 and then modify this as needed. |
|
Looks good to me given the current somewhat unfortunate implementation of universal binaries for the macOS toolchain. I have requested that the compiler scripts change to using @tomerd @neonichu any concerns with this change to unblock Android? |
|
Actually: @buttaface what is the semantic distinction here between |
|
Ideally, nothing different. |
Thanks for clarifying. I think it makes sense to merge this to unblock Android, but over time it seems that the script can be cleaned up. I think that can be done in separate PRs. Would it make sense to make |
|
I actually don't think |
|
Hold on, I just thought of a way to do this without that additional flag, will try it and update this pull if it works. |
That would be great! I don't want to hold this up any longer, but any simplification to what's already a complicated situation would be great. |
Thanks for the reference to that discussion. I think this gets to a concern I have with the terminology here, and the conflation of platform and architecture when speaking of cross-compilation. In my view it doesn't make sense to cross-compile for, say, Windows and Darwin or Android and Linux in a single invocation of the bootstrap script, so really, only one target platform makes sense. But within that platform, it makes sense to either choose the architectures for which to compile or have a "default vs all" switch. While the target triples are currently presented as a flat list, it's really only a single platform with one or more architectures that makes any sense for a single given build. I think ideally it should be possible to specify a target platform and then either a set of target architectures for that platform, or perhaps simpler, a single flag that says "build all architectures for the platform" as opposed to building just a single default architecture (which would, for example, be the architecture of the host). Perhaps that won't suffice for some platforms, where it isn't the host, and there are also multiple architectures to choose from. |
| try: | ||
| command = [args.swiftc_path, '-print-target-info'] | ||
| if cross_compile: | ||
| command += ['-target', args.cross_compile_target_triple] | ||
| cross_compile_json = json.load(open(args.cross_compile_config)) | ||
| command += ['-target', cross_compile_json["target"]] |
There was a problem hiding this comment.
Alright, this works and allows me to remove the extra triple flag. I just realized that the destination.json is just a JSON file, so we can parse it to get the target triple.
@abertelrud, if you agree with this change, just let me know and I will squash it into the first commit and update the commit message and we can get this in.
There was a problem hiding this comment.
Thanks @buttaface, simplifying this down to get the information from the JSON instead of the having to pass another flag is a good improvement. I still want to hear what @neonichu and @tomerd think but this looks good given how things currently work. I will still want to work with @shahmishal to see if we can simplify the conceptual model around building for various platforms and architectures.
There was a problem hiding this comment.
Alright, will squash this commit and update the message.
Yeah, we could reorganize it that way. As I mentioned before, I think |
Absolutely, it was all very understandable at the time. Just thoughts about how to improve on this step-by-step. |
|
Ready to go. |
In addition to '--cross-compile-hosts', add a new bootstrap flag, '--cross-compile-config', that will pass in a JSON file with the destination cross-compilation flags.
finagolfin
changed the title
|
@neonichu, I think you just need to sign off on this? I have updated my build-script pull, swiftlang/swift#36917, that shows how this new bootstrap flag will be invoked. |
|
@swift-ci please smoke test |
|
@neonichu @buttaface This is currently marked with the |
|
Yeah, that tag was added two months ago, when this pull wasn't building. Several changes were made since then, please remove the tag and merge. |
|
Thanks for the PR @buttaface! |
|
Thanks for finally getting this in, I will submit for the 5.5 branch. |
In addition to '--cross-compile-hosts', add a new bootstrap flag, '--cross-compile-config', that will pass in a JSON file with the destination cross-compilation flags.
Motivation:
This enables cross-compiling SPM for other hosts, such as Android.
Modifications:
In addition to
--cross-compile-hosts, add a new bootstrap flag,--cross-compile-config, that will pass in a JSON file with the destination cross-compilation flags.Result:
SPM can be cross-compiled for another OS, with Android as the first example.