Avoid directly reading environment variables

[ehuss]

Cargo has a snapshot of the environment in Config.env. This environment snapshot is primarily used for loading config environment variables.

However, other environment variables (like RUSTFLAGS) do not go through the snapshot. In an effort to try to have a unified location where environment variables are accessed, we would like to consider updating the codebase so that all environment variables are read through the config snapshot.

One would need to scour the codebase for calls to the std::env functions (var, var_os, vars, etc.). The scope of this is uncertain, since there may be some places where it is difficult to access the Config object. This may require some exploration to evaluate the impact.

As for the API, I would consider renaming the existing get_env to something else (like get_config_env), and then adding some public methods like get_env or get_env_os.

[kylematsuda]

Hi! Can I help out with this?

I played around a bit in src/cargo and found a lot of places where this looks easy to change (Config is available or can be added without too much hassle). Also some other spots that seem harder to deal with.

If it'd be helpful, I'll open a PR with what I have so far as a start.

[kylematsuda]

Oops sorry, could you reopen this? I shouldn't have said "Fixes" in my PR, as there are still several places that weren't handled by #11727.

[kylematsuda]

I just opened another PR with a few call sites that I missed in #11727 but are hopefully straightforward.

After that, there are still a handful of calls to env::var, which fall into three main categories:

1. Before Config is constructed. This happens in src/bin/cargo/cli.rs and core::features::channel. One option is to make a dummy object (maybe called PreConfigEnv or something) that just wraps calls to env::var(_os). Maybe this would make things a little more explicit and easier to keep track of in the future, though it does add a little clutter.
2. Test-related stuff. There are few places where it seems like the call to env::var is being used only for tests. For example:
   

   cargo/src/cargo/core/source/source_id.rs

   Lines 416 to 426 in b6c7fbe

   	/// Returns `true` if the remote registry is the standard <https://crates.io>.
   	pub fn is_crates_io(self) -> bool {
   	match self.inner.kind {
   	SourceKind::Registry | SourceKind::SparseRegistry => {}
   	_ => return false,
   	}
   	let url = self.inner.url.as_str();
   	url == CRATES_IO_INDEX
   	|| url == CRATES_IO_HTTP_INDEX
   	|| std::env::var("__CARGO_TEST_CRATES_IO_URL_DO_NOT_USE_THIS").as_deref() == Ok(url)
   	}

   
   This method can't easily be made to take &Config as a parameter since it ends up getting called in the Display impl.
   There's a similar thing in shell::TtyWidth, and resolver::types::ResolverProgress (though the latter is behind a #[cfg(debug_assertions)]). Not sure if the best thing here would be to try to factor these out somehow, gate them behind a #[cfg(..)], or just leave them.
3. Issues because Config isn't Sync + Send + 'static. There are a few places (like 2 or 3) where environment variables are read inside of a closure that has these bounds. One way to fix this could be having config.env hold a newtype struct Env(HashMap<OsString, OsString>) and have config.env() return an &Env. Env can implement get_env and get_env_os and will be Sync + Send. (As a side benefit, this might also be a cleaner way to encapsulate all of the valid UTF-8 checks that are currently done by Config.)

Thoughts on these? (I'm also fine just leaving them, since there's not too many remaining now after #11727.)

[ehuss]

Sure, sorry, I didn't realize there were more updates.

[kylematsuda]

Thanks!

[weihanglo]

expand to see the original comment

Some variables read from snapshot starting from #11727 may need a double check:

• The meaning of RUSTUP_HOME RUSTUP_TOOLCHAIN is not clear to me. Will user get a inconsistent view of rustc/cargo invocations because of that overridden?
• __CARGO_DEFAULT_LIB_METADATA is for Rust releases, not expected to be read via config by users.
• __CARGO_FIX_DIAGNOSTICS_SERVER is for internal communication, not expected to be read from config by users.
• cargo fix related variables are not intended to be read from config I believe.
  • __CARGO_FIX_BROKEN_CODE
  • __CARGO_FIX_YOLO
  • CARGO_FIX_MAX_RETRIES (fix: Iteratively apply suggestions from the compiler #5842 (comment))
    (maybe it should be __CARGO_FIX_MAX_RETRIES or just a constant?)
• Internal test only can read from env directly
  • __CARGO_TEST_INTERNAL_ERROR
  • __CARGO_TESTS_ONLY_SRC_ROOT
  • __CARGO_TEST_CRATES_IO_URL_DO_NOT_USE_THIS
  • __CARGO_TEST_DISABLE_GLOBAL_KNOWN_HOST
  • __CARGO_PACKFILE_LIMIT
  • __CARGO_TEST_MAX_UNPACK_SIZE
  • __CARGO_TEST_MAX_UNPACK_RATIO

Not really relevant anymore. See the discussion.

[weihanglo]

It seems to be fixed by #11727, #11754, and #11828. Going to close out this. If you have any update or I misunderstand the current status, please call it out and we can re-open. Thank you.