Skip to content

Commit

Permalink
Rollup merge of #128841 - lqd:rustc-args, r=onur-ozkan
Browse files Browse the repository at this point in the history
bootstrap: don't use rustflags for `--rustc-args`

r? `@onur-ozkan`

This is going to require a bit of context.

#47558 has added `--rustc-args` to `./x test` to allow passing flags when building `compiletest` tests. It was made specifically because using `RUSTFLAGS` would rebuild the compiler/stdlib, which would in turn require the flag you want to build tests with to successfully bootstrap.

#113178 made the request that it also works for other tests and doctests. This is not trivial to support across the board for `library`/`compiler` unit-tests/doctests and across stages. This issue was closed in #113948 by using `RUSTFLAGS`, seemingly incorrectly since #123489 fixed that part to make it work.

Unfortunately #123489/#113948 have regressed the goals of `--rustc-args`:
- now we can't use rustc args that don't bootstrap, to run the UI tests: we can't test incomplete features. The new trait solver doesn't bootstrap, in-progress borrowck/polonius changes don't bootstrap, some other features are similarly incomplete, etc.
- using the flag now rebuilds everything from scratch: stage0 stdlib, stage1 compiler, stage1 stdlib. You don't need to re-do all this to compile UI tests, you only need the latter to run stdlib tests with a new flag, etc. This happens for contributors, but also on CI today. (Not to mention that in doing that it will rebuild things with flags that are not meant to be used, e.g. stdlib cfgs that don't exist in the compiler; or you could also imagine that this silently enables flags that were not meant to be enabled in this way).

Since then, bd71c71 has started using it to test a stdlib feature, relying on the fact that it now rebuilds everything. So #125011 also regressed CI times more than necessary because it rebuilds everything instead of just stage 1 stdlib.

It's not easy for me to know how to properly fix #113178 in bootstrap, but #113948/#123489 are not it since they regress the initial intent. I'd think bootstrap would have to know from the list of test targets that are passed that the `library` or `compiler` paths that are passed could require rebuilding these crates with different rustflags, probably also depending on stages. Therefore I would not be able to fix it, and will just try in this PR to unregress the situation to unblock the initial use-case.

It seems miri now also uses `./x miri --rustc-args` in this incorrect meaning to rebuild the `library` paths they support to run with the new args. I've not made any bootstrap changes related to `./x miri` in this PR, so `--rustc-args` wouldn't work there anymore. I'd assume this would need to use rustflags again but I don't know how to make that work properly in bootstrap, hence opening as draft, so you can tell me how to do that. I assume we don't want to break their use-case again now that it exists, even though there are ways to use `./x test` to do exactly that.

`RUSTFLAGS_NOT_BOOTSTRAP=flag ./x test library/std` is a way to run unit tests with a new flag without rebuilding everything, while with #123489 there is no way anymore to run tests with a flag that doesn't bootstrap.

---
edit: after review, this PR:
- renames `./x test --rustc-args` to `./x test --compiletest-rustc-args` as it only applies there, and cannot use rustflags for this purpose.
- fixes the regression that using these args rebuilt everything from scratch
- speeds up some CI jobs via the above point
- removes `./x miri --rustc-args` as only library tests are supported, needs to rebuild libstd, and `./x miri --compiletest-rustc-args` wouldn't work since compiletests are not supported.
  • Loading branch information
matthiaskrgr authored Aug 13, 2024
2 parents 37b9567 + 2abfa35 commit 0643c3b
Show file tree
Hide file tree
Showing 11 changed files with 24 additions and 34 deletions.
4 changes: 2 additions & 2 deletions compiler/rustc_codegen_gcc/build_system/src/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -552,7 +552,7 @@ fn asm_tests(env: &Env, args: &TestArg) -> Result<(), String> {
&"--stage",
&"0",
&"tests/assembly/asm",
&"--rustc-args",
&"--compiletest-rustc-args",
&rustc_args,
],
Some(&rust_dir),
Expand Down Expand Up @@ -1020,7 +1020,7 @@ where
&"--stage",
&"0",
&format!("tests/{}", test_type),
&"--rustc-args",
&"--compiletest-rustc-args",
&rustc_args,
],
Some(&rust_path),
Expand Down
2 changes: 1 addition & 1 deletion src/bootstrap/src/core/build_steps/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1817,7 +1817,7 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the

let mut flags = if is_rustdoc { Vec::new() } else { vec!["-Crpath".to_string()] };
flags.push(format!("-Cdebuginfo={}", builder.config.rust_debuginfo_level_tests));
flags.extend(builder.config.cmd.rustc_args().iter().map(|s| s.to_string()));
flags.extend(builder.config.cmd.compiletest_rustc_args().iter().map(|s| s.to_string()));

if suite != "mir-opt" {
if let Some(linker) = builder.linker(target) {
Expand Down
5 changes: 0 additions & 5 deletions src/bootstrap/src/core/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2226,11 +2226,6 @@ impl<'a> Builder<'a> {
rustdocflags.arg("--cfg=parallel_compiler");
}

// Pass the value of `--rustc-args` from test command. If it's not a test command, this won't set anything.
self.config.cmd.rustc_args().iter().for_each(|v| {
rustflags.arg(v);
});

Cargo {
command: cargo,
compiler,
Expand Down
4 changes: 2 additions & 2 deletions src/bootstrap/src/core/builder/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -633,7 +633,7 @@ mod dist {
config.paths = vec!["library/std".into()];
config.cmd = Subcommand::Test {
test_args: vec![],
rustc_args: vec![],
compiletest_rustc_args: vec![],
no_fail_fast: false,
no_doc: true,
doc: false,
Expand Down Expand Up @@ -704,7 +704,7 @@ mod dist {
let mut config = configure(&["A-A"], &["A-A"]);
config.cmd = Subcommand::Test {
test_args: vec![],
rustc_args: vec![],
compiletest_rustc_args: vec![],
no_fail_fast: false,
doc: true,
no_doc: false,
Expand Down
13 changes: 5 additions & 8 deletions src/bootstrap/src/core/config/flags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -357,9 +357,9 @@ pub enum Subcommand {
/// extra arguments to be passed for the test tool being used
/// (e.g. libtest, compiletest or rustdoc)
test_args: Vec<String>,
/// extra options to pass the compiler when running tests
/// extra options to pass the compiler when running compiletest tests
#[arg(long, value_name = "ARGS", allow_hyphen_values(true))]
rustc_args: Vec<String>,
compiletest_rustc_args: Vec<String>,
#[arg(long)]
/// do not run doc tests
no_doc: bool,
Expand Down Expand Up @@ -402,9 +402,6 @@ pub enum Subcommand {
/// extra arguments to be passed for the test tool being used
/// (e.g. libtest, compiletest or rustdoc)
test_args: Vec<String>,
/// extra options to pass the compiler when running tests
#[arg(long, value_name = "ARGS", allow_hyphen_values(true))]
rustc_args: Vec<String>,
#[arg(long)]
/// do not run doc tests
no_doc: bool,
Expand Down Expand Up @@ -509,10 +506,10 @@ impl Subcommand {
}
}

pub fn rustc_args(&self) -> Vec<&str> {
pub fn compiletest_rustc_args(&self) -> Vec<&str> {
match *self {
Subcommand::Test { ref rustc_args, .. } | Subcommand::Miri { ref rustc_args, .. } => {
rustc_args.iter().flat_map(|s| s.split_whitespace()).collect()
Subcommand::Test { ref compiletest_rustc_args, .. } => {
compiletest_rustc_args.iter().flat_map(|s| s.split_whitespace()).collect()
}
_ => vec![],
}
Expand Down
5 changes: 5 additions & 0 deletions src/bootstrap/src/utils/change_tracker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -225,4 +225,9 @@ pub const CONFIG_CHANGE_HISTORY: &[ChangeInfo] = &[
severity: ChangeSeverity::Info,
summary: "New option `llvm.libzstd` to control whether llvm is built with zstd support.",
},
ChangeInfo {
change_id: 128841,
severity: ChangeSeverity::Warning,
summary: "./x test --rustc-args was renamed to --compiletest-rustc-args as it only applies there. ./x miri --rustc-args was also removed.",
},
];
6 changes: 3 additions & 3 deletions src/ci/docker/scripts/x86_64-gnu-llvm.sh
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ if [[ -z "${PR_CI_JOB}" ]]; then
# compiler, and is sensitive to the addition of new flags.
../x.py --stage 1 test tests/ui-fulldeps

# The tests are run a second time with the size optimizations enabled.
../x.py --stage 1 test library/std library/alloc library/core \
--rustc-args "--cfg feature=\"optimize_for_size\""
# Rebuild the stdlib with the size optimizations enabled and run tests again.
RUSTFLAGS_NOT_BOOTSTRAP="--cfg feature=\"optimize_for_size\"" ../x.py --stage 1 test \
library/std library/alloc library/core
fi

# NOTE: intentionally uses all of `x.py`, `x`, and `x.ps1` to make sure they all work on Linux.
Expand Down
3 changes: 1 addition & 2 deletions src/etc/completions/x.py.fish
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ complete -c x.py -n "__fish_seen_subcommand_from doc" -l enable-bolt-settings -d
complete -c x.py -n "__fish_seen_subcommand_from doc" -l skip-stage0-validation -d 'Skip stage0 compiler validation'
complete -c x.py -n "__fish_seen_subcommand_from doc" -s h -l help -d 'Print help (see more with \'--help\')'
complete -c x.py -n "__fish_seen_subcommand_from test" -l test-args -d 'extra arguments to be passed for the test tool being used (e.g. libtest, compiletest or rustdoc)' -r
complete -c x.py -n "__fish_seen_subcommand_from test" -l rustc-args -d 'extra options to pass the compiler when running tests' -r
complete -c x.py -n "__fish_seen_subcommand_from test" -l compiletest-rustc-args -d 'extra options to pass the compiler when running compiletest tests' -r
complete -c x.py -n "__fish_seen_subcommand_from test" -l extra-checks -d 'comma-separated list of other files types to check (accepts py, py:lint, py:fmt, shell)' -r
complete -c x.py -n "__fish_seen_subcommand_from test" -l compare-mode -d 'mode describing what file the actual ui output will be compared to' -r
complete -c x.py -n "__fish_seen_subcommand_from test" -l pass -d 'force {check,build,run}-pass tests to this mode' -r
Expand Down Expand Up @@ -313,7 +313,6 @@ complete -c x.py -n "__fish_seen_subcommand_from test" -l enable-bolt-settings -
complete -c x.py -n "__fish_seen_subcommand_from test" -l skip-stage0-validation -d 'Skip stage0 compiler validation'
complete -c x.py -n "__fish_seen_subcommand_from test" -s h -l help -d 'Print help (see more with \'--help\')'
complete -c x.py -n "__fish_seen_subcommand_from miri" -l test-args -d 'extra arguments to be passed for the test tool being used (e.g. libtest, compiletest or rustdoc)' -r
complete -c x.py -n "__fish_seen_subcommand_from miri" -l rustc-args -d 'extra options to pass the compiler when running tests' -r
complete -c x.py -n "__fish_seen_subcommand_from miri" -l config -d 'TOML configuration file for build' -r -F
complete -c x.py -n "__fish_seen_subcommand_from miri" -l build-dir -d 'Build directory, overrides `build.build-dir` in `config.toml`' -r -f -a "(__fish_complete_directories)"
complete -c x.py -n "__fish_seen_subcommand_from miri" -l build -d 'build target of the stage0 compiler' -r -f
Expand Down
3 changes: 1 addition & 2 deletions src/etc/completions/x.py.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ Register-ArgumentCompleter -Native -CommandName 'x.py' -ScriptBlock {
}
'x.py;test' {
[CompletionResult]::new('--test-args', 'test-args', [CompletionResultType]::ParameterName, 'extra arguments to be passed for the test tool being used (e.g. libtest, compiletest or rustdoc)')
[CompletionResult]::new('--rustc-args', 'rustc-args', [CompletionResultType]::ParameterName, 'extra options to pass the compiler when running tests')
[CompletionResult]::new('--compiletest-rustc-args', 'compiletest-rustc-args', [CompletionResultType]::ParameterName, 'extra options to pass the compiler when running compiletest tests')
[CompletionResult]::new('--extra-checks', 'extra-checks', [CompletionResultType]::ParameterName, 'comma-separated list of other files types to check (accepts py, py:lint, py:fmt, shell)')
[CompletionResult]::new('--compare-mode', 'compare-mode', [CompletionResultType]::ParameterName, 'mode describing what file the actual ui output will be compared to')
[CompletionResult]::new('--pass', 'pass', [CompletionResultType]::ParameterName, 'force {check,build,run}-pass tests to this mode')
Expand Down Expand Up @@ -392,7 +392,6 @@ Register-ArgumentCompleter -Native -CommandName 'x.py' -ScriptBlock {
}
'x.py;miri' {
[CompletionResult]::new('--test-args', 'test-args', [CompletionResultType]::ParameterName, 'extra arguments to be passed for the test tool being used (e.g. libtest, compiletest or rustdoc)')
[CompletionResult]::new('--rustc-args', 'rustc-args', [CompletionResultType]::ParameterName, 'extra options to pass the compiler when running tests')
[CompletionResult]::new('--config', 'config', [CompletionResultType]::ParameterName, 'TOML configuration file for build')
[CompletionResult]::new('--build-dir', 'build-dir', [CompletionResultType]::ParameterName, 'Build directory, overrides `build.build-dir` in `config.toml`')
[CompletionResult]::new('--build', 'build', [CompletionResultType]::ParameterName, 'build target of the stage0 compiler')
Expand Down
10 changes: 3 additions & 7 deletions src/etc/completions/x.py.sh
Original file line number Diff line number Diff line change
Expand Up @@ -1300,7 +1300,7 @@ _x.py() {
return 0
;;
x.py__miri)
opts="-v -i -j -h --no-fail-fast --test-args --rustc-args --no-doc --doc --verbose --incremental --config --build-dir --build --host --target --exclude --skip --include-default-paths --rustc-error-format --on-fail --dry-run --dump-bootstrap-shims --stage --keep-stage --keep-stage-std --src --jobs --warnings --error-format --json-output --color --bypass-bootstrap-lock --llvm-skip-rebuild --rust-profile-generate --rust-profile-use --llvm-profile-use --llvm-profile-generate --enable-bolt-settings --skip-stage0-validation --reproducible-artifact --set --help [PATHS]... [ARGS]..."
opts="-v -i -j -h --no-fail-fast --test-args --no-doc --doc --verbose --incremental --config --build-dir --build --host --target --exclude --skip --include-default-paths --rustc-error-format --on-fail --dry-run --dump-bootstrap-shims --stage --keep-stage --keep-stage-std --src --jobs --warnings --error-format --json-output --color --bypass-bootstrap-lock --llvm-skip-rebuild --rust-profile-generate --rust-profile-use --llvm-profile-use --llvm-profile-generate --enable-bolt-settings --skip-stage0-validation --reproducible-artifact --set --help [PATHS]... [ARGS]..."
if [[ ${cur} == -* || ${COMP_CWORD} -eq 2 ]] ; then
COMPREPLY=( $(compgen -W "${opts}" -- "${cur}") )
return 0
Expand All @@ -1310,10 +1310,6 @@ _x.py() {
COMPREPLY=($(compgen -f "${cur}"))
return 0
;;
--rustc-args)
COMPREPLY=($(compgen -f "${cur}"))
return 0
;;
--config)
COMPREPLY=($(compgen -f "${cur}"))
return 0
Expand Down Expand Up @@ -1862,7 +1858,7 @@ _x.py() {
return 0
;;
x.py__test)
opts="-v -i -j -h --no-fail-fast --test-args --rustc-args --no-doc --doc --bless --extra-checks --force-rerun --only-modified --compare-mode --pass --run --rustfix-coverage --verbose --incremental --config --build-dir --build --host --target --exclude --skip --include-default-paths --rustc-error-format --on-fail --dry-run --dump-bootstrap-shims --stage --keep-stage --keep-stage-std --src --jobs --warnings --error-format --json-output --color --bypass-bootstrap-lock --llvm-skip-rebuild --rust-profile-generate --rust-profile-use --llvm-profile-use --llvm-profile-generate --enable-bolt-settings --skip-stage0-validation --reproducible-artifact --set --help [PATHS]... [ARGS]..."
opts="-v -i -j -h --no-fail-fast --test-args --compiletest-rustc-args --no-doc --doc --bless --extra-checks --force-rerun --only-modified --compare-mode --pass --run --rustfix-coverage --verbose --incremental --config --build-dir --build --host --target --exclude --skip --include-default-paths --rustc-error-format --on-fail --dry-run --dump-bootstrap-shims --stage --keep-stage --keep-stage-std --src --jobs --warnings --error-format --json-output --color --bypass-bootstrap-lock --llvm-skip-rebuild --rust-profile-generate --rust-profile-use --llvm-profile-use --llvm-profile-generate --enable-bolt-settings --skip-stage0-validation --reproducible-artifact --set --help [PATHS]... [ARGS]..."
if [[ ${cur} == -* || ${COMP_CWORD} -eq 2 ]] ; then
COMPREPLY=( $(compgen -W "${opts}" -- "${cur}") )
return 0
Expand All @@ -1872,7 +1868,7 @@ _x.py() {
COMPREPLY=($(compgen -f "${cur}"))
return 0
;;
--rustc-args)
--compiletest-rustc-args)
COMPREPLY=($(compgen -f "${cur}"))
return 0
;;
Expand Down
3 changes: 1 addition & 2 deletions src/etc/completions/x.py.zsh
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ _arguments "${_arguments_options[@]}" \
(test)
_arguments "${_arguments_options[@]}" \
'*--test-args=[extra arguments to be passed for the test tool being used (e.g. libtest, compiletest or rustdoc)]:ARGS: ' \
'*--rustc-args=[extra options to pass the compiler when running tests]:ARGS: ' \
'*--compiletest-rustc-args=[extra options to pass the compiler when running compiletest tests]:ARGS: ' \
'--extra-checks=[comma-separated list of other files types to check (accepts py, py\:lint, py\:fmt, shell)]:EXTRA_CHECKS: ' \
'--compare-mode=[mode describing what file the actual ui output will be compared to]:COMPARE MODE: ' \
'--pass=[force {check,build,run}-pass tests to this mode]:check | build | run: ' \
Expand Down Expand Up @@ -393,7 +393,6 @@ _arguments "${_arguments_options[@]}" \
(miri)
_arguments "${_arguments_options[@]}" \
'*--test-args=[extra arguments to be passed for the test tool being used (e.g. libtest, compiletest or rustdoc)]:ARGS: ' \
'*--rustc-args=[extra options to pass the compiler when running tests]:ARGS: ' \
'--config=[TOML configuration file for build]:FILE:_files' \
'--build-dir=[Build directory, overrides \`build.build-dir\` in \`config.toml\`]:DIR:_files -/' \
'--build=[build target of the stage0 compiler]:BUILD:( )' \
Expand Down

0 comments on commit 0643c3b

Please sign in to comment.