Skip to content

fix(bootstrap): rename exclude flag to skip 🐛 #114001

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 10, 2023
Merged

fix(bootstrap): rename exclude flag to skip 🐛 #114001

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ jobs:
os: macos-13
- name: x86_64-apple-1
env:
SCRIPT: "./x.py --stage 2 test --exclude tests/ui --exclude tests/rustdoc --exclude tests/run-make-fulldeps"
SCRIPT: "./x.py --stage 2 test --skip tests/ui --skip tests/rustdoc --skip tests/run-make-fulldeps"
RUST_CONFIGURE_ARGS: "--build=x86_64-apple-darwin --enable-sanitizers --enable-profiler --set rust.jemalloc --set llvm.ninja=false"
RUSTC_RETRY_LINKER_ON_SEGFAULT: 1
MACOSX_DEPLOYMENT_TARGET: 10.8
Expand Down
10 changes: 4 additions & 6 deletions src/bootstrap/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,19 +317,17 @@ impl StepDescription {
}

fn is_excluded(&self, builder: &Builder<'_>, pathset: &PathSet) -> bool {
if builder.config.exclude.iter().any(|e| pathset.has(&e, builder.kind)) {
if builder.config.skip.iter().any(|e| pathset.has(&e, builder.kind)) {
if !matches!(builder.config.dry_run, DryRun::SelfCheck) {
println!("Skipping {pathset:?} because it is excluded");
}
return true;
}

if !builder.config.exclude.is_empty()
&& !matches!(builder.config.dry_run, DryRun::SelfCheck)
{
if !builder.config.skip.is_empty() && !matches!(builder.config.dry_run, DryRun::SelfCheck) {
builder.verbose(&format!(
"{:?} not skipped for {:?} -- not in {:?}",
pathset, self.name, builder.config.exclude
pathset, self.name, builder.config.skip
));
}
false
Expand Down Expand Up @@ -2129,7 +2127,7 @@ impl<'a> Builder<'a> {
let desc = StepDescription::from::<S>(kind);
let should_run = (desc.should_run)(ShouldRun::new(self, desc.kind));

// Avoid running steps contained in --exclude
// Avoid running steps contained in --skip
for pathset in &should_run.paths {
if desc.is_excluded(self, pathset) {
return None;
Expand Down
4 changes: 2 additions & 2 deletions src/bootstrap/builder/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ fn test_intersection() {
#[test]
fn test_exclude() {
let mut config = configure("test", &["A"], &["A"]);
config.exclude = vec!["src/tools/tidy".into()];
config.skip = vec!["src/tools/tidy".into()];
let cache = run_build(&[], config);

// Ensure we have really excluded tidy
Expand All @@ -137,7 +137,7 @@ fn test_exclude_kind() {
// Ensure our test is valid, and `test::Rustc` would be run without the exclude.
assert!(run_build(&[], config.clone()).contains::<test::CrateLibrustc>());
// Ensure tests for rustc are skipped.
config.exclude = vec![path.clone()];
config.skip = vec![path.clone()];
assert!(!run_build(&[], config.clone()).contains::<test::CrateLibrustc>());
// Ensure builds for rustc are not skipped.
assert!(run_build(&[], config).contains::<compile::Rustc>());
Expand Down
4 changes: 2 additions & 2 deletions src/bootstrap/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ pub struct Config {
pub sanitizers: bool,
pub profiler: bool,
pub omit_git_hash: bool,
pub exclude: Vec<PathBuf>,
pub skip: Vec<PathBuf>,
pub include_default_paths: bool,
pub rustc_error_format: Option<String>,
pub json_output: bool,
Expand Down Expand Up @@ -1112,7 +1112,7 @@ impl Config {

// Set flags.
config.paths = std::mem::take(&mut flags.paths);
config.exclude = flags.exclude;
config.skip = flags.skip.into_iter().chain(flags.exclude).collect();
config.include_default_paths = flags.include_default_paths;
config.rustc_error_format = flags.rustc_error_format;
config.json_output = flags.json_output;
Expand Down
2 changes: 1 addition & 1 deletion src/bootstrap/doc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -570,7 +570,7 @@ fn doc_std(
if builder.no_std(target) == Some(true) {
panic!(
"building std documentation for no_std target {target} is not supported\n\
Set `docs = false` in the config to disable documentation, or pass `--exclude doc::library`."
Set `docs = false` in the config to disable documentation, or pass `--skip library`."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changed more that exclude/skip replace?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@klensy
Sorry, but I don't get it. Would you explain more, please?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before: --exclude doc::library,
after: --skip library

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this part is clear to me
I don't get what you are saying
are you suggesting that this change is irrelevant?

Copy link
Contributor

@klensy klensy Aug 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably. Is this suggestion still valid after change?
Running python x.py --stage=0 doc --exclude library on master version works, but --exclude doc::library actually not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running this on master:

x --stage=0 doc --exclude library

Output:

...truncated
Skipping Set({doc::library}) because it is excluded
...truncated

running it on my change:

x --stage=0 doc --exclude library
...truncated
Skipping Set({doc::library}) because it is excluded
...truncated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran the same with doc::library
nothing got excluded

note that @jyn514 explained this in here (the last line)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, ok.

);
}

Expand Down
7 changes: 5 additions & 2 deletions src/bootstrap/flags.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,10 @@ pub struct Flags {

#[arg(global(true), long, value_name = "PATH")]
/// build paths to exclude
pub exclude: Vec<PathBuf>,
pub exclude: Vec<PathBuf>, // keeping for client backward compatibility
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe change the comment to "build paths to skip, prefer --skip instead` because having both "build paths to exclude" and "build paths to skip" is a bit confusing.

Ideally, we'd hide it from the help message. I see https://docs.rs/clap/latest/clap/struct.Arg.html#method.hide, but I can't seem to make it work for arg.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it's #[arg(hide(true))].

#[arg(global(true), long, value_name = "PATH")]
/// build paths to skip
pub skip: Vec<PathBuf>,
#[arg(global(true), long)]
/// include default paths in addition to the provided ones
pub include_default_paths: bool,
Expand Down Expand Up @@ -318,7 +321,7 @@ pub enum Subcommand {
no_fail_fast: bool,
#[arg(long, value_name = "SUBSTRING")]
/// skips tests matching SUBSTRING, if supported by test tool. May be passed multiple times
skip: Vec<String>,
skip: Vec<PathBuf>,
#[arg(long, value_name = "ARGS", allow_hyphen_values(true))]
/// extra arguments to be passed for the test tool being used
/// (e.g. libtest, compiletest or rustdoc)
Expand Down
4 changes: 2 additions & 2 deletions src/bootstrap/mk/Makefile.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ prepare:
ci-msvc-py:
$(Q)$(CFG_SRC_DIR)/x.py test --stage 2 tidy
ci-msvc-ps1:
$(Q)$(CFG_SRC_DIR)/x.ps1 test --stage 2 --exclude tidy
$(Q)$(CFG_SRC_DIR)/x.ps1 test --stage 2 --skip tidy
ci-msvc: ci-msvc-py ci-msvc-ps1

## MingW native builders
Expand All @@ -72,7 +72,7 @@ ci-msvc: ci-msvc-py ci-msvc-ps1
ci-mingw-x:
$(Q)$(CFG_SRC_DIR)/x test --stage 2 tidy
ci-mingw-bootstrap:
$(Q)$(BOOTSTRAP) test --stage 2 --exclude tidy
$(Q)$(BOOTSTRAP) test --stage 2 --skip tidy
ci-mingw: ci-mingw-x ci-mingw-bootstrap

.PHONY: dist
6 changes: 3 additions & 3 deletions src/bootstrap/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ impl Step for Linkcheck {
if (hosts != targets) && !hosts.is_empty() && !targets.is_empty() {
panic!(
"Linkcheck currently does not support builds with different hosts and targets.
You can skip linkcheck with --exclude src/tools/linkchecker"
You can skip linkcheck with --skip src/tools/linkchecker"
);
}

Expand Down Expand Up @@ -1104,7 +1104,7 @@ impl Step for Tidy {
error: no `rustfmt` binary found in {PATH}
info: `rust.channel` is currently set to \"{CHAN}\"
help: if you are testing a beta branch, set `rust.channel` to \"beta\" in the `config.toml` file
help: to skip test's attempt to check tidiness, pass `--exclude src/tools/tidy` to `x.py test`",
help: to skip test's attempt to check tidiness, pass `--skip src/tools/tidy` to `x.py test`",
PATH = inferred_rustfmt_dir.display(),
CHAN = builder.config.channel,
);
Expand Down Expand Up @@ -1650,7 +1650,7 @@ note: if you're sure you want to do this, please open an issue as to why. In the
cmd.arg("--run-clang-based-tests-with").arg(clang_exe);
}

for exclude in &builder.config.exclude {
for exclude in &builder.config.skip {
cmd.arg("--skip");
cmd.arg(&exclude);
}
Expand Down
10 changes: 5 additions & 5 deletions src/ci/docker/host-x86_64/i686-gnu/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ COPY scripts/sccache.sh /scripts/
RUN sh /scripts/sccache.sh

ENV RUST_CONFIGURE_ARGS --build=i686-unknown-linux-gnu
# Exclude some tests that are unlikely to be platform specific, to speed up
# Skip some tests that are unlikely to be platform specific, to speed up
# this slow job.
ENV SCRIPT python3 ../x.py --stage 2 test \
--exclude src/bootstrap \
--exclude tests/rustdoc-js \
--exclude src/tools/error_index_generator \
--exclude src/tools/linkchecker
--skip src/bootstrap \
--skip tests/rustdoc-js \
--skip src/tools/error_index_generator \
--skip src/tools/linkchecker
2 changes: 1 addition & 1 deletion src/ci/docker/host-x86_64/wasm32/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,4 @@ RUN chown 10719 -R /emsdk-portable/

# Exclude library/alloc due to OOM in benches.
ENV SCRIPT python3 ../x.py test --stage 2 --host='' --target $TARGETS \
--exclude library/alloc
--skip library/alloc
4 changes: 2 additions & 2 deletions src/ci/docker/host-x86_64/x86_64-gnu-llvm-15/script.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ set -ex

# Only run the stage 1 tests on merges, not on PR CI jobs.
if [[ -z "${PR_CI_JOB}" ]]; then
../x.py --stage 1 test --exclude src/tools/tidy && \
../x.py --stage 1 test --skip src/tools/tidy && \
# Run the `mir-opt` tests again but this time for a 32-bit target.
# This enforces that tests using `// EMIT_MIR_FOR_EACH_BIT_WIDTH` have
# both 32-bit and 64-bit outputs updated by the PR author, before
Expand All @@ -16,7 +16,7 @@ if [[ -z "${PR_CI_JOB}" ]]; then
fi

# NOTE: intentionally uses all of `x.py`, `x`, and `x.ps1` to make sure they all work on Linux.
../x.py --stage 2 test --exclude src/tools/tidy && \
../x.py --stage 2 test --skip src/tools/tidy && \
# Run the `mir-opt` tests again but this time for a 32-bit target.
# This enforces that tests using `// EMIT_MIR_FOR_EACH_BIT_WIDTH` have
# both 32-bit and 64-bit outputs updated by the PR author, before
Expand Down
4 changes: 1 addition & 3 deletions src/ci/github-actions/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
# step CI will fail.

---

###############################
# YAML Anchors Definition #
###############################
Expand All @@ -30,7 +29,6 @@
# The expand-yaml-anchors tool will automatically remove this block from the
# output YAML file.
x--expand-yaml-anchors--remove:

- &shared-ci-variables
CI_JOB_NAME: ${{ matrix.name }}
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
Expand Down Expand Up @@ -520,7 +518,7 @@ jobs:

- name: x86_64-apple-1
env: &env-x86_64-apple-tests
SCRIPT: ./x.py --stage 2 test --exclude tests/ui --exclude tests/rustdoc --exclude tests/run-make-fulldeps
SCRIPT: ./x.py --stage 2 test --skip tests/ui --skip tests/rustdoc --skip tests/run-make-fulldeps
RUST_CONFIGURE_ARGS: --build=x86_64-apple-darwin --enable-sanitizers --enable-profiler --set rust.jemalloc --set llvm.ninja=false
RUSTC_RETRY_LINKER_ON_SEGFAULT: 1
MACOSX_DEPLOYMENT_TARGET: 10.8
Expand Down
Loading