cargo subcommands do not pick up [env]

[jonhoo]

Problem

The [env] configuration section (#9539) does not currently impact subcommand invocations. It's not entirely clear whether this is a bug or by design. My instinct is that it's the former, since otherwise setting, say, PATH in ~/.cargo/config.toml won't affect which subcommands are available, but it could also be argued that it should be up to subcommands whether they want to pick up the current configuration (e.g., they may to reload the config rooted somewhere else, which would mean not picking up [env] from ./.cargo/config.toml).

If this is a bug, it should be fixed.

If it's by design, the documentation should be updated to make it clear that these environment variables do not affect subcommands.

Steps

echo '#!/bin/bash' > ~/.cargo/bin/cargo-issue-10094
echo 'env' >> ~/.cargo/bin/cargo-issue-10094
chmod +x ~/.cargo/bin/cargo-issue-10094

rm -rf cargo-issue-10094-repro
cargo new cargo-issue-10094-repro
pushd cargo-issue-10094-repro

mkdir .cargo
echo '[env]' > .cargo/config.toml
echo 'CARGO_ISSUE_10094 = "1"' >> .cargo/config.toml

env=$(cargo issue-10094)
popd

rm -r cargo-issue-10094-repro
rm ~/.cargo/bin/cargo-issue-10094

# Notice that this produces no output
echo "$env" | grep CARGO_ISSUE_10094

Possible Solution(s)

The code here

cargo/src/bin/cargo/main.rs

Lines 171 to 174 in ad50d0d

	let err = match ProcessBuilder::new(&command)
	.env(cargo::CARGO_ENV, cargo_exe)
	.args(args)
	.exec_replace()

needs to pick up environment variables from the config in the same way as is done for compilation invocations in

cargo/src/cargo/core/compiler/compilation.rs

Lines 348 to 358 in ad50d0d

	// Apply any environment variables from the config
	for (key, value) in self.config.env_config()?.iter() {
	// never override a value that has already been set by cargo
	if cmd.get_envs().contains_key(key) {
	continue;
	}
	if value.is_force() || env::var_os(key).is_none() {
	cmd.env(key, value.resolve(self.config));
	}
	}

Or, if the decision is to update the documentation instead, this paragraph should be updated

cargo/src/doc/src/reference/config.md

Lines 485 to 486 in ad50d0d

	The `[env]` section allows you to set additional environment variables for
	build scripts, rustc invocations, `cargo run` and `cargo build`.

Notes

No response

Version

cargo 1.56.0 (4ed5d137b 2021-10-04)
release: 1.56.0
commit-hash: 4ed5d137baff5eccf1bae5a7b2ae4b57efad4a7d
commit-date: 2021-10-04

[joshtriplett]

A couple of notes from today's Cargo meeting:

• This seems reasonable; we should set the environment variables for third-party subcommands too.
• What happens if you set PATH? Does that affect finding the subcommands?
• 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?

We should definitely fix this, but in doing so, we should address corner cases like those.

Personally, I would propose that setting PATH should work, setting CARGO_HOME should not (to avoid dealing with recursion), setting configuration environment variables should not (since people can just as easily set those elsewhere in the same .cargo/config.toml), and overriding environment variables cargo sets for crates should not work.

[jonhoo]

I agree on all points.

For those last two, I would actually suggest that those should never work, not just for subcommands, and that those cases should error out in the code that reads the environment variables from the config in the first place.

[joshtriplett]

@jonhoo Right, those were meant to be things that should always be true about [env], not just things that should be true for subcommands.

[lovesegfault]

Picking up the discussion from #10780 here.

My implementation: master...lovesegfault:cargo:fix-10094

Regarding the questions posed by @joshtriplett:

• we should set the environment variables for third-party subcommands too.

  • This is now the case, the implementation is here. Note, however, that I did not change the default behavior. See note below.

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

  • You can set PATH, but it will affect finding subcommands. You can see how that is done here. So, if you set PATH to something that doesn't contain cargo-foo, invoking cargo foo won't work. There are tests that help elucidate the behavior here.

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

  • Setting any CARGO_ vars in [env] is now forbidden. I think it's just too difficult to make sure they all work without making Cargo recursive, which I suspect we don't want to do.

• What happens if you set configuration environment variables?

  • They're forbidden. At least the CARGO_ ones, I wasn't certain whether we wanted to forbid any other prefixes or specific vars.

One remaining question is what should the default behavior be, we have two options:

1. Apply [env] to subcommands, allow opting-out by setting in_subcommands = false.

• This is less surprising, and arguably more correct, but would be a breaking change.

2. Don't apply [env] to subcommands, allow opting-in by setting in_subcommands = true.

• This is the current behavior, and it is surprising, but would let us get away without a breaking change.

In my current PR, I took the route of (2). Changing it to (1) is trivial, all I need is to flip this value.

Which option should we take?

[jonhoo]

There's also the smaller question of what the option for retaining the old behavior should be named. And arguably, if we're willing to make a breaking change (so option 1), whether there even should be an option for retaining the current behavior. I happen to have a specific use-case where I explicitly want the current behavior, so would strongly prefer the option to be included regardless of whether we change the default or not.

As for naming, I like in-subcommands. Some other suggestions for bikeshedding:

• subcommands
• include-subcommands
• applies-to: cargo | subcommands | ["cargo", "subcommands"]

[jonhoo]

Another note for later: we'll want to make sure that cargo --list and cargo --list --verbose also pick up on [env.PATH] if set for the purposes of discovering subcommands.

[ehuss]

We had a pretty lengthy discussion about this issue today, but unfortunately didn't come to a direct conclusion.

One thing we would like to see is an overview of alternatives so that we can consider the merits of setting the environment for external subcommands versus other solutions.

One concern is that if the environment was set, external subcommands would then begin behaving differently than cargo itself. Cargo doesn't read the [env] table for its own environment (see #11589). If the [env] was set for subcommands, they would then see a different environment than cargo itself. If cargo were to start reading the [env] table for itself, then it would probably make a lot more sense for it to set it for external subcommands.

An alternative is to make it easier for external subcommands to access cargo's config. If that were easier, then external subcommands can decide if they want the [env] table or not by reading it themselves. (That wouldn't address the PATH issue, though.)

Another idea is to somehow make this an opt-in behavior for external subcommands.

Perhaps others could share some comments if there are other considerations or ideas?