Skip to content
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.

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

refactor configuration parsing #122090

Closed
onur-ozkan opened this issue Mar 6, 2024 · 1 comment · Fixed by #125273
Closed

refactor configuration parsing #122090

onur-ozkan opened this issue Mar 6, 2024 · 1 comment · Fixed by #125273
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@onur-ozkan
Copy link
Member

Currently in config parser flow there are multiple other tasks involved such as validating stage 0 binaries and LLVM tools and downloading the beta toolchain. These logics are currently coupled in a single function as can be seen here:

config.initial_rustc = if let Some(rustc) = rustc {
if !flags.skip_stage0_validation {
config.check_stage0_version(&rustc, "rustc");
}
PathBuf::from(rustc)
} else {
config.download_beta_toolchain();
config.out.join(config.build.triple).join("stage0/bin/rustc")
};
config.initial_cargo = if let Some(cargo) = cargo {
if !flags.skip_stage0_validation {
config.check_stage0_version(&cargo, "cargo");
}
PathBuf::from(cargo)
} else {
config.download_beta_toolchain();
config.out.join(config.build.triple).join("stage0/bin/cargo")
};

Doing this results in performing downloads, expecting valid stage 0 binaries and LLVM tools even in unit tests of bootstrap itself.

To avoid this issue, we pass some configurations from the test functions like this:

"--config=/does/not/exist".to_owned(),
"--skip-stage0-validation".to_owned(),

However, this approach is not sustainable as it could be easily missed when adding new tests. If we simplify parser functions to only parse values and perform validations and downloads immediately after the configuration parsing, this could resolve all the issues and make this process much simpler.

@onur-ozkan onur-ozkan added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Mar 6, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 6, 2024
@onur-ozkan
Copy link
Member Author

perform validations and downloads immediately after the configuration parsing

This should be the right place:

fn main() {
let args = env::args().skip(1).collect::<Vec<_>>();
let config = Config::parse(&args);

@onur-ozkan onur-ozkan removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 6, 2024
@jieyouxu jieyouxu added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Mar 6, 2024
Noratrieb added a commit to Noratrieb/rust that referenced this issue Jun 4, 2024
…albertlarsan68

bootstrap: implement new feature `bootstrap-self-test`

Some of the bootstrap logics should be ignored during unit tests because they either make the tests take longer or cause them to fail. Therefore we need to be able to exclude them from the bootstrap when it's called by unit tests. This change introduces a new feature called `bootstrap-self-test`, which is enabled on bootstrap unit tests by default. This allows us to keep the logic separate between compiler builds and bootstrap tests without needing messy workarounds (like checking if target names match those in the unit tests).

Also, resolves rust-lang#122090 (without having to create separate modules)
compiler-errors added a commit to compiler-errors/rust that referenced this issue Jun 4, 2024
…albertlarsan68

bootstrap: implement new feature `bootstrap-self-test`

Some of the bootstrap logics should be ignored during unit tests because they either make the tests take longer or cause them to fail. Therefore we need to be able to exclude them from the bootstrap when it's called by unit tests. This change introduces a new feature called `bootstrap-self-test`, which is enabled on bootstrap unit tests by default. This allows us to keep the logic separate between compiler builds and bootstrap tests without needing messy workarounds (like checking if target names match those in the unit tests).

Also, resolves rust-lang#122090 (without having to create separate modules)
fmease added a commit to fmease/rust that referenced this issue Jun 4, 2024
…albertlarsan68

bootstrap: implement new feature `bootstrap-self-test`

Some of the bootstrap logics should be ignored during unit tests because they either make the tests take longer or cause them to fail. Therefore we need to be able to exclude them from the bootstrap when it's called by unit tests. This change introduces a new feature called `bootstrap-self-test`, which is enabled on bootstrap unit tests by default. This allows us to keep the logic separate between compiler builds and bootstrap tests without needing messy workarounds (like checking if target names match those in the unit tests).

Also, resolves rust-lang#122090 (without having to create separate modules)
@bors bors closed this as completed in 0f86182 Jun 5, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jun 5, 2024
Rollup merge of rust-lang#125273 - onur-ozkan:bootstrap-self-test, r=albertlarsan68

bootstrap: implement new feature `bootstrap-self-test`

Some of the bootstrap logics should be ignored during unit tests because they either make the tests take longer or cause them to fail. Therefore we need to be able to exclude them from the bootstrap when it's called by unit tests. This change introduces a new feature called `bootstrap-self-test`, which is enabled on bootstrap unit tests by default. This allows us to keep the logic separate between compiler builds and bootstrap tests without needing messy workarounds (like checking if target names match those in the unit tests).

Also, resolves rust-lang#122090 (without having to create separate modules)
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue Jun 28, 2024
…san68

bootstrap: implement new feature `bootstrap-self-test`

Some of the bootstrap logics should be ignored during unit tests because they either make the tests take longer or cause them to fail. Therefore we need to be able to exclude them from the bootstrap when it's called by unit tests. This change introduces a new feature called `bootstrap-self-test`, which is enabled on bootstrap unit tests by default. This allows us to keep the logic separate between compiler builds and bootstrap tests without needing messy workarounds (like checking if target names match those in the unit tests).

Also, resolves rust-lang/rust#122090 (without having to create separate modules)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants