Disable benchmark builds by default in build-script #11717
Conversation
Rostepher
changed the title
|
@swift-ci test |
|
Just to double check. The benchmarks will still be built as part of smoke testing right? That is important to make sure that we do not break the benchmarks. As a nice win, you could also have the PR testing run the benchmarks, but force the benchmarks to be run for 1 sample per test where each sample uses 2 iterations. This should be pretty fast. But you would need to measure the amount of time it takes just to be careful. |
|
I'll need to confirm with @shahmishal, but if the smoke-test bots were already passing the |
|
@swift-ci smoke test |
utils/build-script
Outdated
| @@ -40,7 +40,7 @@ from swift_build_support.swift_build_support.SwiftBuildSupport import ( | |||
| SWIFT_REPO_NAME, | |||
| SWIFT_SOURCE_ROOT, | |||
| get_all_preset_names, | |||
| get_preset_options, | |||
There was a problem hiding this comment.
Nitpick: generally we frown on removing trailing space from an entire file because it muddies up the diff. While we don't want to introduce new ones, can you set your editor not to do it by default?
|
Build failed |
|
@swift-ci please test |
|
Build failed |
|
@swift-ci please smoke test |
|
For the short-while I'm going to hold off on merging this until I've finished cleaning up some parts of build-script. The first of which is #11827 |
|
@swift-ci Please nominate Explanation: Disable benchmark builds by default in build-script, and make it completely opt-in using the new --build-benchmarks and --test-benchmarks flags in build-script |
|
This pull request has been nominated for inclusion to master |
…efault, instead it's completely opt-in using the new `--build-benchmarks` and `--test-benchmarks` flags. The `--skip-build-benchmarks` and `--skip-test-benchmarks` flags are now no-ops and can be removed in a later cleanup pass once all uses have been thoroughly eradicated. The `-B` and `--benchmark` flags have been repurposed as an alias for both `--build-benchmarks` and `--test-benchmarks`.
Rostepher
force-pushed
the
bench-the-benchmarks
branch
from
1751689
to
825a18d
Compare
|
@swift-ci please test |
|
Build failed |
|
Build failed |
…st_benchmarks rather than the combined benchmark attribute in the argument namespace.
|
@swift-ci please test |
|
Build failed |
|
Build failed |
…or --build-benchmarks and --test-benchmarks to 0 (False).
|
@swift-ci please test |
|
Build failed |
|
Build failed |
|
@Rostepher Is this something you expect to merge soon? It would be great to no longer have to specify the option to disable building the benchmarks as it is easy to forget if you're doing a build without using a preset, alias, script, etc. |
|
@rudkx I'd like to merge this soon, but I need to resolve some conflicts and ensure my changes to the presets don't mess up the CI bots. Turns out that flipping the flag is not as easy as a simple search and replace due to the complex layers of presets and mixins we have. |
Purpose
This PR modifies
build-scriptandbuild-script-implsuch that they no longer build the benchmark suite by default. Instead it's completely opt-in using the new--build-benchmarksand--test-benchmarksflags inbuild-script. The--skip-build-benchmarksand--skip-test-benchmarksflags are now no-ops and can be removed in a later cleanup pass once all uses have been thoroughly eradicated. The-Band--benchmarkflags have been repurposed as an alias for both--build-benchmarksand--test-benchmarks. Changing the default behavior should not cause much (if any) problems for the vast majority of Swift contributors and instead it's likely that many developers will see improved compilation times (especially from a clean build).I've also taken the liberty to fix uses of
SKIP_TEST_BENCHMARKinbuild-script-impl, rather than the pluralSKIP_TEST_BENCHMARKS, which seems to be a typo as far as I can tell. A bit of digging shows it was added in a refactor, but the command line option is plural.rdar://problem/34375102