[build-script-impl] Add support for performing isolated actions. by ddunbar · Pull Request #2844 · swiftlang/swift

ddunbar · 2016-06-02T15:47:22Z

What's in this pull request?

This adds support to build-script-impl for allowing individual "actions" to be performed. This allows for increased testing of the script, and for the controlling script (build-script) to manage the top-level loop for what actions need to be performed (by executing the -impl script multiple times).

This is intended to help with the refactoring work in SR-237, because it allows for the script to be in a more piecemeal fashion without requiring everything be migrated out to the Python code first.

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.

This adds a new argument --only-execute <name> which can be used by
build-script to invoke the build-script-impl to perform each different
action, while moving the high-level operation loop into the build-script
itself. This should make incremental refactoring of the individual actions
into build-script easier.
This is part of SR-237.

ddunbar · 2016-06-02T15:49:28Z

/cc @gribozavr @rintaro @modocache

By itself this doesn't make a lot of sense, but I think it is a decent approach to allow SR-237 to proceed more incrementally. I went with this approach because it avoids doing a massive refactor on the build-script-impl itself, which would introduce difficult conflicts on many other things.

I have more work in flight to allow build-script to take over control of the top-level loop...

ddunbar · 2016-06-02T15:49:35Z

@swift-ci please smoke test

rintaro · 2016-06-02T17:47:07Z

👍 Yeah.
Recently, almost everything in build-script-impl works with ${host}-${product} combination.
If we want to migrate by "product", we really want this feature.

Presumably, the very rough pseudo control flow would be:

for host in hosts:
    for product in products:
          product.do_build(host)

for host in hosts:
    for product in products:
          product.do_test(host)

for host in hosts:
    for product in products:
          product.do_install(host)

...

Based on that, some products may call build-script-impl, others may execute native implementation.

ddunbar · 2016-06-02T17:53:25Z

Yeah, exactly. Here is a hunk from build-script which uses this, at least just for testing purposes:

-    call_without_sleeping([build_script_impl] + build_script_impl_args)
+
+    # Perform the build process.
+    if True:
+        base_args = [build_script_impl] + build_script_impl_args
+        hosts = [args.host_target]
+        all_available_projects = [
+            'cmark', 'llvm', 'swift', 'lldb', 'llbuild',
+            'libdispatch', 'foundation', 'xctest', 'swiftpm']
+        for stage in ('build', 'test', 'install', 'package', 'lipo'):
+            # Stages where we enumerate each project.
+            if stage in ('build', 'test', 'install'):
+                for host in hosts:
+                    for project in all_available_projects:
+                        print("Performing {} for host {} and project {}".format(
+                            stage, host, project))
+                        call_without_sleeping(
+                            base_args + ["--only-execute", "{}-{}-{}".format(
+                                host, project, stage)])
+                continue
+            elif stage == 'package':
+                call_without_sleeping(
+                    base_args + ["--only-execute", "{}-{}".format(
+                        host, stage)])
+            else:
+                assert stage == 'lipo'
+                call_without_sleeping(
+                    base_args + ["--only-execute", "{}-{}".format(
+                        'merged-hosts', stage)])
+    else:
+        call_without_sleeping([build_script_impl] + build_script_impl_args)

It appears to work, but it's a significant performance regression in null build times (which is what lead to #2847) -- I am still working to see how much I can bring that down. Obviously we would want a cleaner actual implementation, and one thing we need to do first is over the options required to compute the full hosts list correctly.

gribozavr · 2016-06-03T00:30:34Z

utils/build-script-impl

Why not? This information is very useful in build logs.

Indeed, but if left here it would be printed multiple times. My plan was to make the build-script print this information when it switched over to --only-execute.

I'm afraid the build-script does not have this information yet, it does not know the specific targets that will be executed. (It is important to know whether we will be doing short vs. validation vs. long tests for example, or which platforms the tests will be run for.) Did you plan to move this computation over as well?

We have to eventually, so it seems like we might as well do that as part of switching the top-level loop over than come up with another mechanism to get this printed at the right time (I have a strong preference for not have unnecessary extra output).

gribozavr · 2016-06-03T01:03:18Z

This is a great approach to incrementally port the shell script! LGTM!

gribozavr · 2016-06-03T01:03:57Z

@swift-ci Please test

rintaro · 2016-06-03T01:06:12Z

@ddunbar Please consider to add --only-execute to unavailable options introduced in #2834

- This adds a new argument `--only-execute <name>` which can be used by `build-script` to invoke the `build-script-impl` to perform each different action, while moving the high-level operation loop into the `build-script` itself. This should make incremental refactoring of the individual actions into `build-script` easier. - This is part of SR-237.

ddunbar · 2016-06-03T15:44:00Z

Pushed updated version with review feedback.

@swift-ci please test and merge

gribozavr · 2016-06-03T16:01:59Z

utils/build-script-impl

+#
+# Check if the there are any actions to execute for this host and phase (i.e.,
+# "build", "test", or "install")
+function should_execute_host_actions_for_phase() {


Thank you for moving this into a function! The old logic seemed simple, but it turned out to be somehow confusing for me. With a function it is much more clear what is happening.

gribozavr reviewed Jun 3, 2016
View reviewed changes

ddunbar force-pushed the build-script-isolated-actions branch from fb50bba to 1a694dc Compare June 3, 2016 15:42

gribozavr reviewed Jun 3, 2016
View reviewed changes

gribozavr merged commit 2e7ae3d into swiftlang:master Jun 3, 2016

ddunbar deleted the build-script-isolated-actions branch June 6, 2016 16:42

ddunbar mentioned this pull request Jun 6, 2016

Build script invocation helper #2919

Merged

1 task

ddunbar mentioned this pull request Jun 2, 2016

[SR-237] Merge build-script-impl into build-script #42859

Open

Conversation

ddunbar commented Jun 2, 2016

What's in this pull request?

Uh oh!

ddunbar commented Jun 2, 2016

Uh oh!

ddunbar commented Jun 2, 2016

Uh oh!

rintaro commented Jun 2, 2016

Uh oh!

ddunbar commented Jun 2, 2016

Uh oh!

gribozavr Jun 3, 2016

Choose a reason for hiding this comment

Uh oh!

ddunbar Jun 3, 2016

Choose a reason for hiding this comment

Uh oh!

gribozavr Jun 3, 2016

Choose a reason for hiding this comment

Uh oh!

ddunbar Jun 3, 2016

Choose a reason for hiding this comment

Uh oh!

gribozavr commented Jun 3, 2016

Uh oh!

gribozavr commented Jun 3, 2016

Uh oh!

rintaro commented Jun 3, 2016

Uh oh!

ddunbar commented Jun 3, 2016

Uh oh!

gribozavr Jun 3, 2016

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants