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

complicated build stages #119946

Open
onur-ozkan opened this issue Jan 13, 2024 · 4 comments
Open

complicated build stages #119946

onur-ozkan opened this issue Jan 13, 2024 · 4 comments
Labels
T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@onur-ozkan
Copy link
Member

onur-ozkan commented Jan 13, 2024

Current approach on bootstrap stages adds too much complexity on the bootstrap codebase; the complexity continues to grow with the additional changes made on bootstrap and it's not even functioning correctly (like writing incorrect stages to stdout on certain commands).

One possible solution could be to handle stage behaviours as part of the Step trait individually on Step implementation.

Some references to why current implementation of step calculations is making things so difficult/complicated:

impl Step for Rustc {
// We return the stage of the "actual" compiler (not the uplifted one).
//
// By "actual" we refer to the uplifting logic where we may not compile the requested stage;
// instead, we uplift it from the previous stages. Which can lead to bootstrap failures in
// specific situations where we request stage X from other steps. However we may end up
// uplifting it from stage Y, causing the other stage to fail when attempting to link with
// stage X which was never actually built.
type Output = u32;

let compiler_to_use = builder.compiler_for(compiler.stage, compiler.host, target);
if compiler_to_use != compiler {
builder.ensure(Rustc::new(compiler_to_use, target));
let msg = if compiler_to_use.host == target {
format!(
"Uplifting rustc (stage{} -> stage{})",
compiler_to_use.stage,
compiler.stage + 1
)
} else {
format!(
"Uplifting rustc (stage{}:{} -> stage{}:{})",
compiler_to_use.stage,
compiler_to_use.host,
compiler.stage + 1,
target
)
};
builder.info(&msg);
builder.ensure(RustcLink::from_rustc(self, compiler_to_use));
return compiler_to_use.stage;
}

Particularly, the behaviors of the force_use_stage function are not really useful for all use cases (for instance, see #118233 and #118918)

rust/src/bootstrap/src/lib.rs

Lines 1403 to 1417 in ef32456

fn force_use_stage1(&self, stage: u32, target: TargetSelection) -> bool {
!self.config.full_bootstrap
&& !self.config.download_rustc()
&& stage >= 2
&& (self.hosts.iter().any(|h| *h == target) || target == self.build)
}
/// Checks whether the `compiler` compiling for `target` should be forced to
/// use a stage2 compiler instead.
///
/// When we download the pre-compiled version of rustc and compiler stage is >= 2,
/// it should be forced to use a stage2 compiler.
fn force_use_stage2(&self, stage: u32) -> bool {
self.config.download_rustc() && stage >= 2
}

@rustbot label +T-bootstrap

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

I think some of this logic is also just wrong. For instance:

if (false $(|| !$add_bins_to_sysroot.is_empty())?) && $sel.compiler.stage > 0 {
let bindir = $builder.sysroot($sel.compiler).join("bin");
t!(fs::create_dir_all(&bindir));
#[allow(unused_variables)]
let tools_out = $builder
.cargo_out($sel.compiler, Mode::ToolRustc, $sel.target);
$(for add_bin in $add_bins_to_sysroot {
let bin_source = tools_out.join(exe(add_bin, $sel.target));
let bin_destination = bindir.join(exe(add_bin, $sel.compiler.host));
$builder.copy_link(&bin_source, &bin_destination);
})?
let tool = bindir.join(exe($tool_name, $sel.compiler.host));
tool
} else {
tool
}

This copies the clippy/miri built with the stage N compiler into the stage N sysroot. But that makes no sense: for rustc we copy the rustc built with the stage N compiler into the stage N+1 sysroot. The same should happen here.

Currently, miri.exe that is copied to the stage 1 sysroot needs to be loaded with the libraries that are in the stage 2 sysroot. Clearly that makes no sense.

@RalfJung
Copy link
Member

RalfJung commented Apr 1, 2024

So specifically, currently we're copying from stage1-tools-bin to stage1/bin. But we're also copying from stage1-rustc to stage2/bin. One of these has to be wrong, I think -- and I think it's the tools that are wrong.

@onur-ozkan
Copy link
Member Author

onur-ozkan commented Apr 1, 2024

Some references to why current implementation of step calculations is making things so difficult/complicated:

Additional ref. to this issue: https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/Stage.20for.20copying.20tools.20into.20the.20sysroot

bors added a commit to rust-lang-ci/rust that referenced this issue Sep 24, 2024
Fix cargo staging for run-make tests

Follow-up to rust-lang#130642 (comment) to make sure that when

```
$ COMPILETEST_FORCE_STAGE0=1 ./x test run-make --stage 0
```

is used, bootstrap cargo is used in order to avoid building stage 1 rustc. Note that run-make tests are usually not written with `--stage 0` in mind and some tests may rely on stage1 rustc (nightly) behavior, and it is expected that some tests will fail under this invocation.

This PR also fixes `tool::Cargo` staging in compiletest when preparing for `run-make` test mode, by chopping off a stage from the `compiler` passed to `tool::Cargo` such that when the user invokes with stage `N`

```
./x test run-make --stage N
```

the `run-make` test suite will be tested against the cargo built by stage `N` compiler. Let's take `N=1`, i.e. `--stage 1`, without chopping off a stage, previously `./x test run-make --stage 1` will cause stage 1 rustc + std to be built, then stage 2 rustc, and cargo will be produced by the stage 2 rustc, which is clearly not what we want. By chopping off a stage, it means that cargo will be produced by the stage 1 rustc.

cc rust-lang#119946, rust-lang#59864.
See discussions regarding the tool staging at https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/.E2.9C.94.20stage1.20run-make.20tests.20now.20need.20stage2.20rustc.20built.20for.20c.2E.2E.2E.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

No branches or pull requests

3 participants