Refactored build-script-impl for cross-compiling support #2497

karwa · 2016-05-12T19:02:23Z

What's in this pull request?

In the current build-script-impl, we use the variable name "deployment_target" pretty loosely when talking about the Build, Host and Target of the products we're building. You can see this in places - like where the test phase iterates every STDLIB_DEPLOYMENT_TARGET (i.e. Target), before skipping those which are not, effectively, host platforms, so we're tripping up on all these types of "deployment targets" floating around.

I'm aware the build script is being rewritten in python. The rewrite is approaching the stage where it's going to be getting to some of the points I've addressed here (e.g. calculating stdlib targets), so I think it's a good idea if we get this done first, so the python implementation can consider it from the start.

This PR replaces uses of "deployment_target" with "host" when talking about a platform which will ultimately host a Swift compiler. The Build platform is implicitly a Host. It replaces uses of "deployment_target" with "stdlib_target" when talking about a platform for which the stdlib is to be built. This terminology is consistent with the actual swift/llvm build process: llvm_host_triple, SWIFT_HOST_VARIANT_* and host-related flags get set set_build_options_for_host instead of set_deployment_target_options, and SWIFT_STDLIB_TARGETS calculation has been moved to a function - calculate_targets_for_host.

SWIFT_STDLIB_TARGETS now gets recalculated for each host we build, as it should.
Executables are always searched for in the LOCAL_HOST (Build machine) products
Some switching on 'uname' with switches on the host, in support of future cross-compiling
Removed lipo step (it lipos stuff between hosts, which doesn't make sense - possibly a result of confused terminology again?)

Note: The diff is large because of things like moving the stdlib target calculation in to a function and relocating it to be close to the function which sets the host options. The actual changes are not that extreme, and really limited to some variable renaming, function encapsulation and moving.

Before merging this pull request to apple/swift repository:

Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
OS X platform	@swift-ci Please test OS X platform
Linux platform	@swift-ci Please test Linux platform

Note: Only members of the Apple organization can trigger swift-ci.

Use consistent terminology for target platforms, disambiguate uses of
'deployment_target'
Calculate targets per host, in preparation for cross-compiling
Only use executables from LOCAL_HOST
Only execute testable targets of the LOCAL_HOST
Remove some cases of switching on 'uname' in preparation for
cross-compiling

karwa · 2016-05-16T16:14:34Z

Diff is now more manageable

gribozavr · 2016-05-17T05:50:49Z

utils/build-script-impl

            SWIFT_HOST_VARIANT_ARCH="x86_64"
            ;;
+        linux-*)
+        SWIFT_HOST_VARIANT="linux"
+        SWIFT_HOST_VARIANT_SDK="LINUX"


Missing indentation?

gribozavr · 2016-05-17T06:11:02Z

The primary reason why I didn't use the Build/Host/Target terms is because it assumes one combination of the three, and does not work well with multi-target cross-compilers compilers, which Swift is.

gribozavr · 2016-05-17T06:14:04Z

utils/build-script-impl

-done
-
-if [[ "${CROSS_COMPILE_TOOLS_DEPLOYMENT_TARGETS}" ]] && [[ ! "${SKIP_MERGE_LIPO_CROSS_COMPILE_TOOLS}" ]] ; then
-    echo "--- Merging and running lipo for ${CROSS_COMPILE_TOOLS_DEPLOYMENT_TARGETS[@]} ---"


Did you intentionally drop this part? It is important for our internal workflows. Is this handled somehow by the primary build steps now?

Yes, but looking at it more closely, I should have put it behind a switch for the iOS targets.

The purpose of this step is to lipo together the cross-compiled compilers, and copy over the standard library, so that we would get a consistent installation layout ready to be deployed onto a target. This step is Darwin-specific, but other targets will need something similar.

I've added the lipo step back. The install process needed some adjustment to deal with it. Since it's a collection of multiple hosts, it gets placed in to a host-specific subdirectory "merged".

gribozavr · 2016-05-17T06:39:35Z

After reading the script more, I find the change to the plain "host/target" terms to be a readability loss. In the current script, I tried to use "stdlib-deployment-target" and "tools-deployment-target" to be extra clear about what is happening.

gribozavr · 2016-05-17T06:57:51Z

@swift-ci Please test

gribozavr · 2016-05-17T08:19:32Z

Some strange CI issues. Let's try again.

@swift-ci Please test

gribozavr · 2016-05-17T08:51:51Z

@karwa I just wanted to thank you and say that I deeply appreciate your work on the build system.

karwa · 2016-05-17T08:55:40Z

Happy to help!

gribozavr · 2016-05-22T11:05:34Z

@swift-ci Please test

gribozavr · 2016-05-23T00:50:45Z

@swift-ci Please test

shahmishal · 2016-05-24T23:44:32Z

utils/build-script-impl

+                    ;;
+                linux-armv7)
+                    SWIFT_HOST_VARIANT_ARCH="armv7"
+                    llvm_host_triple="${SWIFT_HOST_VARIANT_ARCH}-linux-gnueabihf"


Please substitute SWIFT_HOST_VARIANT_ARCH with armv7 for clarity.

I wanted to emphasise that the LLVM and Swift triples should be exactly the same. I've had issues with an "armv7-linux" Swift triple and "arm-linux" LLVM triple, for example. There is probably a better way to do this, such as a single "triple" setting that gets passed to both LLVM and Swift (we can now set SWIFT_HOST_TRIPLE explicitly when building, so that's an option)

That would probably be a good thing to do anyway. We have logic here to calculate the llvm_host_triple for OSX/iOS targets, but they duplicate that in SwiftConfigureSDK.cmake. The triples for the arm architectures are duplicated in CMakeLists.txt, and if we wanted to cross-compile hosts for other linux platforms, we'd have to have this triple logic in both places and they'd have to produce the exact same triples. We should do it in one place, and since both LLVM and Swift need it, it should be in the build script.

gribozavr · 2016-05-30T08:21:55Z

@karwa Thanks! The install tree looks a little funny, Swift does not seem to be installed into the toolchain:

swift-nightly-install/Library/Developer/Toolchains/swift-LOCAL-2016-05-27-a.xctoolchain/usr/bin/lldb # LLDB is OK
swift-nightly-install/usr/bin/swift # Swift is not

karwa · 2016-05-30T08:25:29Z

swift will be inside the toolchain:

swift-nightly-install/Library/Developer/Toolchains/swift-LOCAL-2016-05-27-a.xctoolchain/usr/bin/swift

gribozavr · 2016-05-30T08:33:21Z

@karwa Sorry, I don't understand you... I built a toolchain and the file list I posted is what I got. Are you saying that you are working on a fix?

karwa · 2016-05-30T08:39:35Z

No, I'm saying that swift is installed where it should be - inside the toolchain directory. The path you posted was in the install_destdir itself.

This is the tree I got from build_toolchain:
toolchain.txt

gribozavr · 2016-05-30T08:39:45Z

Looks like this is the issue:

DESTDIR="${host_install_destdir}" "${CMAKE}" --build "${build_dir}" -- ${INSTALL_TARGETS}

Should it use ${host_install_destdir}${host_install_prefix} instead?

gribozavr · 2016-05-30T08:44:03Z

The path you posted was in the install_destdir itself.

Well, yes, that's the issue I'm pointing out. The tree I posted is what I got from build-toolchain, too.

gribozavr · 2016-05-30T08:44:35Z

@swift-ci Please test

karwa · 2016-05-30T08:44:38Z

No I'm still pretty sure this is right - the install prefix wasn't in the DSTDIR variable before these changes, either. Swift gets this information from the CMAKE_INSTALL_PREFIX variable.

If you have an extra copy of swift, maybe it's left over from another build?

gribozavr · 2016-05-30T08:44:47Z

Let's see if it reproduces in CI.

gribozavr · 2016-05-30T08:48:02Z

No I'm still pretty sure this is right - the install prefix wasn't in the DSTDIR variable before these changes, either. Swift gets this information from the CMAKE_INSTALL_PREFIX variable.

But when we call the CMake action to install, we override the directory with DESTDIR environment variable.

If you have an extra copy of swift, maybe it's left over from another build?

It is not an extra copy, it is the only copy. (And I'm always doing from-scratch builds, removing the ../build directory and swift-nightly-* directories.)

gribozavr · 2016-05-30T08:49:55Z

I will also build another toolchain without this PR to check which commands were run before.

karwa · 2016-05-30T08:57:53Z

utils/build-script-impl

@@ -1546,32 +1767,32 @@ for deployment_target in "${HOST_TARGET}" "${CROSS_COMPILE_TOOLS_DEPLOYMENT_TARG

                cmake_options=(
                    "${cmake_options[@]}"
-                    -DCMAKE_INSTALL_PREFIX:PATH="${INSTALL_PREFIX}"
-                    -DLLVM_CONFIG:PATH="$(build_directory_bin ${deployment_target} llvm)/llvm-config"
+                    -DCMAKE_INSTALL_PREFIX:PATH="$(get_host_install_prefix ${host})"


This is where we set the install prefix for swift. It's in CMakeCache.txt. Should be:

CMAKE_INSTALL_PREFIX:PATH=/Library/Developer/Toolchains/swift-LOCAL-2016-05-29-a.xctoolchain/usr

Yes, this is exactly right, and it is what I'm seeing. So the issue is either somewhere else, or it is my local misconfiguration. See my other comment -- I'm running another build.

gribozavr · 2016-05-30T08:59:41Z

No I'm still pretty sure this is right - the install prefix wasn't in the DSTDIR variable before these changes, either. Swift gets this information from the CMAKE_INSTALL_PREFIX variable.

But when we call the CMake action to install, we override the directory with DESTDIR environment variable.

I finally see what you meant, sorry, I misunderstood you. I'm building again now. I can confirm that CMAKE_INSTALL_PREFIX for LLVM and Swift are correct (and my other build confirms that it is the same prefix as before), now waiting for the installation step on the new build.

gribozavr · 2016-05-30T21:22:07Z

I just built the toolchains again on OS X and Linux, and everything matches up exactly! So it was a misconfiguration on my side, sorry for wasting your time, @karwa.

karwa · 2016-05-30T21:27:10Z

@gribozavr Thank you so much for all your help and for reviewing it. Also to @rintaro and everybody else.

jrose-apple · 2016-10-25T17:17:51Z

I can't tell for sure, but it looks like this commit introduced Linux triples of the form "armv7-linux-gnueabihf" instead of "armv7-unknown-linux-gnueabihf", causing SR-3019. Was this intentional?

karwa · 2016-10-25T18:40:39Z

@jrose nope. I picked that to be as close as possible to the GCC triple arm-linux-gnueabihf (but I don't think anybody uses armv6/7 on GCC). I wasn't aware that triples in LLVM needed a fourth component.

jrose-apple · 2016-10-25T20:39:38Z

They need three components, but the second component has to be a vendor. Are you sure the GCC triple is arm-linux-gnueabihf and not arm-unknown-linux-gnueabihf?

jrose-apple · 2016-10-25T20:40:08Z

Okay, I could have just checked for that, indeed it is.

karwa · 2016-10-25T20:46:26Z

Yeah sorry, you're right that the second component should be "unknown" (rustc apparently uses that), but GCC seems to use an incorrect triple: http://packages.ubuntu.com/precise/devel/gcc-arm-linux-gnueabihf

I've been having some issues with LLDB reloading images because of mismatching architectures. I'll investigate if this could be the reason for it...

jrose-apple · 2016-10-25T20:55:16Z

I doubt it is, but we should pull in a fix anyway.

karwa force-pushed the refactored-build-script branch 2 times, most recently from 13c2e34 to 1513d54 Compare May 16, 2016 16:07

gribozavr reviewed May 17, 2016
View reviewed changes

karwa force-pushed the refactored-build-script branch 3 times, most recently from 8161717 to af33777 Compare May 18, 2016 04:39

shahmishal reviewed May 24, 2016
View reviewed changes

karwa reviewed May 30, 2016
View reviewed changes

gribozavr merged commit d54bb62 into swiftlang:master May 30, 2016

gribozavr mentioned this pull request May 31, 2016

Allow running iphoneos-armv7s tests #2785

Merged

1 task

karwa mentioned this pull request May 31, 2016

[build-script] never expand install prefix as relative path #2795

Merged

1 task

karwa deleted the refactored-build-script branch June 1, 2016 23:36

modocache mentioned this pull request Sep 15, 2016

Port of Foundation to Android swiftlang/swift-corelibs-foundation#622

Merged

karwa mentioned this pull request Oct 25, 2016

Change arm linux triples #5460

Merged

finagolfin mentioned this pull request Dec 20, 2021

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

Merged

modocache mannequin mentioned this pull request Jun 6, 2016

[SR-1362] Android: Swift/Android on Mac OS X #43971

Closed

Refactored build-script-impl for cross-compiling support #2497

Refactored build-script-impl for cross-compiling support #2497

Uh oh!

Conversation

karwa commented May 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's in this pull request?

Uh oh!

karwa commented May 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribozavr commented May 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribozavr commented May 17, 2016

Uh oh!

gribozavr commented May 17, 2016

Uh oh!

gribozavr commented May 17, 2016

Uh oh!

gribozavr commented May 17, 2016

Uh oh!

karwa commented May 17, 2016

Uh oh!

gribozavr commented May 22, 2016

Uh oh!

gribozavr commented May 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karwa May 25, 2016 • 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!

gribozavr commented May 30, 2016

Uh oh!

karwa commented May 30, 2016

Uh oh!

gribozavr commented May 30, 2016

Uh oh!

karwa commented May 30, 2016

Uh oh!

gribozavr commented May 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gribozavr commented May 30, 2016

Uh oh!

gribozavr commented May 30, 2016

Uh oh!

karwa commented May 30, 2016

Uh oh!

gribozavr commented May 30, 2016

Uh oh!

gribozavr commented May 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gribozavr commented May 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribozavr commented May 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gribozavr commented May 30, 2016

Uh oh!

karwa commented May 30, 2016

Uh oh!

jrose-apple commented Oct 25, 2016

Uh oh!

karwa commented Oct 25, 2016

karwa commented May 12, 2016 •

edited

Loading

karwa May 25, 2016 •

edited

Loading

gribozavr commented May 30, 2016 •

edited

Loading

gribozavr commented May 30, 2016 •

edited

Loading

gribozavr commented May 30, 2016 •

edited

Loading