[build] Fix --skip-build-llvm so that its minimal targets, like tblgen, are still built

[finagolfin]

There have been some regressions this year that broke the way the three base products- cmark, llvm, and swift- are built if their skip-build-* flags are applied. For example, building the standalone stdlib no longer works after pulls #29373, #29500, and #32256. I just noticed this when trying to run this similar build-script command to build the Swift stdlib alone on linux with the latest July 12 source snapshot from trunk and matching prebuilt official compiler (I use an extended version of this command when cross-compiling the stdlib for Android):

./swift/utils/build-script -R --no-assertions --build-runtime-with-host-compiler --build-swift-tools=0 --native-swift-tools-path=/home/butta/712-swift/usr/bin/ --native-llvm-tools-path=/home/butta/712-swift/usr/bin/ --native-clang-tools-path=/home/butta/712-swift/usr/bin/ --skip-build-cmark --skip-build-llvm -j5

The three base products were always handled differently in the build scripts until this year, because they would usually be built and relied on each other. For example, LLVM has to be configured for the Swift product to be configured.

The way the build-script worked till January was that --skip-build-* meant the product was neither configured or built, with the exception of the base products: CMark, LLVM, and Swift. For those three, --skip-build-swift or the equivalent would mean that that product was configured, but not built (with one exception, a few LLVM targets like tblgen would always be built even with --skip-build-llvm or --skip-build back then).

Since January, the first two pulls above changed the base products to match all other products, ie --skip-build-* always means don't configure and don't build (with a strange exception of configuring and building the minimal LLVM targets only if the --skip-build flag was passed, likely a mistake). The way this worked was that build-script-impl was always run for each of the base products and the flags in that bash script would then disable both configuring and building the unwanted base products.

Finally, #32256 last month added another layer and disabled even running build-script-impl if a skip-build-* flag was passed for a base product. That made the logic from the first two pulls disabling those same base products in build-script-impl redundant.

This pull partially reverts those three pulls for the --skip-build-llvm flag alone, with the only difference fromchanging it back to the way it worked last year that it will now configure the minimal LLVM targets but not build them. This gets the above pasted command to build the standalone stdlib working again.

I'm not sure if more should be done, so I'm only offering this as a potential solution to spur discussion. @atrick and @gottesmm, let me know what you think these --skip-build* flags for the three base products should do, given the historical behavior and dependencies laid out above.

[atrick]

It looks like your PR will work. As long as --skip-build continues to configure cmark, llvm, and swift, but doesn't build anything other than LLVM tablegen stuff, then I'll be happy. To me, the build-script is only a configuration script, and my workflow cannot work without the ability to reconfigure without rebuilding LLVM.

Just to be clear, the only purpose of the --skip-build flag I'm aware of is to configure the requested products without building them. generate build directory only without building This was always the intention of the flag, and the way I've been using it literally since the moment build-script was written.

The terribly-named -skip-build-<product> flags actually control configuration:

# The --skip-build parameter, with no product name, does not affect the
# configuration (CMake parameters). You can turn this option on and
# off in different invocations of the script for the same build
# directory without affecting configutation.
#
# skip-build-* and build-* parameters affect the CMake configuration
# (enable/disable those components).

This PR completely broke the --skip-build option:
#29373

My workflow was blocked until I fixed that functionality:
#29500

AFAICT, your PR will keep the skip-build functionality working.

[finagolfin]

@atrick, yes, --skip-build alone will keep working as it was before Alex changed it in January after this pull, with only one difference for LLVM: before it would configure and build the minimal LLVM tblgen targets, ie it would still build something, whereas now it will configure all the LLVM targets but not build them.

My problem is not --skip-build though, it's that your pull changed the way --skip-build-llvm worked, and last month's pull made it even more that way. If you have any input on that changed behavior for the specific base product flags and fixing it, let me know.

[atrick]

before it would configure and build the minimal LLVM tblgen targets, ie it would still build something

Hmm. Ok, I thought the tablegen tool itself would need to be built in order to configure Swift. At any rate, as long as I can reconfigure Swift without building all of LLVM then mission accomplished.

I have no need to use --skip-build-llvm other than to build a standalone stdlib. Since that's your goal too, I think we're aligned. My change tried to preserve the behavior that the previous PR #29373 introduced, where --skip-build-llvm completely skipped LLVM configuration (IIRC). This seemed consistent with the other --skip-build-product flags, but the end result of being able to build standalone stdlib is what really matters. Could you make it clear in the script comments why --skip-build-llvm needs to be handled differently?

[finagolfin]

Hmm. Ok, I thought the tablegen tool itself would need to be built in order to configure Swift

You're right. I was only checking for the standalone Swift stdlib, where it isn't needed. Configuring the Swift compiler build normally with --skip-build-llvm looks for tablegen, so I've now changed it back to the way it was last year: the minimal LLVM targets are always built regardless of --skip-build or --skip-build-llvm.

My change tried to preserve the behavior that the previous PR #29373 introduced, where --skip-build-llvm completely skipped LLVM configuration

No, that was done in your pull, including removing Alex's comment explaining why it couldn't be done. 😉

Could you make it clear in the script comments why --skip-build-llvm needs to be handled differently?

I've added comments everywhere now, including resurrecting most of Alex's original comment.

I think this is ready to go, once it passes the CI. After it gets in, I will submit a pull with the build-script-impl changes alone for the release/5.3 branch, so anyone using --skip-build-llvm with the 5.3 releases won't hit this either.

[atrick]

My change tried to preserve the behavior that the previous PR #29373 introduced, where --skip-build-llvm completely skipped LLVM configuration

No, that was done in your pull, including removing Alex's comment explaining why it couldn't be done. 😉

I thought that code was redundant with the code I added here:
https://github.com/apple/swift/pull/29500/files#diff-65b44eb6cb88af2161d8e1176231aad0R2208-R2223

At any rate, this PR now looks like it's just making -skip-build-llvm behave the same as --skip-build with respect to LLVM. I actually prefer that behavior. I thought the intention behind PR #29373 was to add "don't configure and don't build" options for each product, so I tried to preserve that intent. I had no idea there was also a supported build-swift-tools option, which doesn't appear in the top level script, and presumably needs swift to be configured even though it isn't built? (I'm just guessing at what actually broke for you)

[finagolfin]

At any rate, this PR now looks like it's just making -skip-build-llvm behave the same as --skip-build with respect to LLVM.

Yep, that's all it does now, reverting back to the way it was last year.

I had no idea there was also a supported build-swift-tools option, which doesn't appear in the top level script, and presumably needs swift to be configured even though it isn't built?

Yeah, it's just passed to SWIFT_INCLUDE_TOOLS, which disables building the compiler and other host tools but leaves alone configuring and building the target stdlib.

(I'm just guessing at what actually broke for you)

The Swift CMake config still needs some generated LLVM CMake config files when building the Swift stdlib standalone. That's why the first iteration of this pull, that simply configured the LLVM targets but didn't build them, was enough for me to build the Swift stdlib alone, but I changed it to build the minimal LLVM targets now, since that's needed when configuring the Swift compiler build.

[atrick]

Now it all finally makes sense. Thanks. These build-script flags are a mess. If it gets properly cleaned up we should just make it clear that configuring swift depends on configuring llvm, configuring stdlib depends on configuring llvm (or hopefully we can completely break that dependence), and configuring llvm requires building some minimal tools. It's really not that hard of a problem.

[finagolfin]

@atrick, if this is okay now, can you trigger the CI and merge, as I can't do either.

[atrick]

@swift-ci smoke test and merge
...oops this was never fully CI tested

[atrick]

@swift-ci test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 0034aa948ceb2e7d38c73d7406df776413365747

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 38fa32e0119b86eb44fa31de73cb4a5931dce584

[finagolfin]

The macOS CI failure for lldb looks unrelated. Anyway, please hold off on the CI for now, as I see a test from #32256 that will need to be modified.

[finagolfin]

Modified the relevant test but it isn't run on the CI right now, so this pull will have to wait till #32954 enabling that test is merged first.

[finagolfin]

Alright, @atrick, ready to run the CI now, got the related tests enabled again.

[finagolfin]

Or @davezarzycki, since you're up, could you just kick off the CI for this?

[davezarzycki]

@swift-ci please test

[finagolfin]

Thanks. This test was disabled on the CI before, should run now.

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 38fa32e0119b86eb44fa31de73cb4a5931dce584

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 38fa32e0119b86eb44fa31de73cb4a5931dce584

[finagolfin]

Linux passes, macOS CI appears broken right now.

@hlopko, since you're the only person to submit a pull to this repo this morning, maybe you know who to report this macOS CI breakage to.

[atrick]

That known test failure was the only failure. So it's safe to smoke test now.

[atrick]

@swift-ci smoke test

[finagolfin]

Alright, finally passed the CI and I confirmed that the modified test was run: ready for merge.

[atrick]

Thanks!

[finagolfin]

Thanks for reviewing @atrick, I'll submit a backport pull for the 5.3 branch next. Also, if you have any input on my related pull #32922, let me know.

[gottesmm]

@buttaface This is a good fix. Thanks! I have been meaning to find time to fix this for some time. It also broke a workflow that I use for refactoring quickly (--skip-build-llvm with the Xcode code generator... as you said... you need these binaries if you want to build swift since they are binaries we rely on existing).

Thanks for updating the test as well! It is so important that we have these build-system unit tests to freeze the behavior of build-script to stop things like this from sneaking into the tree!

[finagolfin]

Sure, I use it too, so just scratching my own itch. 😄