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

rust.codegen-backends interacts confusingly with paths #109610

Closed
jyn514 opened this issue Mar 25, 2023 · 4 comments · Fixed by #109642
Closed

rust.codegen-backends interacts confusingly with paths #109610

jyn514 opened this issue Mar 25, 2023 · 4 comments · Fixed by #109642
Assignees
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@jyn514
Copy link
Member

jyn514 commented Mar 25, 2023

I tried this code:

x build rustc_codegen_cranelift

I expected this to happen: Bootstrap builds cranelift, since I named it explicitly.
Instead, this happened: Nothing, since I didn't have cranelift configured in codegen-backends:

rust/src/bootstrap/compile.rs

Lines 998 to 1009 in ce6adcc

impl Step for CodegenBackend {
type Output = ();
const ONLY_HOSTS: bool = true;
// Only the backends specified in the `codegen-backends` entry of `config.toml` are built.
const DEFAULT: bool = true;
fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
run.paths(&["compiler/rustc_codegen_cranelift", "compiler/rustc_codegen_gcc"])
}
fn make_run(run: RunConfig<'_>) {
for &backend in &run.builder.config.rust_codegen_backends {

At a minimum, I would expect this to give an error or warning that I need to configure codegen-backends. It would be even nicer if we could treat codegen-backends as only controlling the defaults, instead of as a hard-off switch, like how rust.compiler-docs works. That should be possible to do by looking at the run.paths we're passed.

cc @bjorn3

@jyn514 jyn514 added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) A-control-flow Area: Relating to control flow A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. and removed A-control-flow Area: Relating to control flow labels Mar 25, 2023
@lenko-d
Copy link
Contributor

lenko-d commented Mar 25, 2023

@rustbot claim

@bjorn3
Copy link
Member

bjorn3 commented Mar 25, 2023

The first element in codegen-backends is used as default backend. In addition even if the CodegenBackend step were to directly use the passed in paths, assembling the toolchain did still only add the backends in codegen-backends to the sysroot. Having path specifiers for one step affect another step doesn't feel right. For check builds I do agree this is an improvement. Although I personally actually use this behavior to have ./x.py check compiler/rustc_codegen_cranelift build all backends, not just cranelift (by enabling all in codegen-backends) It did be nice to have a replacement for this. In addition for ./x.py build at the very least a warning makes sense.

@jyn514
Copy link
Member Author

jyn514 commented Mar 25, 2023

Having path specifiers for one step affect another step doesn't feel right.

Does it make sense to separate cg_clif and cg_gcc into different steps in that case?

cc @antoyo

@bjorn3
Copy link
Member

bjorn3 commented Mar 25, 2023

That doesn't have any effect on if specifying a backend as path will add it to the sysroot.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 2, 2023
…racts_confusingly_with_paths, r=Mark-Simulacrum

check for missing codegen backeng config

Fixes [rust-lang#109610](rust-lang#109610)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 3, 2023
…racts_confusingly_with_paths, r=Mark-Simulacrum

check for missing codegen backeng config

Fixes [rust-lang#109610](rust-lang#109610)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. 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