Build script builders #23257

drodriguez · 2019-03-13T02:46:04Z

Applies to SR-237

This is a transition version of #23038. I tried to reduce the code to only the essential to show the idea behind the “product builders”. This version hides the invocations to build-script-impl in a builder, and that way, one can replace each product builder one by one, which should make reviews easier.

This is probably easier to look at commit by commit, because the code movements are clearer. There are three commits that mostly move code from one file to another:

Moving a small utility method into shell.
Moving HostSpecificConfiguration from the main script into its own file, and also bring parts of BuildScriptInvocation with it.
Moving large parts of BuildScriptInvocation that dealt with transforming the Python arguments into build-script-impl arguments into BuildScriptImplBuilder.
The rest of the movements are mostly inside the files themselves.

The final intention of this changes is replacing build-script-impl to allow the build script to not depend on Bash, and be compatible with Windows.

/cc @Rostepher, @benlangmuir, @compnerd

benlangmuir

I'm really happy about moving the responsibility for invoking build-script-impl into the Product. I think this is a natural evolution toward killing build-script-impl ❤️

I skimmed over the patch, and it seems reasonable to me, but I haven't dug into the details.

One thing that I'm curious about: is it possible to verify that this doesn't change the invocations to build-script-impl, for example using --dry-run and comparing with the previous behaviour? If we could do that for all of our presets, that would be great.

utils/swift_build_support/swift_build_support/products/product_builder.py

benlangmuir · 2019-03-15T15:37:36Z

utils/swift_build_support/swift_build_support/products/product_builder.py

+#
+# This source file is part of the Swift.org open source project
+#
+# Copyright (c) 2014 - 2018 Apple Inc. and the Swift project authors


I don’t know why I remember changing that. It must have been in a parallel dimension. I will fix it everywhere.

drodriguez

I will see if there's a way to run with dry-run and compare before and after these changes. I'm afraid that I will have modified the order of some arguments and maybe some values, which might make comparing a little harder.

utils/swift_build_support/swift_build_support/products/product_builder.py

drodriguez · 2019-03-15T23:14:59Z

utils/swift_build_support/swift_build_support/products/product_builder.py

+#
+# This source file is part of the Swift.org open source project
+#
+# Copyright (c) 2014 - 2018 Apple Inc. and the Swift project authors


I don’t know why I remember changing that. It must have been in a parallel dimension. I will fix it everywhere.

drodriguez · 2019-03-18T22:57:26Z

Good idea running the dry-run. I found two problems: the tests for the products built without build-script-impl weren't being run, and the build directory for Ninja was named differently.

Additionally, in the new version --llvm-cmake-options and --swift-cmake-options are not passed to all the invocations of build-script-impl, only to those that make sense (CMark do not receive anything, LLVM only receives LLVM, Swift only receives Swift, etc…). I don’t think this will be a problem. From what I see in build-script-impl, each product receives their --foo-cmake-options and cannot access the other products options.

To test I wrote a couple of small scripts (https://gist.github.com/drodriguez/8bc2a0c228b7afad5cec97448dd04bec): one that listed every preset in the preset file and run it with dry-run (skipping mixins and buildbot_iphoneos_arm64_crosscompiler which fails in Linux because it uses xcrun), and another that compared two runs of the script (one in the workdir without these changes, and one in the workdir with them). To compare one has to remove the appearances of --(llvm|swift)-cmake-options, but if I force the new script to generate those, the results from both pre and post are exactly the same.

benlangmuir · 2019-03-19T21:24:35Z

Nice! Thanks for testing it out. It gives me a lot of confidence in the change to know it gives the same commands.

Additionally, in the new version --llvm-cmake-options and --swift-cmake-options are not passed to all the invocations of build-script-impl, only to those that make sense (CMark do not receive anything, LLVM only receives LLVM, Swift only receives Swift, etc…). I don’t think this will be a problem. From what I see in build-script-impl, each product receives their --foo-cmake-options and cannot access the other products options.

I think right now "libcxx" is using llvm_cmake_options; could you take a look? Otherwise I think you're right and only passing the relevant options is a nice simplification.

drodriguez · 2019-03-19T23:09:14Z

It is quite complicated to trace build-script-impl. Indeed libcxx makes use of llvm_cmake_options, however, that variable gets build independently from --llvm-cmake-options.

--llvm-cmake-options is picked up in build-script-impl L2122-2125 and appended to cmake_options, however, while building libcxx, the variable product will be libcxx, so it will try to pick up --libcxx-cmake-options not --llvm-cmake-options.

llvm_cmake_options is built from other sources: it starts empty at L421, and gets modified in different lines of that function, but never trying to pick up --llvm-cmake-options. It is also modified in L2030, but again, it is not affected by that argument from the command line.

benlangmuir · 2019-03-20T18:44:13Z

Ah, thanks for clearing up my confusion.

drodriguez · 2019-04-03T02:05:01Z

@Rostepher: I’m sorry to bother you again. It would be great to have this PR moving again, since it is interesting for the Windows work, and it will allow me to also try to compile more pieces for the Android targets when all the pieces are there. If you think someone else should review this, that's also fine. For the time being, it would be nice to have the test passing (I have commit access, but I still don’t have swift-ci access for some reason).

If anyone want to see how this is used later, I started uploading more pieces into my repo. The LLVM builder needs to be the first one, since other builders depend on it. It is also one of the most complicated ones. The code is at https://github.com/drodriguez/swift/tree/llvm-builder. Another example is the CMark builder, which builds on top of the LLVM one. The code is at https://github.com/drodriguez/swift/tree/cmark-builder.

Thanks.

compnerd · 2019-04-03T21:31:48Z

@swift-ci please test

swift-ci · 2019-04-03T22:37:17Z

Build failed
Swift Test Linux Platform
Git Sha - c6908eec9ebdc97e2e11f6db6c982a087b16cc84

swift-ci · 2019-04-03T22:40:55Z

Build failed
Swift Test OS X Platform
Git Sha - c6908eec9ebdc97e2e11f6db6c982a087b16cc84

This will allow using HostSpecificConfiguration from other parts that are not the main script in the future. Additionally, a lot of code that was part of BuildScriptInvocation has been moved into HostSpeficiaConfiguration because nobody else needed it, and HostSpecificConfiguration was highly coupled with the Invocation object (through one of its arguments). This should make the Configuration object a better citizen in other scenarios.

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.

BuildScriptImplBuilder is a ProductBuilder that invokes build-script-impl to work. The implementation is moved code from the main script BuildScriptInvocation type into the new builder. This will allow us to replace each of the product builders one by one and check them before commiting to the new system. There's a little reorganization in the execute method of BuildScriptInvocation to make it less tied to the whims of build-script-impl.

…Builders. Create a new helper for IndexStoreDB and SourcekitLSP called BuildScriptHelperBuilder. Move most of the code that existed in a function in indexstoredb.py into that helper object. Create small builders for IndexStoreDB and SourcekitLSP which use this helper object as their base. Modify the main build-script to invoke the new builder instead of the specific do_build/do_test. This way, all products are using the same system to be build, but some use build-script-impl, and others use build-script-helper.py.

…e it.

drodriguez · 2019-04-04T02:26:36Z

Thanks @compnerd for starting the tests.

I hope I have fixed the problem with the last push.

I will also start creating smaller PR with each of the commits to try to make these reviews less difficult. I hope with smaller commits the code movements will be clearer.

drodriguez · 2020-01-10T16:50:41Z

Abandoning these efforts. It might still be useful for better Windows builds for most of the community, but I will not have time to keep improving this and it doesn't seem to be a lot of interest in improving the build system in this direction.

benlangmuir reviewed Mar 15, 2019

View reviewed changes

Rostepher self-requested a review March 15, 2019 19:08

drodriguez commented Mar 15, 2019

View reviewed changes

drodriguez force-pushed the build-script-builders branch 2 times, most recently from ea717ae to 11a8a9b Compare March 18, 2019 22:04

drodriguez force-pushed the build-script-builders branch from 11a8a9b to c6908ee Compare April 2, 2019 23:30

drodriguez added 6 commits April 3, 2019 19:18

[build-script] Move call_without_sleeping into the shell module

877c251

fixup! [build-script] Introduce ProductBuilder. Transform Ninja to us…

8afd7f4

…e it.

drodriguez force-pushed the build-script-builders branch from c6908ee to 8afd7f4 Compare April 4, 2019 02:19

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 closed this Jan 10, 2020

Build script builders #23257

Build script builders #23257

Uh oh!

Conversation

drodriguez commented Mar 13, 2019

Uh oh!

benlangmuir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

benlangmuir Mar 15, 2019

Choose a reason for hiding this comment

Uh oh!

drodriguez Mar 15, 2019

Choose a reason for hiding this comment

Uh oh!

drodriguez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

drodriguez Mar 15, 2019

Choose a reason for hiding this comment

Uh oh!

drodriguez commented Mar 18, 2019

Uh oh!

benlangmuir commented Mar 19, 2019

Uh oh!

drodriguez commented Mar 19, 2019

Uh oh!

benlangmuir commented Mar 20, 2019

Uh oh!

drodriguez commented Apr 3, 2019

Uh oh!

compnerd commented Apr 3, 2019

Uh oh!

swift-ci commented Apr 3, 2019

Uh oh!

swift-ci commented Apr 3, 2019

Uh oh!

drodriguez commented Apr 4, 2019

Uh oh!

drodriguez commented Jan 10, 2020

Uh oh!

Uh oh!