Clean build-script by adding subcommands

[kimdv]

Based on this comment: #381 (comment)

Also: Since build-script-helper.py now does so many things, we should probably migrate it to using argparse subcommands. But that’s something for a completely standalone PR.

I tried to come with an example on how it could be structured. A command could look like: python3 ./build-script.py degyb --degyb-only --verbose
What do you think @ahoppen ?

Then we don't need to pass the --toolchain etc if it's not needed

[ahoppen]

Yes, that’s pretty much what I was thinking as well.

I think the possible subcommands could be

• build: Similar to ./build-script.py without any arguments
• test: Similar to ./build-script.py --test
• generate-xcodeproj: Similar to ./build-script.py --generate-xcodeproj
• degyb: Similar to ./build-script.py --degyb-only
• run-swiftsyntaxbuilder-generation: Similar to ./build-script.py --run-swiftsyntaxbuilder-generation

We should be able to get rid of the options I mentioned in the “Similar to ” commands once we move to sub parsers.

[kimdv]

Okay.
I've tried to add the commands.
There is some arguments that are duplicated.

Should we add them to the parser so all sub-commands can use them?

[ahoppen]

I just took a look at the actions that are currently supported by build-script.py and think it would make sense to provide the following sub-parser actions, with these options. Given that we now have gyb and SwiftSyntaxBuilderGeneration, I think it also makes sense to combine the two generation steps into one sub-command and decouple code-generation from building.

Actions:

generate-xcodeproj  Generate an Xcode project for SwiftSyntax.
  -v, --verbose         Enable verbose logging.
  --xcconfig-path XCCONFIG_PATH

generate-source-code    Generate source code using gyb and SwiftSyntaxBuilderGeneration
  -v, --verbose         Enable verbose logging.
  --add-source-locations
  --gyb-exec GYB_EXEC   Path to the gyb tool
  -v, --verbose         Enable verbose logging.
  -r, --release         Build in release mode.
  --build-dir BUILD_DIR
  --disable-sandbox     Disable sandboxes when building with SwiftPM
  --multiroot-data-file MULTIROOT_DATA_FILE
  --toolchain TOOLCHAIN
  --verify              Instead of writing the newly generated files to the source directory, assert that the generated files match the ones currently in the source directory

build                   Build all SwiftSyntax modules (does not re-generate source code - this is a change from the current behavior)
  -v, --verbose         Enable verbose logging.
  -r, --release         Build in release mode.
  --build-dir BUILD_DIR
  --disable-sandbox     Disable sandboxes when building with SwiftPM
  --multiroot-data-file MULTIROOT_DATA_FILE
  --toolchain TOOLCHAIN

test                    Run tests (does not re-generate source code - this is a change from the current behavior)
  -v, --verbose         Enable verbose logging.
  -r, --release         Build in release mode.
  --build-dir BUILD_DIR
  --disable-sandbox     Disable sandboxes when building with SwiftPM
  --multiroot-data-file MULTIROOT_DATA_FILE
  --toolchain TOOLCHAIN
  --skip-lit-tests      Don't run lit-based tests
  --filecheck-exec FILECHECK_EXEC

Regarding the common args, I think it makes sense to have two functions that add these common args (one for --verbose and one for all the build flags), similar to https://github.com/apple/sourcekit-lsp/blob/472a06c88af97135b6faa450c5854e8f1a0ab8de/Utilities/build-script-helper.py#L169-L181

[kimdv]

@ahoppen I added your suggestions on groups.
Made it a bit more clean.

[ahoppen]

Thanks. I think this looks a lot cleaner than our old build-script and I like the separation of the subcommands in different functions 👍

[kimdv]

@ahoppen I have fixed all the comments.
How do we setup the CI?

I'm not sure it knows about the new sub-commands?

[ahoppen]

You would need to update the way that the the build-script in the main Swift repo invokes SwiftSyntax’s build-script.py here.

[kimdv]

swiftlang/swift#59651

@swift-ci please test

[kimdv]

swiftlang/swift#59651

@swift-ci please test

[kimdv]

Failed because it didn't recognise --verbose.
It need to come before the build argument, as it is in the root parser.

This syntax seems a bit strange @ahoppen ?

[kimdv]

swiftlang/swift#59651

@swift-ci please test

[ahoppen]

Failed because it didn't recognise --verbose. It need to come before the build argument, as it is in the root parser.

This syntax seems a bit strange @ahoppen ?

Ah, I remember. This is because --verbose is an argument to the top level parser. We probably want to add --verbose as an option to every sub parser.

[kimdv]

swiftlang/swift#59651

@swift-ci please test

[ahoppen]

One minor thing: CONTRIBUTING.md needs to be updated to include the build subcommand.

Afterwards, let’s 🚢 it

[kimdv]

swiftlang/swift#59651

@swift-ci please test

[kimdv]

@ahoppen should swiftlang/swift#59651 also be rebuild?

[ahoppen]

@ahoppen should apple/swift#59651 also be rebuild?

Yes. I don’t expect it to find any different issues but it’s better to be safe than sorry.

[ahoppen]

No, I mixed up the PRs. I thought this was about making matching Swift files for the Python definitions. No need to re-build swift if you just changed CONTRIBUTING.md