fix: make subcommands pick up [env]

[lovesegfault]

What does this PR try to resolve?

Fixes #10094

This also aims to address some of the issues pointed out in the issue, namely:

This seems reasonable; we should set the environment variables for third-party subcommands too.

This happens by default now. Users can opt-out by setting apply_to_subcommands = false for the var.

What happens if you set PATH? Does that affect finding the subcommands?

TODO

What happens if you set CARGO_HOME? Do we read .cargo/config.toml from the new location?
What happens if you set configuration environment variables?

Setting any CARGO_ var from [env] is now forbidden, and will result in an
error. I'm not sure we want something so strict, so it probably warrants further
discussion.

How should we test and review this PR?

I added tests env_external_subcommands and env_no_cargo_vars.

Additional information

I didn't add any special handling of PATH, which means currently it will not
cause the subcommand to not be found, even if the specified PATH doesn't
contain the subcommand.

I think this could lead to surprising behavior in a scenario like this:

1. User has cargo-foobar in their PATH
2. cargo-foobar relies on foobar-cli being in PATH
3. The user has foobar-cli in their PATH
4. They override PATH in [env] to one containing neither cargo-foobar nor
   foobar-cli
5. They can invoke cargo-foobar, but it will fail due to foobar-cli not bein
   in its PATH

Some options to solve this are:

1. We can make the vars apply prior to finding the subcommand, which should make
   cargo-foobar to not be found, which is a bit less surprising
2. We make apply_to_subcommands default to false (the current behavior)

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

[jonhoo]

Users can opt-out by setting apply_to_subcommands = false for the var.

bikeshedding: I don't love the name as it's very verbose compared to most Cargo config variables. How about in_subcommands?

• We can make the vars apply prior to finding the subcommand, which should make
  cargo-foobar to not be found, which is a bit less surprising

I think we should do this — it's very weird for a PATH set with apply_to_subcommands = true to not also be used to find the subcommand in the first place.

• We make apply_to_subcommands default to false (the current behavior)

I think that needs to be the case anyway, since we have to preserve backwards-compatibility here.

[epage]

This also aims to address some of the issues pointed out in the issue, namely:

While we understand and appreciate the work that has gone into this, we ask that design discussions happen on Issues first and implementations to wait on the issue being marked as "Accepted'. Could we move this discussion over to the Issue?

In the mean time, I'm going to close this as there are still open issues on the design that need settling.

From CONTRIBUTING.md

We encourage people to discuss their design before hacking on code. Typically, you file an issue or start a thread on the internals forum before submitting a pull request

From Process

Due to limited review capacity, the Cargo team is not accepting new features or major changes at this time. Please consult with the team before opening a new PR. Only issues that have been explicitly marked as accepted will be reviewed.

[jonhoo]

@epage I'm a little confused — this follows the suggestions from @joshtriplett in #10094 (comment). I suppose we can take the open questions over there for discussion if that's preferred, though I didn't actually see any disagreement there, so it feels as though these are less open questions and more "we'll need to pick a path and stick with it", with basic consensus on what that path should be.

[lovesegfault]

@epage could you chime in on what I should do next to get this fixed?

There is already an issue for this, and there's already been a discussion on how to solve it, with no one disagreeing. I implemented what was proposed there.

What do you think needs settling before I submit this PR?

[joshtriplett]

Clarifying something: we had discussions on 10094, and I suggested an approach that I thought would work. This PR, correctly, raised a few additional questions and open issues, as well as design possibilities that we need to decide between. For instance, apply_to_subcommands, or the strictness about CARGO_, and the issue of PATH.

From experience, the Cargo team has found that doing some discussion directly on a PR has been harder and higher-bandwidth for us to deal with than getting the discussion done first and then having a PR that implements the conclusion. That way, when we're reviewing the PR, we just need to worry about the implementation, and we don't have to simultaneously review design proposals in code form and not just prose form.

Closing this PR is not an indication that we don't want to see this change; on the contrary, we definitely want to see things finished and shipped. Rather, we'd just like to direct the design discussion back to the issue, deal with the open design questions, and once we have a settled design we'd be happy to see a PR.

[jonhoo]

@joshtriplett That makes sense, thanks for the clarification! Will follow up in the issue 👍

[epage]

Sorry for the delay in getting a response back to the confusion over my post. I had just left on a road trip and wifi didn't work at any of our hotels. Thanks @joshtriplett for clarifying things in my absence!

[jonhoo]

All good — a road trip without WiFi sounds wonderful!