Bugs in "component add" message

[smoelius]

I think there may be a couple of bugs here:

rustup/src/toolchain/distributable.rs

Lines 441 to 445 in 6e8769c

	let selector = match self.cfg.get_default()? {
	Some(ToolchainName::Official(n)) if n == self.desc => "",
	_ => " --toolchain {toolchain}",
	};
	Err(anyhow!("'{binary_lossy}' is not installed for the toolchain '{desc}'.\nTo install, run `rustup component add {selector}{component_name}`"))

• selector is not part of the format string, so {toolchain} won't get filled in.
• There appears to be a space missing between {selector} and {component} name.

[rami3l]

@smoelius Thanks a lot for filing this issue! Would you be so kind as to make a PR fixing this one?

[smoelius]

Fixing the bug itself looks not so hard.

Is there a test I could copy and modify to verify the fix? Or should I submit the fix without a test?

[rami3l]

Is there a test I could copy and modify to verify the fix? Or should I submit the fix without a test?

@smoelius It depends on how you may reproduce this issue, and how stable that reproduction would be.

I guess it can be stably reproduced when you do rls +nightly --version. If that's correct, a quick search points to

rustup/tests/suite/cli_misc.rs

Lines 529 to 542 in 6e8769c

	#[test]
	fn run_rls_when_not_installed() {
	setup(&|config| {
	config.expect_ok(&["rustup", "default", "stable"]);
	config.expect_err(
	&["rls", "--version"],
	&format!(
	"'rls{}' is not installed for the toolchain 'stable-{}'.\nTo install, run `rustup component add rls`",
	EXE_SUFFIX,
	this_host_triple(),
	),
	);
	});
	}

[smoelius]

In a fork at 6e8769c with no changes, cargo test --features=test cli_self_upd::update_exact produces this:

> rustup self update
out.ok: true
out.stdout (2 lines):
      rustup updated - 6e8769c76 (from 1.27.0)
    
    
out.stderr (2 lines):
    info: checking for self-update
    info: downloading self-update
    
expected.ok: true
expected.stdout (2 lines):
      rustup updated - 1.27.0 (from 1.27.0)
    
    
expected.stderr (2 lines):
    info: checking for self-update
    info: downloading self-update
    
[src/test/mock/clitools.rs:601:13] out.stdout == stdout = false
[src/test/mock/clitools.rs:602:13] out.stderr == stderr = true
thread 'suite::cli_self_upd::update_exact' panicked at tests/suite/cli_self_upd.rs:303:16:

The same command passes when run on the rustup repo.

Any idea what could be wrong?

[rami3l]

Any idea what could be wrong?

@smoelius This is #1783.

I think you can safely ignore this test locally (in fact, I recommend running only the tests you added, and probably other relevant tests such as this run_rls_when_not_installed() thing, and leaving the rest to the CI). It depends on the git state (more specifically, git depth), as you can probably see right now. As long as everything is green on our own CI it should be fine.

Context: https://rust-lang.github.io/rustup/dev-guide/version-numbers.html

[smoelius]

Can I ask what would be considered the "right" way to fix the update_exact test?

[rami3l]

Can I ask what would be considered the "right" way to fix the update_exact test?

@smoelius As I explained above, do a full clone of the repo with all the tags and this test will pass locally. It's just that it's not GitHub's default currently...