Skip to content

Commit ea47a69

Browse files
committed
add CargoCmd handling env vars centrally with reporting utilities
1 parent b11f13b commit ea47a69

File tree

5 files changed

+154
-37
lines changed

5 files changed

+154
-37
lines changed

Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

crates/spirv-builder/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,3 +51,4 @@ cargo_metadata = "0.19.2"
5151
notify = { version = "7.0", optional = true }
5252
# Pinning clap, as newer versions have raised min rustc version without being marked a breaking change
5353
clap = { version = "=4.5.37", optional = true, features = ["derive"] }
54+
log = { version = "0.4.22", features = ["std"] }

crates/spirv-builder/src/cargo_cmd.rs

Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,145 @@
1+
use std::collections::HashSet;
2+
use std::env;
3+
use std::ffi::{OsStr, OsString};
4+
use std::fmt::{Display, Formatter};
5+
use std::ops::{Deref, DerefMut};
6+
use std::process::Command;
7+
8+
/// Filters the various env vars that a `cargo` child process would receive and reports back
9+
/// what was inherited and what was removed. By default, removes all env vars that influences
10+
/// the cargo invocations of a spirv-builder's build or cargo-gpu's install action.
11+
pub struct CargoCmd {
12+
cargo: Command,
13+
vars_os: Vec<(OsString, OsString)>,
14+
removed: HashSet<OsString>,
15+
}
16+
17+
impl CargoCmd {
18+
pub fn new() -> Self {
19+
let mut cargo = CargoCmd::new_no_filtering();
20+
21+
// Clear Cargo environment variables that we don't want to leak into the
22+
// inner invocation of Cargo (because e.g. build scripts might read them),
23+
// before we set any of our own below.
24+
cargo.retain_vars_os(|(key, _)| {
25+
!key.to_str()
26+
.is_some_and(|s| s.starts_with("CARGO_FEATURES_") || s.starts_with("CARGO_CFG_"))
27+
});
28+
29+
// NOTE(eddyb) Cargo caches some information it got from `rustc` in
30+
// `.rustc_info.json`, and assumes it only depends on the `rustc` binary,
31+
// but in our case, `rustc_codegen_spirv` changes are also relevant,
32+
// so we turn off that caching with an env var, just to avoid any issues.
33+
cargo.env("CARGO_CACHE_RUSTC_INFO", "0");
34+
35+
// NOTE(firestar99) If you call SpirvBuilder in a build script, it will
36+
// set `RUSTC` before calling it. And if we were to propagate it to our
37+
// cargo invocation, it will take precedence over the `+toolchain` we
38+
// previously set.
39+
cargo.env_remove("RUSTC");
40+
41+
// NOTE(tuguzT) Used by Cargo to call executables of Clippy, Miri
42+
// (and maybe other Cargo subcommands) instead of `rustc`
43+
// which could affect its functionality and break the build process.
44+
cargo.env_remove("RUSTC_WRAPPER");
45+
46+
cargo.env_remove("CARGO_ENCODED_RUSTFLAGS");
47+
48+
cargo
49+
.env_remove("RUSTC")
50+
.env_remove("RUSTC_WRAPPER")
51+
.env_remove("RUSTC_WORKSPACE_WRAPPER")
52+
.env_remove("RUSTFLAGS")
53+
.env_remove("CARGO")
54+
.env_remove("RUSTUP_TOOLCHAIN");
55+
56+
cargo
57+
}
58+
59+
pub fn new_no_filtering() -> Self {
60+
Self {
61+
cargo: Command::new("cargo"),
62+
vars_os: env::vars_os().collect(),
63+
removed: HashSet::default(),
64+
}
65+
}
66+
67+
pub fn retain_vars_os(&mut self, mut f: impl FnMut((&OsString, &OsString)) -> bool) {
68+
for (key, value) in &self.vars_os {
69+
if !f((key, value)) {
70+
self.removed.insert(key.clone());
71+
self.cargo.env_remove(key);
72+
}
73+
}
74+
}
75+
76+
pub fn env_remove(&mut self, key: impl AsRef<OsStr>) -> &mut Self {
77+
self.removed.insert(key.as_ref().to_os_string());
78+
self.cargo.env_remove(key);
79+
self
80+
}
81+
82+
pub fn env(&mut self, key: impl AsRef<OsStr>, val: impl AsRef<OsStr>) -> &mut Self {
83+
self.removed.remove(key.as_ref());
84+
self.cargo.env(key, val);
85+
self
86+
}
87+
88+
pub fn env_var_report(&self) -> EnvVarReport {
89+
let mut inherited = self.vars_os.clone();
90+
inherited.retain(|(key, _)| !self.removed.contains(key));
91+
EnvVarReport {
92+
inherited,
93+
removed: self.removed.clone(),
94+
}
95+
}
96+
}
97+
98+
impl Display for CargoCmd {
99+
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
100+
f.debug_struct("CargoCmd")
101+
.field("cargo", &self.cargo)
102+
.field("env_vars", &self.env_var_report())
103+
.finish()
104+
}
105+
}
106+
107+
impl Deref for CargoCmd {
108+
type Target = Command;
109+
110+
fn deref(&self) -> &Self::Target {
111+
&self.cargo
112+
}
113+
}
114+
115+
impl DerefMut for CargoCmd {
116+
fn deref_mut(&mut self) -> &mut Self::Target {
117+
&mut self.cargo
118+
}
119+
}
120+
121+
#[derive(Clone, Debug, Default)]
122+
pub struct EnvVarReport {
123+
pub inherited: Vec<(OsString, OsString)>,
124+
pub removed: HashSet<OsString>,
125+
}
126+
127+
impl Display for EnvVarReport {
128+
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
129+
let removed = self
130+
.removed
131+
.iter()
132+
.map(|key| format!("{}", key.to_string_lossy()))
133+
.collect::<Vec<_>>();
134+
let inherited = self
135+
.inherited
136+
.iter()
137+
.map(|(key, value)| format!("{}: {}", key.to_string_lossy(), value.to_string_lossy()))
138+
.collect::<Vec<_>>();
139+
140+
f.debug_struct("EnvVarReport")
141+
.field("removed", &removed)
142+
.field("inherited", &inherited)
143+
.finish()
144+
}
145+
}

crates/spirv-builder/src/lib.rs

Lines changed: 6 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@
7171
// #![allow()]
7272
#![doc = include_str!("../README.md")]
7373

74+
pub mod cargo_cmd;
7475
mod depfile;
7576
#[cfg(feature = "watch")]
7677
mod watch;
@@ -961,7 +962,7 @@ fn invoke_rustc(builder: &SpirvBuilder) -> Result<PathBuf, SpirvBuilderError> {
961962

962963
let profile = if builder.release { "release" } else { "dev" };
963964

964-
let mut cargo = Command::new("cargo");
965+
let mut cargo = cargo_cmd::CargoCmd::new();
965966
if let Some(toolchain) = &builder.toolchain_overwrite {
966967
cargo.arg(format!("+{toolchain}"));
967968
}
@@ -1014,35 +1015,6 @@ fn invoke_rustc(builder: &SpirvBuilder) -> Result<PathBuf, SpirvBuilderError> {
10141015

10151016
cargo.arg("--target-dir").arg(target_dir);
10161017

1017-
// Clear Cargo environment variables that we don't want to leak into the
1018-
// inner invocation of Cargo (because e.g. build scripts might read them),
1019-
// before we set any of our own below.
1020-
for (key, _) in env::vars_os() {
1021-
let remove = key
1022-
.to_str()
1023-
.is_some_and(|s| s.starts_with("CARGO_FEATURES_") || s.starts_with("CARGO_CFG_"));
1024-
if remove {
1025-
cargo.env_remove(key);
1026-
}
1027-
}
1028-
1029-
// NOTE(eddyb) Cargo caches some information it got from `rustc` in
1030-
// `.rustc_info.json`, and assumes it only depends on the `rustc` binary,
1031-
// but in our case, `rustc_codegen_spirv` changes are also relevant,
1032-
// so we turn off that caching with an env var, just to avoid any issues.
1033-
cargo.env("CARGO_CACHE_RUSTC_INFO", "0");
1034-
1035-
// NOTE(firestar99) If you call SpirvBuilder in a build script, it will
1036-
// set `RUSTC` before calling it. And if we were to propagate it to our
1037-
// cargo invocation, it will take precedence over the `+toolchain` we
1038-
// previously set.
1039-
cargo.env_remove("RUSTC");
1040-
1041-
// NOTE(tuguzT) Used by Cargo to call executables of Clippy, Miri
1042-
// (and maybe other Cargo subcommands) instead of `rustc`
1043-
// which could affect its functionality and break the build process.
1044-
cargo.env_remove("RUSTC_WRAPPER");
1045-
10461018
// NOTE(eddyb) this used to be just `RUSTFLAGS` but at some point Cargo
10471019
// added a separate environment variable using `\x1f` instead of spaces,
10481020
// which allows us to have spaces within individual `rustc` flags.
@@ -1051,21 +1023,18 @@ fn invoke_rustc(builder: &SpirvBuilder) -> Result<PathBuf, SpirvBuilderError> {
10511023
join_checking_for_separators(rustflags, "\x1f"),
10521024
);
10531025

1054-
let profile_in_env_var = profile.replace('-', "_").to_ascii_uppercase();
1055-
10561026
// NOTE(eddyb) there's no parallelism to take advantage of multiple CGUs,
10571027
// and inter-CGU duplication can be wasteful, so this forces 1 CGU for now.
1028+
let profile_in_env_var = profile.replace('-', "_").to_ascii_uppercase();
10581029
let num_cgus = 1;
10591030
cargo.env(
10601031
format!("CARGO_PROFILE_{profile_in_env_var}_CODEGEN_UNITS"),
10611032
num_cgus.to_string(),
10621033
);
10631034

1064-
let build = cargo
1065-
.stderr(Stdio::inherit())
1066-
.current_dir(path_to_crate)
1067-
.output()
1068-
.expect("failed to execute cargo build");
1035+
cargo.stderr(Stdio::inherit()).current_dir(path_to_crate);
1036+
log::debug!("building shaders with `{cargo}`");
1037+
let build = cargo.output().expect("failed to execute cargo build");
10691038

10701039
// `get_last_artifact` has the side-effect of printing invalid lines, so
10711040
// we do that even in case of an error, to let through any useful messages

tests/difftests/tests/Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)