Send environment variables from client, hash CARGO_ env vars in Rust#92
Conversation
| executable: &str, | ||
| parsed_args: &ParsedArguments, | ||
| cwd: &str, | ||
| env_vars: &Vec<(String, String)>, |
There was a problem hiding this comment.
Perhaps &[(String, String)]?
There was a problem hiding this comment.
...yeah that makes more sense. Fixed.
| cmd.args(&arguments) | ||
| .args(&["--print", "file-names"]) | ||
| .env_clear() | ||
| .envs(env_vars.iter().map(|&(ref k, ref v)| (k, v))) |
There was a problem hiding this comment.
This seems like it's somewhat tricky to remember (clear the env before setting more env vars). I wonder if sccache could benefit from a Command look alike which allows introspection (we still need to add this to libstd...) to maybe package up arguments like the executable, cwd, env, and base arguments?
(just a random note for a future PR/refactoring)
There was a problem hiding this comment.
Yeah, that's a sensible idea. I actually went back and changed this to add envs on RunCommand to make it slightly less verbose, but encapsulating all this commonality seems like a good idea.
| // we'll just hash the CARGO_ env vars and hope that's sufficient. | ||
| // Upstream Rust issue tracking getting information about env! usage: | ||
| // https://github.com/rust-lang/rust/issues/40364 | ||
| for &(ref var, ref val) in env_vars.iter() { |
There was a problem hiding this comment.
I think we'll want to be sure to sort these before hashing, right?
There was a problem hiding this comment.
Oops, good catch! Might have even caught that with the unit test I added if I had used more than one env var...
| /// The commandline arguments passed to the compiler. | ||
| pub args: Vec<String>, | ||
| /// The environment variables present when the compiler was executed, as (var, val). | ||
| pub env_vars: Vec<(String, String)>, |
There was a problem hiding this comment.
With the new serde version I think this could also start to be a OsString as well
There was a problem hiding this comment.
I pushed the serde bump to master and then fell down this rabbit hole. The only thing that I didn't have a great solution for was adding OsStrings to the hash, so I wrote a crappy equivalent of OsStringExt::as_bytes for Windows that just mem::transmutes. I suspect it will not ever be a problem in practice (and I added a unit test that should fail if it ever breaks) but it sure feels wrong.
…ozilla#89 These variables are very commonly used in env! invocations, so include them in hash generation so we don't cache compilations improperly.
| let cached_env_vars: HashSet<OsString> = CACHED_ENV_VARS.iter().map(|v| OsStr::new(v).to_os_string()).collect(); | ||
| for &(ref var, ref val) in env_vars.iter() { | ||
| if cached_env_vars.contains(var) { | ||
| m.update(os_str_bytes(var)); |
There was a problem hiding this comment.
This runs a risk of breaking in future rust versions, but could Hash for OsStr be used here instead?
…s-workflow feat/ci/action: add gh-pages deployment job
2 participants
This fixes a longstanding issue about not using the client's environment for compilation, and then uses that fix to include CARGO_ env vars in Rust hash generation.