diff --git a/Cargo.lock b/Cargo.lock index 65974cb4d5..bda18167f4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2820,6 +2820,7 @@ version = "0.9.0" dependencies = [ "cargo_metadata", "clap", + "log", "memchr", "notify", "raw-string", diff --git a/crates/spirv-builder/Cargo.toml b/crates/spirv-builder/Cargo.toml index f5c1bd2206..75a119cb28 100644 --- a/crates/spirv-builder/Cargo.toml +++ b/crates/spirv-builder/Cargo.toml @@ -51,3 +51,4 @@ cargo_metadata = "0.19.2" notify = { version = "7.0", optional = true } # Pinning clap, as newer versions have raised min rustc version without being marked a breaking change clap = { version = "=4.5.37", optional = true, features = ["derive"] } +log = { version = "0.4.22", features = ["std"] } diff --git a/crates/spirv-builder/src/cargo_cmd.rs b/crates/spirv-builder/src/cargo_cmd.rs new file mode 100644 index 0000000000..ba6e067577 --- /dev/null +++ b/crates/spirv-builder/src/cargo_cmd.rs @@ -0,0 +1,157 @@ +use std::collections::HashSet; +use std::env; +use std::ffi::{OsStr, OsString}; +use std::fmt::{Display, Formatter}; +use std::ops::{Deref, DerefMut}; +use std::process::Command; + +/// Filters the various env vars that a `cargo` child process would receive and reports back +/// what was inherited and what was removed. By default, removes all env vars that influences +/// the cargo invocations of a spirv-builder's build or cargo-gpu's install action. +pub struct CargoCmd { + cargo: Command, + vars_os: Vec<(OsString, OsString)>, + removed: HashSet, +} + +impl CargoCmd { + pub fn new() -> Self { + let mut cargo = CargoCmd::new_no_filtering(); + + // Clear Cargo environment variables that we don't want to leak into the + // inner invocation of Cargo (because e.g. build scripts might read them), + // before we set any of our own below. + cargo.retain_vars_os(|(key, _)| { + !key.to_str() + .is_some_and(|s| s.starts_with("CARGO_FEATURES_") || s.starts_with("CARGO_CFG_")) + }); + + // NOTE(eddyb) Cargo caches some information it got from `rustc` in + // `.rustc_info.json`, and assumes it only depends on the `rustc` binary, + // but in our case, `rustc_codegen_spirv` changes are also relevant, + // so we turn off that caching with an env var, just to avoid any issues. + cargo.env("CARGO_CACHE_RUSTC_INFO", "0"); + + // NOTE(firestar99) If you call SpirvBuilder in a build script, it will + // set `RUSTC` before calling it. And if we were to propagate it to our + // cargo invocation, it will take precedence over the `+toolchain` we + // previously set. + cargo.env_remove("RUSTC"); + + // NOTE(tuguzT) Used by Cargo to call executables of Clippy, Miri + // (and maybe other Cargo subcommands) instead of `rustc` + // which could affect its functionality and break the build process. + cargo.env_remove("RUSTC_WRAPPER"); + + // overwritten by spirv-builder anyway + cargo.env_remove("CARGO_ENCODED_RUSTFLAGS"); + + cargo + .env_remove("RUSTC") + .env_remove("RUSTC_WRAPPER") + .env_remove("RUSTC_WORKSPACE_WRAPPER") + .env_remove("RUSTFLAGS") + .env_remove("CARGO") + .env_remove("RUSTUP_TOOLCHAIN"); + + // ignore any externally supplied target dir + // spirv-builder: we overwrite it with `SpirvBuilder.target_dir_path` anyway + // cargo-gpu: we want to build it in our cache dir + cargo.env_remove("CARGO_TARGET_DIR"); + + cargo + } + + pub fn new_no_filtering() -> Self { + Self { + cargo: Command::new("cargo"), + vars_os: env::vars_os().collect(), + removed: HashSet::default(), + } + } + + pub fn retain_vars_os(&mut self, mut f: impl FnMut((&OsString, &OsString)) -> bool) { + for (key, value) in &self.vars_os { + if !f((key, value)) { + self.removed.insert(key.clone()); + self.cargo.env_remove(key); + } + } + } + + pub fn env_remove(&mut self, key: impl AsRef) -> &mut Self { + self.removed.insert(key.as_ref().to_os_string()); + self.cargo.env_remove(key); + self + } + + pub fn env(&mut self, key: impl AsRef, val: impl AsRef) -> &mut Self { + self.removed.remove(key.as_ref()); + self.cargo.env(key, val); + self + } + + pub fn env_var_report(&self) -> EnvVarReport { + let mut inherited = self.vars_os.clone(); + inherited.retain(|(key, _)| !self.removed.contains(key)); + EnvVarReport { + inherited, + removed: self.removed.clone(), + } + } +} + +impl Default for CargoCmd { + fn default() -> Self { + Self::new() + } +} + +impl Display for CargoCmd { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + f.debug_struct("CargoCmd") + .field("cargo", &self.cargo) + .field("env_vars", &self.env_var_report()) + .finish() + } +} + +impl Deref for CargoCmd { + type Target = Command; + + fn deref(&self) -> &Self::Target { + &self.cargo + } +} + +impl DerefMut for CargoCmd { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.cargo + } +} + +#[derive(Clone, Debug, Default)] +pub struct EnvVarReport { + pub inherited: Vec<(OsString, OsString)>, + pub removed: HashSet, +} + +impl Display for EnvVarReport { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + let removed = self + .removed + .iter() + .map(|key| format!("{}", key.to_string_lossy())) + .collect::>(); + let inherited = self + .inherited + .iter() + .map(|(key, value)| format!("{}: {}", key.to_string_lossy(), value.to_string_lossy())) + .collect::>(); + + f.debug_struct("EnvVarReport") + .field("removed", &removed) + .field("inherited", &inherited) + .finish() + } +} diff --git a/crates/spirv-builder/src/lib.rs b/crates/spirv-builder/src/lib.rs index 6161916fac..bb967eeedb 100644 --- a/crates/spirv-builder/src/lib.rs +++ b/crates/spirv-builder/src/lib.rs @@ -71,6 +71,7 @@ // #![allow()] #![doc = include_str!("../README.md")] +pub mod cargo_cmd; mod depfile; #[cfg(feature = "watch")] mod watch; @@ -961,7 +962,7 @@ fn invoke_rustc(builder: &SpirvBuilder) -> Result { let profile = if builder.release { "release" } else { "dev" }; - let mut cargo = Command::new("cargo"); + let mut cargo = cargo_cmd::CargoCmd::new(); if let Some(toolchain) = &builder.toolchain_overwrite { cargo.arg(format!("+{toolchain}")); } @@ -1014,30 +1015,6 @@ fn invoke_rustc(builder: &SpirvBuilder) -> Result { cargo.arg("--target-dir").arg(target_dir); - // Clear Cargo environment variables that we don't want to leak into the - // inner invocation of Cargo (because e.g. build scripts might read them), - // before we set any of our own below. - for (key, _) in env::vars_os() { - let remove = key - .to_str() - .is_some_and(|s| s.starts_with("CARGO_FEATURES_") || s.starts_with("CARGO_CFG_")); - if remove { - cargo.env_remove(key); - } - } - - // NOTE(eddyb) Cargo caches some information it got from `rustc` in - // `.rustc_info.json`, and assumes it only depends on the `rustc` binary, - // but in our case, `rustc_codegen_spirv` changes are also relevant, - // so we turn off that caching with an env var, just to avoid any issues. - cargo.env("CARGO_CACHE_RUSTC_INFO", "0"); - - // NOTE(firestar99) If you call SpirvBuilder in a build script, it will - // set `RUSTC` before calling it. And if we were to propagate it to our - // cargo invocation, it will take precedence over the `+toolchain` we - // previously set. - cargo.env_remove("RUSTC"); - // NOTE(eddyb) this used to be just `RUSTFLAGS` but at some point Cargo // added a separate environment variable using `\x1f` instead of spaces, // which allows us to have spaces within individual `rustc` flags. @@ -1046,21 +1023,18 @@ fn invoke_rustc(builder: &SpirvBuilder) -> Result { join_checking_for_separators(rustflags, "\x1f"), ); - let profile_in_env_var = profile.replace('-', "_").to_ascii_uppercase(); - // NOTE(eddyb) there's no parallelism to take advantage of multiple CGUs, // and inter-CGU duplication can be wasteful, so this forces 1 CGU for now. + let profile_in_env_var = profile.replace('-', "_").to_ascii_uppercase(); let num_cgus = 1; cargo.env( format!("CARGO_PROFILE_{profile_in_env_var}_CODEGEN_UNITS"), num_cgus.to_string(), ); - let build = cargo - .stderr(Stdio::inherit()) - .current_dir(path_to_crate) - .output() - .expect("failed to execute cargo build"); + cargo.stderr(Stdio::inherit()).current_dir(path_to_crate); + log::debug!("building shaders with `{cargo}`"); + let build = cargo.output().expect("failed to execute cargo build"); // `get_last_artifact` has the side-effect of printing invalid lines, so // we do that even in case of an error, to let through any useful messages diff --git a/tests/difftests/tests/Cargo.lock b/tests/difftests/tests/Cargo.lock index a8238f3fea..2a7b6b03f4 100644 --- a/tests/difftests/tests/Cargo.lock +++ b/tests/difftests/tests/Cargo.lock @@ -1148,6 +1148,7 @@ name = "spirv-builder" version = "0.9.0" dependencies = [ "cargo_metadata", + "log", "memchr", "raw-string", "rustc_codegen_spirv-types",