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.

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

Jump to bottom

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:

rust/src/bootstrap/src/core/config/config.rs

Lines 1429 to 1447 in 3314d5c

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:

rust/src/bootstrap/src/core/config/tests.rs

Lines 18 to 19 in 3314d5c

"--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:

rust/src/bootstrap/src/bin/main.rs

Lines 20 to 23 in 3314d5c

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
@onur-ozkan onur-ozkan mentioned this issue May 20, 2024
Merged
Nilstrieb added a commit to Nilstrieb/rust that referenced this issue Jun 4, 2024
@Nilstrieb
Rollup merge of rust-lang#125273 - onur-ozkan:bootstrap-self-test, r=…
7735809 
…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
@compiler-errors
Rollup merge of rust-lang#125273 - onur-ozkan:bootstrap-self-test, r=…
7525590 
…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
@fmease
Rollup merge of rust-lang#125273 - onur-ozkan:bootstrap-self-test, r=…
ae9aef2 
…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
@rust-timer
Unrolled build for rust-lang#125273
6737249 
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

bootstrap: implement new feature bootstrap-self-test onur-ozkan/rust
3 participants
@jieyouxu @onur-ozkan @rustbot