[build] Add a flag that allows disabling appending the host target's name to the install-destdir for a cross-compiled toolchain #40633

finagolfin · 2021-12-20T15:42:45Z

This is useful if you're cross-compiling the toolchain for a single host and don't want your specified install path modified.

I have been patching out the hostname for years when cross-compiling the toolchain for Android, so I just came up with this build-script flag instead, finagolfin/termux-packages@17cb147, and that worked. Another advantage is when cross-compiling standalone SDKs, these unnecessary hostname directories won't be applied.

I'm not particular about hacking in a build flag like this, so if someone has a better idea, I'm all ears. @karwa, you added this in #2497 more than five years ago and @gribozavr approved it, any input welcome.

karwa · 2021-12-21T06:17:34Z

I have no idea, sorry. It's been too long, and the build script has totally changed in that time.

Although I think "hostname" in this case might be a little ambiguous. When I looked at it just now, I thought it would be appending a name like "Karls-Laptop" (i.e. a network hostname)... which would be alarming and not making a lot of sense.

I feel like there are already so many flags, it's hard to know what any of them do or why you'd use them. Ideally at least their names would be extremely clear.

finagolfin · 2021-12-21T10:38:07Z

the build script has totally changed in that time.

I was surprised to find that this installation code is untouched since you wrote it in 2016. Of course, not many people cross-compile the Swift toolchain, so that's why.

I feel like there are already so many flags, it's hard to know what any of them do or why you'd use them.

Most come with a short description in the help, as I've written here, so that usually hasn't been a problem for me.

finagolfin · 2022-01-05T21:50:48Z

@kubamracek, would you review?

kubamracek · 2022-01-05T23:14:26Z

Looks fine to me, maybe I'd suggest some shorter name for the flag, but actually I think @compnerd, @edymtt and @gottesmm are probably better folks to review this change.

edymtt

The general approach looks good to me -- we are still relying on build-script-impl for part of our build logic, and in this case I don't think we can avoid updating it.

My only suggestion would be to rename the flag -- more details in the comments (start from utils/build_swift/build_swift/driver_arguments.py)

utils/swift_build_support/swift_build_support/build_script_invocation.py

utils/build-script-impl

utils/build_swift/build_swift/driver_arguments.py

…name to the install-destdir for a cross-compiled toolchain This is useful if you're cross-compiling the toolchain for a single host and don't want your specified install path modified.

finagolfin · 2022-01-17T17:32:42Z

@edymtt, made all the changes you asked for, ready for review.

edymtt

That looks good to me, thanks a lot for incorporating my feedback!

edymtt · 2022-01-19T20:13:23Z

utils/build_swift/build_swift/driver_arguments.py

+           default=True,
+           help="Append each cross-compilation host target's name as a subdirectory "
+                "for each cross-compiled toolchain's destdir, useful when building "
+                "multiple toolchains and can be disabled if only cross-compiling one.")


I like how you phrased this description -- in particular how concise it is.

edymtt · 2022-01-19T20:17:50Z

utils/build-script-impl

-        elif [[ "${host}" == "merged-hosts" ]]; then
-            # This assumes that all hosts are merged to the lipo.
+        elif [[ "${host}" == "merged-hosts" ]] ||
+             [[ "$(true_false ${CROSS_COMPILE_APPEND_HOST_TARGET_TO_DESTDIR})" == "FALSE" ]]; then


This turned out way better than I imagined -- thanks for reminding me about the true_false facility.

edymtt · 2022-01-19T20:19:41Z

@swift-ci please smoke test

edymtt · 2022-01-19T20:19:56Z

@swift-ci please test Apple Silicon

edymtt · 2022-01-19T20:20:59Z

preset=stdlib_S_standalone_minimal_macho_x86_64,build,test
@swift-ci please test with toolchain and preset

edymtt · 2022-01-19T20:21:29Z

@swift-ci please python lint

edymtt · 2022-01-19T20:23:44Z

@swift-ci Please Build Toolchain

swift-ci · 2022-01-19T21:01:27Z

Build failed
Swift Test OS X Platform
Git Sha - 3e77176

edymtt · 2022-01-19T21:47:09Z

@swift-ci please smoke test macOS

edymtt · 2022-01-19T21:50:03Z

stdlib_S_standalone_minimal_macho_x86_64,build,test is failing with the same error as the incremental job, e.g. https://ci.swift.org/job/oss-swift-test-stdlib-with-toolchain-minimal/1219/console

12:15:56 ******************** TEST 'Swift(freestanding-x86_64) :: Interop/Cxx/stdlib/use-std-vector.swift' FAILED ********************
<snip>
12:15:56 --
12:15:56 Exit Code: 1
12:15:56 
12:15:56 Command Output (stderr):
12:15:56 --
12:15:56 Undefined symbols for architecture x86_64:
12:15:56   "__ZNSt3__17forwardIDnEEOT_RNS_16remove_referenceIS1_E4typeE", referenced from:
12:15:56       __ZNSt3__117__compressed_pairIPiNS_9allocatorIiEEEC2IDnNS_18__default_init_tagEEEOT_OT0_ in lto.o
12:15:56       __ZNSt3__122__compressed_pair_elemIPiLi0ELb0EEC2IDnvEEOT_ in lto.o
<snip>

edymtt · 2022-01-19T21:51:43Z

@swift-ci please smoke test Linux

finagolfin · 2022-01-19T22:55:34Z

All tests should pass, since cross-compiling the toolchain for non-Darwin platforms isn't tested on the CI.

swift-ci · 2022-01-19T23:41:02Z

Linux Toolchain (Ubuntu 16.04)
Download Toolchain
Git Sha - 3e77176

Install command
tar zxf swift-PR-40633-781-ubuntu16.04.tar.gz
More info

finagolfin · 2022-01-20T07:18:17Z

Failing macOS tests are unrelated to this pull.

kubamracek requested review from compnerd, edymtt and gottesmm January 5, 2022 23:13

edymtt requested changes Jan 12, 2022

View reviewed changes

utils/swift_build_support/swift_build_support/build_script_invocation.py Outdated Show resolved Hide resolved

utils/build-script-impl Outdated Show resolved Hide resolved

utils/build_swift/build_swift/driver_arguments.py Outdated Show resolved Hide resolved

[build] Add a flag that allows disabling appending the host target's …

3e77176

…name to the install-destdir for a cross-compiled toolchain This is useful if you're cross-compiling the toolchain for a single host and don't want your specified install path modified.

finagolfin force-pushed the cross-compile-install-path branch from d113d7b to 3e77176 Compare January 17, 2022 17:30

finagolfin changed the title ~~[build] Add a flag to disable appending the hostname to the install-destdir for a cross-compiled toolchain~~ [build] Add a flag that allows disabling appending the host target's name to the install-destdir for a cross-compiled toolchain Jan 17, 2022

edymtt approved these changes Jan 19, 2022

View reviewed changes

edymtt merged commit 45aeca8 into swiftlang:main Jan 20, 2022

finagolfin deleted the cross-compile-install-path branch January 20, 2022 18:01

finagolfin mentioned this pull request Jan 23, 2022

[5.6][build] Make it easier to cross-compile the stdlib and use it #40976

Closed

[build] Add a flag that allows disabling appending the host target's name to the install-destdir for a cross-compiled toolchain #40633

[build] Add a flag that allows disabling appending the host target's name to the install-destdir for a cross-compiled toolchain #40633

Uh oh!

Conversation

finagolfin commented Dec 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karwa commented Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

finagolfin commented Dec 21, 2021

Uh oh!

finagolfin commented Jan 5, 2022

Uh oh!

kubamracek commented Jan 5, 2022

Uh oh!

edymtt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

finagolfin commented Jan 17, 2022

Uh oh!

edymtt left a comment

Choose a reason for hiding this comment

Uh oh!

edymtt Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

edymtt Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

edymtt commented Jan 19, 2022

Uh oh!

edymtt commented Jan 19, 2022

Uh oh!

edymtt commented Jan 19, 2022

Uh oh!

edymtt commented Jan 19, 2022

Uh oh!

edymtt commented Jan 19, 2022

Uh oh!

swift-ci commented Jan 19, 2022

Uh oh!

edymtt commented Jan 19, 2022

Uh oh!

edymtt commented Jan 19, 2022

Uh oh!

edymtt commented Jan 19, 2022

Uh oh!

finagolfin commented Jan 19, 2022

Uh oh!

swift-ci commented Jan 19, 2022

Uh oh!

finagolfin commented Jan 20, 2022

Uh oh!

Uh oh!

finagolfin commented Dec 20, 2021 •

edited

Loading

karwa commented Dec 21, 2021 •

edited

Loading

edymtt left a comment •

edited

Loading