tool::prepare_tool_cargo should run builder.ensure automatically, based on tool mode

[tgross35]

I was somewhat thrown off by 6062059#diff-675065335a90de4a4e1ac5bc0a078709f865ff2d357ba2650309f0ad8614b1d3R3531-R3549; setting Mode::ToolStd or some of the other Mode::Tool* options caused a can't find crate for `core` (and std) error. This was fixed by adding builder.ensure(compile::Std::new(compiler, bootstrap_host));.

Since prepare_tool_cargo knows the mode, it should just call builder.ensure itself. This manual ensure could then be removed from the rest of build_steps/test.rs.

[onur-ozkan]

Tools test steps usually ensure their corresponding step in tool module and that is usually managed by ToolBuild step. When you look at ToolBuild::run, you can see that it handles this problem:

rust/src/bootstrap/src/core/build_steps/tool.rs

Lines 71 to 89 in 2e6fc42

	/// Builds a tool in `src/tools`
	///
	/// This will build the specified tool with the specified `host` compiler in
	/// `stage` into the normal cargo output directory.
	fn run(self, builder: &Builder<'_>) -> PathBuf {
	let compiler = self.compiler;
	let target = self.target;
	let mut tool = self.tool;
	let path = self.path;
	match self.mode {
	Mode::ToolRustc => {
	builder.ensure(compile::Std::new(compiler, compiler.host));
	builder.ensure(compile::Rustc::new(compiler, target));
	}
	Mode::ToolStd => builder.ensure(compile::Std::new(compiler, target)),
	Mode::ToolBootstrap => {} // uses downloaded stage0 compiler libs
	_ => panic!("unexpected Mode for tool build"),
	}

So, if you add this new tool under tool module with ToolBuild, this problem will be handled automatically.

[tgross35]

I opened this because a few other tools seem to call builder.ensure(compile::Std::new(...)) right before tool::prepare_tool_cargo (Miri, CompiletestTest, RunMakeSupport, CrateRustdoc, TierCheck) so it seemed like a common pattern.

Are you saying that the new TestFloatParse could just be a ToolBuild step? I think I looked at that first but since it needs to both cargo test and cargo run with some args, it seemed like it was easier to just add a new step (I could easily have missed something here).

[onur-ozkan]

I think the right pattern should be having a specific build step in tool module (so we can also build it with x build), then ensure that in the test step.

[tgross35]

Alright, it ideally would also have a x run but that isn't implemented. I don't want to touch that PR now since it's pretty large and already in the queue, but I'll send a follow up after it merges and r? you.

[lucarlig]

@tgross35 still working on this?

[tgross35]

I guess this slipped my mind. All yours @lucarlig if you are interested, just comment @rustbot claim and it will assign you.

[lucarlig]

will try it out,
@rustbot claim

[jieyouxu]

I opened this because a few other tools seem to call builder.ensure(compile::Std::new(...)) right before tool::prepare_tool_cargo (Miri, CompiletestTest, RunMakeSupport, CrateRustdoc, TierCheck) so it seemed like a common pattern.

The RunMakeSupport one looks wrong to me now in that that ensure on std shouldn't be there. At the time I implemented that, I had no idea what I was doing, I just copied whatever code was around it lol.

[lucarlig]

I think this issue is not needed @onur-ozkan based on #131855

I opened this because a few other tools seem to call builder.ensure(compile::Std::new(...)) right before tool::prepare_tool_cargo (Miri, CompiletestTest, RunMakeSupport, CrateRustdoc, TierCheck) so it seemed like a common pattern.

The RunMakeSupport one looks wrong to me now in that that ensure on std shouldn't be there. At the time I implemented that, I had no idea what I was doing, I just copied whatever code was around it lol.

regarding this maybe is worth opening a new issue @jieyouxu ? Or can you explain exactly what I need to do, I can add it to the PR.
regarding TestFloatParse pr is there already #131731

[jieyouxu]

For specific ones that might have special handling yes, otherwise I believe the tool builds already do the std handling. Those test steps seem to me like they have subtly different needs so I don't think they want to or can be trivially unified.

[jieyouxu]

The RunMakeSupport one looks wrong to me now in that that ensure on std shouldn't be there. At the time I implemented that, I had no idea what I was doing, I just copied whatever code was around it lol.

Hm actually no, I read it again, the RunMakeSupport is a test step and has special handling, nevermind.