Warn when installing with a non-default toolchain

[smoelius]

This PR implements the cargo changes for #11036. The rustup changes were implemented in rust-lang/rustup#4518.

The PR is currently organized as four commits:

• The first expands the rustup tests to set RUSTUP_TOOLCHAIN_SOURCE. This change is not strictly necessary, but it improves the rustup tests' fidelity.
• The second moves the pkg function to a location where the next commit can use it.
• The third implements a test to check the fourth commit's fix.
• The fourth warns when cargo install is invoked with a non-default toolchain, as indicated by RUSTUP_TOOLCHAIN_SOURCE.

In the third commit, two significant changes were made to simulated_rustup_environment:

• Previously, the constructed proxy would call an always-panicking executable whenever it was asked to run cargo. Now, the proxy can be made to run the cargo under test.
• The proxy can now be made to set additional environment variables (e.g., RUSTUP_TOOLCHAIN_SOURCE) before calling the proxied tool.

The PR is currently marked as draft because rust-lang/rustup#4518 has not yet been released. Technically, this PR could be merged before then. But testing it with a fixed rustup would seem to make sense.

Nits are welcome.

cc: @epage

[rustbot]

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[weihanglo]

EDIT: The force push was to make Clippy happy.

Feel free to force-push and re-organize your commits during the iterations.

[epage]

Downside to matches! is we can miss it if new values are added. For now, that is not likely to happen as this is all for one feature. That might not always be the case.

I would

• Move the &str -> RustupToolchainSource to a function on get_rustup_toolchain_source (shouldn't touch gctx)
• Have a function on RustupToolchainSource that lts is ask the question we are asking here and uses a match with every case enumerated

[smoelius]

I don't think I follow what you're asking for.

Could you give me some pseudo-code? Or could you point to a function that uses this pattern?

[epage]

Move the &str -> RustupToolchainSource to a function on get_rustup_toolchain_source (shouldn't touch gctx)

That has happened

Have a function on RustupToolchainSource that lts is ask the question we are asking here and uses a match with every case enumerated

impl RustupToolchainSource {
    fn is_current_dir_override(&self) -> bool {
        match self {
        }
    }
}

(name is not prescriptive, just the first thing I could think of for what might describe what we care about)

[epage]

Note that this is still not resolved. My request is to use an exhaustive match for where we determine the policy for when we care and suggested that can be split out into a function.

The current state of the PR is is_<variant> methods are added and are being used like the match! that was here before.

[smoelius]

My request is to use an exhaustive match for where we determine the policy for when we care and suggested that can be split out into a function.

Are you asking for something like this?

impl RustupToolchainSource {
    fn is_current_dir_override(&self) -> bool {
        match self {
            Self::Default => false,
            Self::Environment => true,
            Self::CommandLine => false,
            Self::OverrideDB => true,
            Self::ToolchainFile => true,
            Self::Other(other) => true,
        }
    }
}

[epage]

Yes, though Self::Other is new since my original framing. The question is what should we do in that case. My guess is the decision should be owned by the caller of this function, so returning Option<bool> and we should likely skip warning the user to avoid false positives. Maybe we could log a message in the Other case.

[smoelius]

Just FYI, I know it looks like only some of the comments have been addressed. I am still hoping to get feedback on #16131 (comment) and #16131 (comment).

[smoelius]

I resolved comments that I thought I implemented uncontroversially. I hope that was okay.

EDIT: I also hid a few of my own comments that were old.

[epage]

Please clean up your commits for how they should be reviewed and merged, rather than for how it was developed.

[weihanglo]

nit: Can we have some explanation in the commit message, so the commit standalone tells why we need this? It is hard to tell whether this is a refactor or a bugfix for the testsuite.

[smoelius]

Are you asking for one comment per commit? Or one comment describing the entire PR in one of the commits? Either way, I am happy to oblige.

[weihanglo]

I reviewed the code commit by commit, so was specifically asking the first commit. Not a blocker though. Thanks!

[smoelius]

I added a comment describing the PR to the first commit. Please tell me if it is unclear or insufficient.

[weihanglo]

Instead of passing boolean we can make the argument impl snapbox::IntoData so that every test has its own snapshot. What do you think?

[smoelius]

What would the impl snapbox::IntoData be? Could you give me an example?

[epage]

He's asking to pass in stderr rather than creating it within the test

[epage]

For more context:

• it makes the test easier to evaluate what it does
• we prefer using snapshots (str![]), rather than hard coded values, wherever possible so we can easily update all tests when unrelated formatting changes

[epage]

Looks like this is still outstanding

[smoelius]

I could easily change cargo_install_with_toolchain_source's second argument from &str to impl snapbox::IntoData. But I sense that more is needed?

[epage]

The signature should be changed to that and a str![] should be passed in, rather than calling warning_with_optional_rust_toolchain_source

[weihanglo]

As far as I know, rustup hasn't yet claimed those values are stable. At least the doc doesn't say anything more than the environment key https://github.com/rust-lang/rustup/blob/de2b979bd4569cacfabf501bb58464c9139b0d6d/doc/user-guide/src/environment-variables.md?plain=1#L75.

• Could we have a link to the rustup source code if we want to make an assumption on that?
• Should we check with the rustup team to ensure these value strings are stable?

[smoelius]

I am happy to get confirmation from the Rustup team that the five values are stable, but would you feel more comfortable if they were listed directly in the documentation?

[weihanglo]

Yeah, thanks. But actually if we have #16131 (comment) maybe it is not really necessary?

[epage]

Similar to REAL_RUSTUP_HOME, we shouldn't need a REAL_RUSTC because RUSTC (if it is set which it isn't guaranteed to be) is not cleared within the process that can also read REAL_RUSTC

[epage]

Oh, cargo sets it on the build script and we are forwarding it along.

[epage]

While a lot simpler of a solution, I'm not too thrilled with this because

• say we could get rid of our build script for production reasons (which would be ideal), we'd then have production builds running a build script for tests
• we have to use env! which I assume makes caching more difficult because it has an absolute path in it that can change, particularly for CI

[epage]

@weihanglo your thoughts?

[weihanglo]

The cache issue might be a deal-breaker. Thanks epage for calling that out.

[smoelius]

Should I switch back to the earlier approach then? The one that inspired #16131 (comment)?

[epage]

Something like the original.

The original code is written in an odd way (why remove env variables when they aren't present?) that is further compounded with the change to remove RUSTUP_* env variables in this PR.

I would move the RUSTUP_* env variable changes to the start of this PR and remove the env_remove calls in that commit as they are redundant. Feel free to even split that out into a separate PR so we can more quickly review and merge that while other parts of this are still under discussion as those changes stand on their own.

[smoelius]

Feel free to even split that out into a separate PR...

It's #16469, but it's currently failing.

Regardless, should I remove the Clear RUSTUP_* environment variables in test_env commit from this PR and proceed with everything else?

[epage]

Add RustupEnvironmentBuilder

When doing a refactor commit, particularly when the diff is more difficult to read (indentation level, moving), please separate out any logical changes into a separate commit

[epage]

Set RUSTUP_TOOLCHAIN_SOURCE in rustup tests

Why is this needed?

We need to be able to work with old rustup that doesn't set this, so it is probably good for us to have some existing tests that leave it unset

[smoelius]

I would be happy to get rid of the commit, but I don't know how to act on the "tests that leave it [RUSTUP_TOOLCHAIN_SOURCE] unset" comment.

Could you give me more guidance on tests would like to have once Rustup 1.29 is published?

[epage]

Flip this around, why do we need this commit?

Cargo works with old and new rustup, so these shoudn't be required from the perspective of "need to mirror rustup exactly. cc2855c will clear RUSTUP_TOOLCHAIN_SOURCE so we don't need to worry about cargo test when cargo comes through the rustup proxy setting RUSTUP_TOOLCHAIN_SOURCE, causing the tests to be confused.

[smoelius]

I added the .env("RUSTUP_TOOLCHAIN_SOURCE", ..) to simluate rustup's behavior, because I thought that was what the existing .env("RUSTUP_TOOLCHAIN", ..) just below them were meant to do.

But I appreciate what you are saying.

Should I just remove this commit then?

[epage]

I lean towards removing it

[smoelius]

I'm still cleaning up commits. Thanks for your patience.

[smoelius]

I think the PR is ready to be looked at again.

[epage]

Why is this needed to be set on every cargo call?
(same for CargoProjectExt)

(thanks for splitting this out as it makes it easier to not miss details like this)

[smoelius]

Not setting RUSTUP_HOME in cargo_process causes the following tests to fail:

• install::multiple_pkgs_path_set
• install::relative_install_location_with_path_set

Not setting RUSTUP_HOME in CargoProjectExt causes the following tests to fail:

• cargo_alias_config::alias_shadowing_external_subcommand
• cargo_alias_config::builtin_alias_shadowing_external_subcommand
• jobserver::runner_inherits_jobserver

Would you rather that the individual tests be modified?

[epage]

Would you rather that the individual tests be modified?

Yes. In fact, I consider these tests failing suspect and should be evaluated. By setting RUSTUP_HOME explicitly, we at least isolate and identify these. We can then follow up by evaluating and adjusting as needed.

[epage]

non-blocking (particularly with how late I thought of this): we should use expect and reason. I posted #16461 to migrate some cases over. The reason for this is "consistency with rustup".