[build-script] Compute targets in `build-script` #2880

ddunbar · 2016-06-04T05:49:52Z

What's in this pull request?

This is a work-in-progress branch to move the computation of the target list up to build-script. It does size by inventing a new mechanism to pass host-specific variables down to build-script-impl, and it also factors out explicit objects for platform/target to attach metadata to.

Finally, this branch itself doesn't actually remove the build-script-impl logic. Instead, it runs both implementations and asserts that they compute the same result -- the logic itself is hairy so this has been a good source of catching bugs in the implementation for all the different flag combinations.

Resolved bug number: (SR-)

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
All supported platforms	@swift-ci Please smoke test and merge
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
All supported platforms	@swift-ci Please test and merge
OS X platform	@swift-ci Please test OS X platform
OS X platform	@swift-ci Please benchmark
Linux platform	@swift-ci Please test Linux platform

Lint Testing

Language	Comment
Python	@swift-ci Please Python lint

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

ddunbar · 2016-06-04T05:51:17Z

/cc @gribozavr @rintaro @modocache

I'd like to clean this up a bit more before landing, but I'm posting it now in case anyone has feedback on the approach. The good news is that this will allow us to start ripping out much more of the code in build-script-impl once it lands and we are comfortable ripping out the validation logic.

rintaro · 2016-06-04T08:31:28Z

After this PR land, direct use of build-script-impl is almost impossible.
I think, it's time to hide it in Python package directory.
master...rintaro:build-script-hide-impl

karwa · 2016-06-06T03:48:02Z

utils/swift_build_support/swift_build_support/targets.py

+        self.name = name
+        self.targets = [Target(self, arch) for arch in archs]
+        self.sdk_name = name.upper() if sdk_name is None else sdk_name
+        self.is_darwin = is_darwin


Why is the is_darwin flag here? Couldn't we just check if a Platform is a DarwinPlatform or not via the class hierarchy?

Yeah, it is just ends up being a lot more verbose for clients (and we branch on this frequently). I could of course have just made it a computed property using that, though, which makes more sense... I will fix that.

ddunbar · 2016-06-06T15:36:55Z

@rintaro I agree about the build-script becoming unusable, but I'm not sure if it is worth moving it... my hope is that we continue to iterate quickly on just eliminating it, and in practice anyone using it has to migrate no matter what.

gribozavr · 2016-06-06T23:48:27Z

utils/build-script

+            target.name
+            for target in stdlib_targets])
+    targets_group.add_argument(
+        "--build-stdlib-deployment-targets",


I'd like to have a consistent model (from the user's perspective) between cross-compiling the tools and the library. What about --cross-compile-tools-for-targets and --cross-compile-stdlib-for-targets?

I'm just porting over existing names from build-script-impl, can we separate out renames/behavior changes? Or are you trying to ensure it gets cleaned up as we port?

I'd be OK with adding these as FIXMEs.

Actually, I'm not even sure I understand the comment. This flag is just a filter on --stdlib-deployment-targets... what makes those names more consistent?

Sorry, re-reading now I see that my initial message was confusing. What I meant is:

--cross-compile-hosts => --cross-compile-tools-for-hosts --stdlib-deployment-targets => --cross-compile-stdlib-for-targets

Does this make sense?

So I take it we're not supporting cross-compiling for NxM hosts and targets?

That is, --cross-compile-stdlib-for-targets might be confused with some future option to specify which stdlibs are compiled for the cross-compiled hosts (e.g. I might be on OSX, building a linux-armv7 compiler capable of targeting itself and linux-x86_64). Currently, cross-compiled hosts can only target themselves. If you want anything else, you need to build it for the build machine and manually copy it over.

So I take it we're not supporting cross-compiling for NxM hosts and targets?

I can totally see adding support for building N cross-stdlibs in one run, but I would prefer to limit ourselves to cross-compiling just one compiler, which I think is the common case. If you think cross-compiling multiple compilers would be easy to support, I don't have objections in principle -- especially if there is a compelling usecase.

Yes, actually I was just thinking maybe we shouldn't concat these? How about this for an interface:

--cross-compile-host linux-armv7 \ --cross-compile-stdlib-targets linux-armv7 linux-x86_64 \ --cross-compile-host linux-x86_64 \ --cross-compile-stdlib-targets linux-x86_64 linux-armv7 android-armv7 \

If we get a list of lists, we can match up the host with the list of stdlib targets. That would be very cool, and opens up some possibilities:

How about we replace it with a verb? Merge --stdlib-deployment-targets with --cross-compile-stdlib-deployment-targets, in to --target, so you can go:

./build-script -R \ --target macos-x86_64 \ # First time, it's for the local host --cross-compile-for linux-armv7 \ --target linux-armv7 linux-x86_64 \ # now it's for args.cross_compile_for[idx-1] --cross-compile-for linux-x86_64 \ --target linux-x86_64 linux-armv7 android-armv7 \

This would work nicely with the explicit build/test lists, and any other options you might want to specify per-host.

We already support multiple hosts in one run. I don't think there's any difference between supporting one foreign host or supporting several. It's just another spin of the loop.

There is a small difference -- for example, we lipo together the foreign compilers for different Darwin slices. But we could just say that if we build any cross-compile any compilers for Darwin hosts, we will lipo them together.

That's what we do at the moment. Any lipo-able hosts will get merged together unless you tell it to skip that step (again, should really be opt-in instead of opt-out, but only the Darwin hosts are lipo-able anyway)

gribozavr · 2016-06-07T01:34:28Z

@rintaro I agree about the build-script becoming unusable, but I'm not sure if it is worth moving it...

I think build-script-impl has been not suitable for manual use for a long time already. The only time I would expect someone to call it directly is while debugging it. I don't think moving the file is worth the churn.

gribozavr · 2016-06-07T03:15:49Z

utils/build-script

+            # w.r.t. testing, but it was actually unused.
+
+            # As a special case, Darwin embedded non-simulator platforms have
+            # different behavior for the test condition.


I actually hope that this would become a general pattern for all non-native targets that require a separate device to run the tests.

Ok, will clarify that (and maybe model explicitly in the Platform object)

One thing that still confuses me: under what circumstances can we run all tests for those platforms? Is my reading of the build script correct in that that is not currently ever possible?

Good call. Currently, support for testing iOS devices is not open source. I was thinking about something like a flag which allows you to tell build-script which devices you have, for example, --on-device-tests=iphoneos-arm64 Linux-armv7, or maybe just --on-device-tests (implying "all"), and then build-script will run on-device tests unless explicitly asked to run the host-only tests. Does this make sense, or is it too much magic?

That makes sense to me... it matches the behavior of lit for example which just tries to run all of the tests which are supported by the configuration you are building, but leaves it up to user to verify that the all the tests you actually wanted to run are running.

I do think it would be good to have some notes printed about when classes of tests are going to be skipped because of missing support.

gribozavr · 2016-06-07T03:46:27Z

LGTM modulo comments!

karwa · 2016-06-08T08:43:04Z

utils/build-script

+
+        return {
+            "SWIFT_STDLIB_TARGETS": " ".join(swift_stdlib_targets),
+            "SWIFT_SDKS": " ".join(sorted(swift_sdks)),


It doesn't matter if swift_sdks are sorted, only that they are unique. We used sort -u as a cheap-and-cheerful way to do that in build-script-impl. If you try to configure iphoneos-armv7 and iphoneos-armv7s, they'll both get resolved to IOS. Then when you try to configure for IOS;IOS, CMake will complain about the duplicate.

Yeah, I understand. The reason why we sort here is so that the refactor validation logic inside build-script-impl works (because we compute the exact same result).

ddunbar · 2016-06-08T15:58:08Z

@gribozavr @rintaro @karwa pushed an updated version which I am personally happy with, and would like to land ASAP and address feedback in follow on work.

This PR depends on #2919, and I will rebase once that lands.

I have completely reworked the last commit in this sequence to factor out a helper object, add additional comments, and respond to feedback. I have tried to structure this in such a way that it will be useful when we are doing all of the build-script interactions ourselves (not via build-script-impl).

The other commits in the sequence are basically unchanged, so I recommend focusing only on that one for re-reviewing purposes.

karwa · 2016-06-08T16:50:35Z

utils/build-script

+        self.targets_to_test = []
+        self.benchmark_targets_to_build = []
+        self.benchmark_targets_to_run = []
+        self.sdks_to_configure = set()


Can we reorder these, so the configure targets come first?

I don't really like the names here. It starts with needed_deployment_targets - how/why is it needed? It should be called stdlib_targets_to_configure, but that's a bit strange because it would look like stdlib_targets_to_build, except _to_build doesn't actually contain stdlib-deployment-targets, it contains Swift build targets.

I would suggest the following:

needed_deployment_targets -> stdlib_targets_to_configure needed_deployment_targets_to_build -> stdlib_targets_to_build stdlib_targets_to_build -> swift_stdlib_build_targets targets_to_test -> swift_test_run_targets benchmark_targets_to_build -> swift_benchmark_build_targets benchmark_targets_to_run -> swift_benchmark_run_targets

These are all internal to the build script, so as long as we pass the correct ones along to -impl we should be okay.

What's your justification for including swift_ in some of the names? This is all related to Swift, so I didn't feel its inclusion was adding any clarity.

These are the build targets that we'll get by configuring swift. stdlib-deployment-targets aren't unique to the 'swift' product and might get used elsewhere - for example when compiling Foundation; it's a target library, so we should really loop over all those targets and build for them.

Ah! That makes sense -- swift there is a reference to swift the individual project among many. In that case I like your names, I will revise.

Pushed update with revised variable names and the comment above this.

karwa · 2016-06-08T17:11:37Z

utils/build-script

+            # For platforms which normally require a connected device to
+            # test, the default behavior is to run tests that only require
+            # the host (i.e., they do not attempt to execute).
+            if deployment_platform.is_darwin and \


This is the only place we use it, right? Seems easy enough to make generalised. I mean, if this were written in Swift, the compiler would go bananas at you checking for the existence of instance variables based solely on another instance variable. We should set a good example!

I'm not sure what you mean, are you talking about the is_darwin flag? Yes it is the only use currently, but I believe there are other places we will need it as the build-script-impl merging progresses.

We only have comparable checks in two places:

if a target has darwin symbols (i.e. whether or not we should dsymutil -- but we could generalise that as an executable_format and check for Mach-O instead; it's technically more accurate and that's useful information for cross-compilers anyway. I built llvm-dsymutil for linux, and if I had any Mach-Os on there I needed to strip, I'd be using it.)

Whether or not to lipo. Could also go under the Mach-O heading.

This, however, doesn't include examples like the one I pasted above, where we have code duplication that is doing the same thing for three different Darwin embedded platforms, but hadn't been factored into common code.

IMHO, factoring it into common code (even using is_darwin) is monotonic improvement that then makes it easier to generalize to a non-Darwin attribute. So I see this as a step in the direction you want...

That wasn't a great example; host tests are soon not going to be Darwin-exclusive. The fact that they are exclusive isn't actually inherent to Darwin; it's a limitation of what we have implemented right now. That's actually a great example of why not to hide everything behind is_darwin-esque magic IMO.

Maybe it's bikeshedding, but this is how it starts. Once that is_darwin flag is in there, it'll be used for everything.

ddunbar · 2016-06-08T21:10:06Z

Rebased after landing #2919

ddunbar · 2016-06-08T21:41:13Z

@swift-ci please Python lint

ddunbar · 2016-06-08T21:42:10Z

@swift-ci please smoke test

ddunbar · 2016-06-08T21:50:39Z

Linux test failure definitely my fault, investigating.

ddunbar · 2016-06-08T22:16:22Z

Turns out Linux bash didn't behave the same way in parsing environment variables, fixed to normalize out - -> _.

@swift-ci please smoke test

ddunbar · 2016-06-08T23:23:58Z

Linux failure is unrelated -- OS X failure is based on a misunderstanding of mine about the shell API, which I intend to adjust in #2959.

- This isn't yet used, but we need an easy way for `build-script` to pass host-specific variables down to `build-script-impl`. - I don't think it is worth complicating the main argument parsing logic with a syntax for passing associative arrays on the command line, so this just passes them via environment variables. - Part of SR-237.

- This is so that we can have a place to attach the additional metadata we need on platform or target specific behaviors.

- This adds several platform/target properties used to configure the default behavior for a build. - This also adds an API to find a target from a name.

- Introduces a new `HostSpecificConfiguration` which holds the computed information for each host we need to use. - This relies on the functionality to pass host-specific variables to the script via environment variables. - The computation itself has been cleaned up a tiny bit (mostly via using the factored out platform/target properties), but is otherwise a straight port of the logic from `build-script-impl`. - This commit does not yet remove that code from `build-script-impl`; instead it validates that the two implementations are computing the same result. This is useful right now for testing the validity of the port.

ddunbar · 2016-06-09T02:31:41Z

@swift-ci please smoke test

ddunbar · 2016-06-09T03:26:10Z

Linux failure is unrelated, and got a LGTM from @gribozavr

#2880 appends a `Target` instance to an array of strings when building Android. This causes a syntax error when building Android. Fix the error to repair the Android build.

rintaro mentioned this pull request Jun 4, 2016

[build-script] Introduce "concat" argument action #2884

Merged

1 task

karwa reviewed Jun 6, 2016
View reviewed changes

This was referenced Jun 6, 2016

[build-script] use explicit build/test list between build-script and build-script-impl #2804

Closed

Build script invocation helper #2919

Merged

gribozavr reviewed Jun 6, 2016
View reviewed changes

gribozavr reviewed Jun 7, 2016
View reviewed changes

karwa reviewed Jun 8, 2016
View reviewed changes

ddunbar force-pushed the build-script-compute-targets branch from 04a916f to 10f7c47 Compare June 8, 2016 15:55

karwa reviewed Jun 8, 2016
View reviewed changes

ddunbar changed the title ~~[WIP] [build-script] Compute targets in build-script~~ [build-script] Compute targets in build-script Jun 8, 2016

karwa reviewed Jun 8, 2016
View reviewed changes

ddunbar force-pushed the build-script-compute-targets branch 2 times, most recently from 827dfec to b112623 Compare June 8, 2016 21:08

ddunbar force-pushed the build-script-compute-targets branch 2 times, most recently from cc35486 to f8da62a Compare June 8, 2016 22:15

kylef mentioned this pull request Jun 8, 2016

install fails on FreeBSD kylef/swiftenv#36

Closed

ddunbar added 7 commits June 8, 2016 17:22

[build-script] Lift --cross-compile-hosts to build-script level.

c03943c

[build-script] Create explicit Platform and Target types.

bf75c8f

- This is so that we can have a place to attach the additional metadata we need on platform or target specific behaviors.

[build-script] Add a couple more options at the build-script layer.

6df6ae7

[swift_build_support.targets] Add some additional metadata.

f407cdf

- This adds several platform/target properties used to configure the default behavior for a build. - This also adds an API to find a target from a name.

[build-script] Several PEP8 fixes.

d515078

ddunbar force-pushed the build-script-compute-targets branch from f8da62a to d515078 Compare June 9, 2016 00:24

ddunbar merged commit 8a2fd16 into swiftlang:master Jun 9, 2016

gribozavr mentioned this pull request Jun 9, 2016

[Android] Add testing support #1714

Merged

2 tasks

This was referenced Jun 9, 2016

[CMake] Replace SWIFT_SDKS with SWIFT_CROSS_COMPILE_DEPLOYMENT_TARGETS #2835

Closed

[build-script] Add support for using isolated -impl actions. #2988

Merged

finagolfin mentioned this pull request Jul 31, 2020

[build] Remove unemployed 'skip-{build,test}-*' flags from build-script-impl #33154

Merged

[build-script] Compute targets in build-script #2880

[build-script] Compute targets in build-script #2880

Uh oh!

Conversation

ddunbar commented Jun 4, 2016

What's in this pull request?

Resolved bug number: (SR-)

Uh oh!

ddunbar commented Jun 4, 2016

Uh oh!

rintaro commented Jun 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddunbar commented Jun 6, 2016

Uh oh!

gribozavr Jun 6, 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!

Choose a reason for hiding this comment

Uh oh!

gribozavr Jun 6, 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!

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!

karwa Jun 8, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribozavr commented Jun 7, 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!

Choose a reason for hiding this comment

Uh oh!

gribozavr commented Jun 7, 2016

Uh oh!

karwa Jun 8, 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!

Choose a reason for hiding this comment

Uh oh!

ddunbar commented Jun 8, 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!

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!

[build-script] Compute targets in `build-script` #2880

[build-script] Compute targets in `build-script` #2880

gribozavr Jun 6, 2016 •

edited

Loading

gribozavr Jun 6, 2016 •

edited

Loading

karwa Jun 8, 2016 •

edited

Loading

karwa Jun 8, 2016 •

edited

Loading

karwa Jun 8, 2016 •

edited

Loading