[android] Support building the host tools and with the static stdlib #34963
Conversation
There was a problem hiding this comment.
Is this three unrelated changes? There seems to be the change to not use SWIFT_CONFIGURED_SDKS, which seems incomplete (per the previous comment). Then the change in the static-stdlib-args.lnk. And then the changes in the build-script-impl. Or are they necessary for each other?
I would recommend using independent PRs for each change. Easier to review, and easier to understand.
For the changes in build-script-impl you probably want to provide a "test plan". I don't think the standard workflows will exercise those code paths, so it is complicated to evaluate if they are correct without knowing what they intend to do.
| @@ -1756,7 +1768,8 @@ for host in "${ALL_HOSTS[@]}"; do | |||
| ) | |||
| fi | |||
|
|
|||
| if [[ ! "${SKIP_BUILD_ANDROID}" ]]; then | |||
| if [[ ! "${SKIP_BUILD_ANDROID}" ]] || | |||
| [[ $(is_cross_tools_host ${host}) && "${host}" == "android-"* ]]; then | |||
There was a problem hiding this comment.
Is this because if you don't skip building Android, the stdlib would also be built? Isn't there a cleaner or clearer way of doing this?
There was a problem hiding this comment.
No, this is a minor addition to signify that the NDK can also be used to cross-compile the host tools and not just the Android stdlib, as SKIP_BUILD_ANDROID confusingly just means "don't build the Android stdlib." It fixes a bug I hit where I was only cross-compiling the host tools for Android and not the stdlib, which the unmodified version of this check would screw up.
There was a problem hiding this comment.
I understand that you are trying to cross-compile the tools. What I don't understand is that all these variables (if I remember correctly) are used during the compilation of the stdlib. Some host tools use Swift, and so they require a stdlib to be built, so I don't understand how the stdlib is both skipped, and used at the same time.
There was a problem hiding this comment.
I don't believe that any of the Swift host tools, like the Swift compiler or SourceKit, use any Swift yet, as I was building the host tools alone till earlier this year. A search for .swift files in the lib/ and tools/ directories only turns up anything in tools/swift-inspect, which I don't think is built by default.
This is the last use of
Other than the two uses in this pull that I'm changing, that's it: I am simply fixing this bug, where you pass in |
As mentioned in the OP, these are odds and ends I needed when building a full Swift toolchain for Android. Since this is so small, I'd rather just keep it all in one pull here.
Since those involve building a Swift toolchain that will run natively on Android, I'm the only one currently using them and they're not currently tested by any CI. |
There was a problem hiding this comment.
About the unrelated changes: it is not the first time I ask you to split unrelated changes, and because the pull requests with unrelated changes keep coming, I am afraid it would not be the last. It is not a pet peeve of mine or anything, it is how the Contributing Code guidelines (https://swift.org/contributing/#contributing-code) describe it should be done. It makes everyone reviewing your code spend more time looking at the changes, trying to understand how the changes are supposed to be related. Splitting also benefits you: the change for static-stdlib-args.lnk haven't had any comment, but it is being hold back because the other changes that do not affect it.
About the testing plan: specially because they are not currently tested in CI, if someone wants to verify this changes are solving a problem, what can they do to verify that the code is correct. Besides solving the problem, it is also important to explain what the problem was, and how it was solved.
Finally, about the modifications in build-script-impl/build-script. As far as I understand those files were supposed to be used for CI, but they have been started to be used for day to day development. Adding specifics for other purposes only complicates those files even more (because most people do not have access to Termux and cannot check that their modifications to unbreak CI will not break Termux in the process). For specific systems/flavors packaging, it might be better to have an external set of scripts using the underlying CMake invocations (that's how the Windows packaging works, for example). Just my two cents.
| @@ -1756,7 +1768,8 @@ for host in "${ALL_HOSTS[@]}"; do | |||
| ) | |||
| fi | |||
|
|
|||
| if [[ ! "${SKIP_BUILD_ANDROID}" ]]; then | |||
| if [[ ! "${SKIP_BUILD_ANDROID}" ]] || | |||
| [[ $(is_cross_tools_host ${host}) && "${host}" == "android-"* ]]; then | |||
There was a problem hiding this comment.
I understand that you are trying to cross-compile the tools. What I don't understand is that all these variables (if I remember correctly) are used during the compilation of the stdlib. Some host tools use Swift, and so they require a stdlib to be built, so I don't understand how the stdlib is both skipped, and used at the same time.
|
@swift-ci please test |
|
Build failed |
I have split off pulls on your recommendation in the past, even submitting one-line commits if they affect other OS platforms. In this case, there is a common theme: everything is scoped to Android. I understand that any increase in commit size takes longer to review, but this pull is so small that I'm fine with that.
I'm not pulling these changes out of thin air, they are all currently used by my open-source patches to cross-compile the Swift toolchain for the OSS Termux app for Android.
On your prompting above, I've added additional comments that now explain everything.
I don't know that that was ever the case, new users who build this project are prompted to use those scripts for "day to day development" too.
Not if they're properly scoped, as these changes are, and these build scripts have long included support for niche platforms like Cygwin and Haiku that are not tested by any CI, and have bitrotted in the way you point out. I'm well aware that might happen for any Android-specific modifications too, and since I maintain the Termux package of the Swift toolchain, I will simply update those when necessary. It has not been a problem so far. Thanks for running the CI, macOS failure when running |
|
Could someone run the Mac CI again? |
|
@swift-ci test macos |
|
Passed CI, ready to merge. |
|
Ping, this is ready to go. |
|
@drodriguez, ready to go? |
|
@varungandhi-apple, this has been ready for a couple weeks now, mind getting it in? |
|
(Re-runnning CI since the last pass was over a week back) @swift-ci please test |
|
Ping, a final CI run and this can be merged. |
|
@swift-ci test |
|
Build failed |
|
Build failed |
|
CI is broken right now because of lldb issue. |
|
@varungandhi-apple, if the CI is not broken because of other pulls, could you retest this and merge? |
|
@swift-ci test |
|
@drodriguez is this okay to merge/are there changes that should be made? I skimmed through your comments and I'm not sure. |
|
I think I remember running this in my local Android CI, and didn't break anything (but that was more than a month ago). If it creates problem when merged, we can always revert and rework. |
Some odds and ends I needed when cross-compiling the entire toolchain for Android. @drodriguez, @compnerd, and @drexin, I believe you are best qualified to review this.