[build-script] Introduce ProductBuilder. Transform Ninja to use it. #23822

drodriguez · 2019-04-05T19:42:40Z

ProductBuilder allows us to tackle the different way than the different
products need to be build. The builders follow a very simple interface,
but inside them the details are hidden.

Previously the Ninja product was both a Product and also the process to build Ninja. The
methods that did the build have moved into ProductBuilder to match the
future ProductBuilders.

Product and ProductBuilder are very close, but the current design of Product is not adequate to compile for different hosts and lacks of some access to other parts of the script. Instead of modifying Product, I decided to keep separate from it to allow the rest of the script to keep working the same (it will also allow to keep the previous way of doing things and the new way of doing things in parallel, which might made the migration less scary).

Pieces that ProductBuilder requires, but weren’t available in Product are the target host and the workspace information. The first is needed to generate the right parameters to invoke CMake and other build scripts, while the second is necessary to find out about other product locations (Swift product references LLVM, LLDB references both Swift and LLVM), as well as allowing access to the build results for the current target, and also the tools host target.

Later commits will show how ProductBuilder is leveraged to invoke different building scripts and how flexible is the setup.

This was part of #23257. I’m trying to split the PR in smaller ones to make the reviews easier. Other PRs in this group are #23303, #23803, and #23810 .

/cc @compnerd

compnerd · 2019-04-06T00:36:54Z

utils/build-script

-            source_dir=self.workspace.source_dir("ninja"),
-            build_dir=self.workspace.build_dir("build", "ninja"))
+            workspace=self.workspace,
+            host=StdlibDeploymentTarget.get_target_for_name(


None of this is your doing, I'm wondering if you know the history. Why is this in StdlibDeploymentTarget? Why is this being built for the host_target and not build_target? This is the tool being built for the build right?

I think StdlibDeploymentTarget is a little misnamed at this point, because it is used for more things that just the stdlib. If you think about it as CompilationTarget or DeploymentTarget is easier to understand.

Ninja is build only for the machine building the rest of the pieces (it is build if it cannot be found in the toolchain/path), so it is fine to “hardcode” here to host_target (which is the current machine).

If you look at the implementation of make_builder, it actually ignores the target. The previous implementation didn’t care about the target (so it builds for the local target).

PD: there's a big mess of names in the build script. Sometimes host refers to the device that run the code (arguments --host-test and --skip-test-ios-host which control to run or not tests on the external devices), but sometimes refers to the machine doing the build (like this host_target and arguments like --build-runtime-with-host-compiler and --host-cc).

Right, I figured it was a poorly named variable at this point. I was thinking of it as TargetInformation. I was wondering if you knew why this had come to be poorly named at this point.

As to the Ninja bits, right - its again a name thing. target is the target where generated code will be run, host is where the generated code generator will run, and build is the machine where the build is occurring. These are well documented names, so it was a question of does the current machinery not name things accordingly?

I think that making it deal with cross-compilation is great thing, but way outside the scope of this current change .

About the renaming, from comments and clues in the code, I think there was a previous try to make everything more standard, but it was necessary to keep the argument names the same for compatibility, which probably makes things a little bit more complicated. I can see if something can be done as a cleanup when all these pieces fall into place (I prefer to have the “wrong” names, since they match better what the build-script-impl have).

compnerd · 2019-04-06T00:39:26Z

@swift-ci please test

swift-ci · 2019-04-06T01:38:04Z

Build failed
Swift Test OS X Platform
Git Sha - 29159b9cc513a0d46ea3a5aeba7eabde4d691880

swift-ci · 2019-04-06T02:33:20Z

Build failed
Swift Test Linux Platform
Git Sha - 29159b9cc513a0d46ea3a5aeba7eabde4d691880

utils/swift_build_support/swift_build_support/products/product_builder.py

utils/swift_build_support/tests/products/test_ninja.py

utils/swift_build_support/swift_build_support/products/ninja.py

drodriguez

Fixed a couple of points pointed out by Ross, and also removed the linter problem pointed out by CI.

utils/swift_build_support/swift_build_support/products/ninja.py

utils/swift_build_support/swift_build_support/products/product_builder.py

compnerd · 2019-04-07T17:51:31Z

@swift-ci please test

swift-ci · 2019-04-07T17:53:32Z

Build failed
Swift Test Linux Platform
Git Sha - 29159b9cc513a0d46ea3a5aeba7eabde4d691880

As proposed in swiftlang#23822, the do_* methods should only be using the * parts. Because all building should have the same names, this commit removes the prefix from all the method instances.

drodriguez · 2019-04-09T00:29:34Z

Removed the do_ from this PR, as I did in #23871. No other changes at the moment.

PD: the test were green before I pushed again.

Rostepher · 2019-04-09T01:34:49Z

@swift-ci please smoke test

drodriguez · 2019-04-09T18:46:13Z

Moved the initializer into the formal requirements of the ProductBuilder. It looks a little bit out of place for single product builders, but I hope it will not create problems later.

compnerd · 2019-04-12T16:23:54Z

@swift-ci please smoke test

drodriguez · 2019-04-12T18:19:45Z

The failures seems to be related to #23980, which disables those tests completely.

drodriguez · 2019-04-29T21:17:03Z

@swift-ci please test

swift-ci · 2019-04-29T23:07:04Z

Build failed
Swift Test Linux Platform
Git Sha - 1ee8a0482daeef8394e41e72f30bdc92b53addb5

drodriguez · 2019-04-29T23:20:47Z

The Linux failure seems to be fixed in #24372.

@swift-ci please test linux platform

drodriguez · 2019-04-30T18:15:31Z

@Rostepher, @compnerd: I would like to restart my efforts in these PRs, if everyone is onboard. Without this PR and some of the others I am a little bit blocked about the next steps. If there's further feedback or requests, it would be my pleasure to make the changes.

(PD: other people can also provide feedback)

drodriguez · 2019-05-08T22:55:41Z

First and foremost, thank for all the work done in this PRs and the rest of the project.

We know that we are all very busy, but it would be great to know if the radio silence is because we are busy or something related to the PRs contents. If the PRs conflict with internal systems and modifications, I would not mind modifying this to adapt better (or help modifying the internal systems if that's possible), but without any clue of what can be modified, I cannot do that much.

Thanks, and sorry for insisting again.

utils/swift_build_support/swift_build_support/products/product_builder.py

utils/swift_build_support/tests/products/test_ninja.py

drodriguez

Thanks for the feedback. I will change a couple of things and resubmit.

utils/swift_build_support/swift_build_support/products/product_builder.py

utils/swift_build_support/tests/products/test_ninja.py

ProductBuilder allows us to tackle the different way than the different products need to be build. The builders follow a very simple interface, but inside them the details are hidden. Previously the Ninja product was both a Product and ProductBuilder. The methods that did the build have moved into ProductBuilder to match the future ProductBuilders.

drodriguez · 2019-05-13T19:56:50Z

@swift-ci please test

Rostepher

I think I'm satisfied with these changes now.

drodriguez requested review from rintaro, gottesmm and Rostepher April 5, 2019 19:42

compnerd reviewed Apr 6, 2019

View reviewed changes

Rostepher reviewed Apr 6, 2019

View reviewed changes

utils/swift_build_support/swift_build_support/products/product_builder.py Outdated Show resolved Hide resolved

Rostepher reviewed Apr 6, 2019

View reviewed changes

utils/swift_build_support/tests/products/test_ninja.py Outdated Show resolved Hide resolved

Rostepher reviewed Apr 6, 2019

View reviewed changes

utils/swift_build_support/swift_build_support/products/ninja.py Outdated Show resolved Hide resolved

Rostepher reviewed Apr 6, 2019

View reviewed changes

utils/swift_build_support/swift_build_support/products/ninja.py Outdated Show resolved Hide resolved

drodriguez mentioned this pull request Apr 6, 2019

[utils] Remove unused method. #23840

Merged

drodriguez force-pushed the build-script-product-builder-ninja branch from 29159b9 to a353096 Compare April 6, 2019 20:50

drodriguez commented Apr 6, 2019

View reviewed changes

utils/swift_build_support/swift_build_support/products/ninja.py Outdated Show resolved Hide resolved

utils/swift_build_support/swift_build_support/products/product_builder.py Outdated Show resolved Hide resolved

drodriguez mentioned this pull request Apr 8, 2019

[build-script] Move some calculations into independent methods. #23865

Merged

drodriguez mentioned this pull request Apr 8, 2019

[build-script] Rename all do_* methods to simply *. #23871

Merged

drodriguez force-pushed the build-script-product-builder-ninja branch from a353096 to e5eac33 Compare April 9, 2019 00:28

drodriguez force-pushed the build-script-product-builder-ninja branch 2 times, most recently from fd406cd to 9ac1b7a Compare April 9, 2019 18:44

This was referenced Apr 9, 2019

[build-script] Move parts of BuildScriptInvocation into HostSpecificConfiguration #23915

Merged

[build-script] Split execute_one_impl_action. #23917

Merged

[build-script] Rename is_build_script_impl_product to needs_toolchain. #23918

Closed

This was referenced Apr 12, 2019

[build-script] Extract most parts related to build-script-impl #23982

Closed

[build-script] Move HostSpecificConfiguration out of main script. #23988

Merged

drodriguez force-pushed the build-script-product-builder-ninja branch from 9ac1b7a to 1ee8a04 Compare April 29, 2019 18:58

Rostepher reviewed May 11, 2019

View reviewed changes

utils/swift_build_support/swift_build_support/products/product_builder.py Outdated Show resolved Hide resolved

Rostepher reviewed May 11, 2019

View reviewed changes

utils/swift_build_support/tests/products/test_ninja.py Outdated Show resolved Hide resolved

Rostepher reviewed May 11, 2019

View reviewed changes

utils/swift_build_support/tests/products/test_ninja.py Outdated Show resolved Hide resolved

drodriguez commented May 13, 2019

View reviewed changes

utils/swift_build_support/swift_build_support/products/product_builder.py Outdated Show resolved Hide resolved

utils/swift_build_support/tests/products/test_ninja.py Outdated Show resolved Hide resolved

utils/swift_build_support/tests/products/test_ninja.py Outdated Show resolved Hide resolved

drodriguez force-pushed the build-script-product-builder-ninja branch from 1ee8a04 to 5c0e743 Compare May 13, 2019 19:53

Rostepher self-requested a review May 14, 2019 00:02

Rostepher approved these changes May 14, 2019

View reviewed changes

drodriguez merged commit 0de91dc into swiftlang:master May 14, 2019

drodriguez deleted the build-script-product-builder-ninja branch May 14, 2019 18:56

[build-script] Introduce ProductBuilder. Transform Ninja to use it. #23822

[build-script] Introduce ProductBuilder. Transform Ninja to use it. #23822

Uh oh!

Conversation

drodriguez commented Apr 5, 2019

Uh oh!

compnerd Apr 6, 2019

Choose a reason for hiding this comment

Uh oh!

drodriguez Apr 6, 2019

Choose a reason for hiding this comment

Uh oh!

compnerd Apr 7, 2019

Choose a reason for hiding this comment

Uh oh!

drodriguez Apr 8, 2019

Choose a reason for hiding this comment

Uh oh!

compnerd commented Apr 6, 2019

Uh oh!

swift-ci commented Apr 6, 2019

Uh oh!

swift-ci commented Apr 6, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

drodriguez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

compnerd commented Apr 7, 2019

Uh oh!

swift-ci commented Apr 7, 2019

Uh oh!

drodriguez commented Apr 9, 2019

Uh oh!

Rostepher commented Apr 9, 2019

Uh oh!

drodriguez commented Apr 9, 2019

Uh oh!

compnerd commented Apr 12, 2019

Uh oh!

drodriguez commented Apr 12, 2019

Uh oh!

drodriguez commented Apr 29, 2019

Uh oh!

swift-ci commented Apr 29, 2019

Uh oh!

drodriguez commented Apr 29, 2019

Uh oh!

drodriguez commented Apr 30, 2019

Uh oh!

drodriguez commented May 8, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

drodriguez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

drodriguez commented May 13, 2019

Uh oh!

Rostepher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!