Remove missing_tools config

[Gankra]

The scope of this issue changed; see here for the current status.

Original issue description for historians:

---

config.toml lets you request tools be built, and it lets you ignore build failures with dist.missing-tools = true, but it doesn't let you specify that some packages must be built, while others are ok to miss.

Ideally, it would support either an optional-tools array, or missing-tools could take an array of tools that are ok to miss, so that this nuance could be expressed.

This is a little niche, but I'm running into this while hooking up a "build rust from source" option in firefox's CI, where rustfmt is optional but nice-to-have for lints. Currently I think the best option is to using missing-tools = true and check the output for warnings about skipped tools in the dist step. This is a bit fragile though, so it would be nice if this was built in.

@Mark-Simulacrum mentionned this would also be desirable for miri on stable/beta?

[RalfJung]

@Mark-Simulacrum mentionned this would also be desirable for miri on stable/beta?

That might be related to #74709.

[jyn514]

This is a little niche, but I'm running into this while hooking up a "build rust from source" option in firefox's CI, where rustfmt is optional but nice-to-have for lints. Currently I think the best option is to using missing-tools = true and check the output for warnings about skipped tools in the dist step. This is a bit fragile though, so it would be nice if this was built in.

Note that rustfmt always compiles successfully now, and miri should too. So I'm not sure missing-tools is useful at all anymore? I can't think of anything that would fail to build.

[jyn514]

Mentoring instructions:

1. Change ToolBuild to unconditionally require tools to build:

   rust/src/bootstrap/tool.rs

   Line 80 in 11e002a

   let is_expected = compile::stream_cargo(builder, cargo, vec![], &mut |msg| {

   . Please also switch from stream_cargo to run_cargo at the same time to avoid duplicating code.
2. Change the Output for ToolBuild to be PathBuf instead of Option<PathBuf>.
3. Remove the is_optional_tool field from ToolBuild
4. Remove the missing_tools field from Config and Dist - it doesn't seem to do anything since 6cfa7ef.

[tharunsuresh-code]

I can take this up if no one else is working on it.

• If I understand correctly, changing to run_cargo requires changing the function arguments (Quoted above)? I am not sure if it is a trivial change here
• Also, changing Output to PathBuf requires a cascading change in the following functions in which it is called?
• There are no missing tools in Dist I believe.

I am kinda new here, so any help would be much appreciated.

[jyn514]

@tharunsuresh-code right, this change is a little non-trivial - I marked it as E-easy because the compiler errors should all point you in the right direction :) if you do the things in the mentoring steps and it compiles it should be 90% of the way there.

All the cascading changes you mentioned are things I would expect to change 👍 sounds like you're on the right track.

[tharunsuresh-code]

Got it, thanks! I will give it a shot.

[tharunsuresh-code]

In tool.rs, now that we are changing Output to PathBuf, .expect() does not work. Instead can we use panic! as shown below in ToolBuild function. Does this make sense, and remove all cascading expects?

if is_expected {
    // HACK(#82501): on Windows, the tools directory gets added to PATH when running tests, and
    // compiletest confuses HTML tidy with the in-tree tidy. Name the in-tree tidy something
    // different so the problem doesn't come up.
    if tool == "tidy" {
        tool = "rust-tidy";
    }
    let cargo_out = builder.cargo_out(compiler, self.mode, target).join(exe(tool, target));
    let bin = builder.tools_dir(compiler).join(exe(tool, target));
    builder.copy(&cargo_out, &bin);
    bin
} else {
    panic!("expected to build -- essential tool")
}

Or we can return an empty path above, check try_exists(), and panic at each appropriate function call?

[jyn514]

@tharunsuresh-code I think all tools should have is_expected set now, right? So we can remove both that variable and the else branch with the panic altogether.

We shouldn't run try_exists, if a Step returns a path without building anything that's a bug but there's too many paths to check it for every single one

[tharunsuresh-code]

Got it! So I have raised a pull request from my understanding of the required changes, please review and let me know what further things need to be modified. Specifically, I am not sure if the run_cargo function arguments are correct.

[tharunsuresh-code]

Somehow the tidy checks are failing at step "Building stage0 tool tidy". It throws an error like the filename is too long (partially shown below). I am still trying to get my head around the run_cargo function call from ToolBuild.

/media/d_drive/My_files/Visual_Studio_Code/github/rust_commit/rust/build/x86_64-unknown-linux-gnu/stage0-tools-bin/rust-tidy: line 1: h/media/d_drive/My_files/Visual_Studio_Code/github/rust_commit/rust/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/release/deps/libserde_derive-bf55f35e0f4b24b4.sot/...

[tharunsuresh-code]

So in run_cargo function, I figured that the stamp path is something temporary in which I was passing the cargo_out path resulting in it being overridden. So I made the changes below:

https://github.com/rust-lang/rust/blob/ae0e1454263fe64b64168895820cf68ec35ca01d/src/bootstrap/tool.rs#L87-L90

@jyn514 Could you please suggest what's the right way to assign the stamp path? And any other information would be much appreciated.

[RalfJung]

A previous attempt at implementing this failed since Miri did not build on all tier 2 targets. The problematic targets are no longer tier 2 though, so it might be worth trying this again.