feat(spirv-builder): warn when RUSTGPU_CODEGEN_ARGS replaces extra_args

crates/spirv-builder/src/lib.rs

@@ -883,57 +883,23 @@ fn join_checking_for_separators(strings: Vec<impl Borrow<str>>, sep: &str) -> St
     strings.join(sep)
 }
 
-pub struct RustcOutput {
-    pub compile_result: CompileResult,
-    pub deps: Vec<RawString>,
+/// Overrides read from `RUSTGPU_*` environment variables, see [`SpirvBuilder::rustflags`].
+#[derive(Default)]
+struct EnvOverrides {
+    /// `RUSTGPU_CODEGEN_ARGS`: replaces [`SpirvBuilder::extra_args`].
+    codegen_args: Option<String>,
+    /// `RUSTGPU_RUSTFLAGS`: appended to the rustflags.
+    rustflags: Option<String>,
+    /// `RUSTGPU_CARGOFLAGS`: appended to the cargo args.
+    cargoflags: Option<String>,
 }
 
 impl SpirvBuilder {
-    fn invoke_rustc(&self) -> Result<RustcOutput, SpirvBuilderError> {
-        let path_to_crate = self
-            .path_to_crate
-            .as_ref()
-            .ok_or(SpirvBuilderError::MissingCratePath)?;
-        let target;
-        {
-            let target_str = self
-                .target
-                .as_ref()
-                .ok_or(SpirvBuilderError::MissingTarget)?;
-            target = SpirvTarget::parse(target_str)?;
-            if !path_to_crate.is_dir() {
-                return Err(SpirvBuilderError::CratePathDoesntExist(
-                    path_to_crate.clone(),
-                ));
-            }
-        }
-
-        let toolchain_rustc_version =
-            if let Some(toolchain_rustc_version) = &self.toolchain_rustc_version {
-                toolchain_rustc_version.clone()
-            } else {
-                query_rustc_version(self.toolchain_overwrite.as_deref())?
-            };
-
-        // Okay, this is a little bonkers: in a normal world, we'd have the user clone
-        // rustc_codegen_spirv and pass in the path to it, and then we'd invoke cargo to build it, grab
-        // the resulting .so, and pass it into -Z codegen-backend. But that's really gross: the user
-        // needs to clone rustc_codegen_spirv and tell us its path! So instead, we *directly reference
-        // rustc_codegen_spirv in spirv-builder's Cargo.toml*, which means that it will get built
-        // alongside build.rs, and cargo will helpfully add it to LD_LIBRARY_PATH for us! However,
-        // rustc expects a full path, instead of a filename looked up via LD_LIBRARY_PATH, so we need
-        // to copy cargo's understanding of library lookup and find the library and its full path.
-        let rustc_codegen_spirv = Ok(self.rustc_codegen_spirv_location.clone())
-            .transpose()
-            .unwrap_or_else(find_rustc_codegen_spirv)?;
-        if !rustc_codegen_spirv.is_file() {
-            return Err(SpirvBuilderError::RustcCodegenSpirvDylibDoesNotExist(
-                rustc_codegen_spirv,
-            ));
-        }
-
+    /// The `rustc` flags (for `CARGO_ENCODED_RUSTFLAGS`) to build the shader crate with the
+    /// `rustc_codegen_spirv` backend at `codegen_backend`.
+    fn rustflags(&self, codegen_backend: &Path, env_overrides: &EnvOverrides) -> Vec<String> {
         let mut rustflags = vec![
-            format!("-Zcodegen-backend={}", rustc_codegen_spirv.display()),
+            format!("-Zcodegen-backend={}", codegen_backend.display()),
             // Ensure the codegen backend is emitted in `.d` files to force Cargo
             // to rebuild crates compiled with it when it changes (this used to be
             // the default until https://github.com/rust-lang/rust/pull/93969).
@@ -962,14 +928,41 @@ impl SpirvBuilder {
             "-Zshare-generics=off".to_string(),
         ];
 
-        // Wrapper for `env::var` that appropriately informs Cargo of the dependency.
-        let tracked_env_var_get = |name| {
-            if self.build_script.get_dependency_info() {
-                println!("cargo:rerun-if-env-changed={name}");
-            }
-            env::var(name)
-        };
+        let llvm_args = join_checking_for_separators(self.llvm_args(env_overrides), " ");
+        if !llvm_args.is_empty() {
+            rustflags.push(["-Cllvm-args=", &llvm_args].concat());
+        }
+
+        let target_features = join_checking_for_separators(self.target_features(), ",");
+        if !target_features.is_empty() {
+            rustflags.push(["-Ctarget-feature=", &target_features].concat());
+        }
+
+        if self.deny_warnings {
+            rustflags.push("-Dwarnings".to_string());
+        }
+
+        if let Some(extra_rustflags) = &env_overrides.rustflags {
+            rustflags.extend(extra_rustflags.split_whitespace().map(|s| s.to_string()));
+        }
+
+        rustflags
+    }
+
+    /// `RUSTGPU_CODEGEN_ARGS` replaces [`Self::extra_args`], which is easy to miss when both are
+    /// set, so this warns about the ignored ones.
+    fn ignored_extra_args_warning(&self, env_overrides: &EnvOverrides) -> Option<String> {
+        (env_overrides.codegen_args.is_some() && !self.extra_args.is_empty()).then(|| {
+            format!(
+                "`RUSTGPU_CODEGEN_ARGS` is set, ignoring the codegen args set via \
+                 `SpirvBuilder::extra_arg`: {:?}",
+                self.extra_args
+            )
+        })
+    }
 
+    /// The `rustc_codegen_spirv` args, passed via `-Cllvm-args`.
+    fn llvm_args(&self, env_overrides: &EnvOverrides) -> Vec<String> {
         let mut llvm_args = vec![];
         if self.multimodule {
             llvm_args.push("--module-output=multiple".to_string());
@@ -1002,51 +995,111 @@ impl SpirvBuilder {
         if self.optimizer.preserve_bindings {
             llvm_args.push("--preserve-bindings".to_string());
         }
-        let mut target_features = vec![];
         let abort_strategy = match self.shader_panic_strategy {
             ShaderPanicStrategy::SilentExit => None,
             ShaderPanicStrategy::DebugPrintfThenExit {
                 print_inputs,
                 print_backtrace,
-            } => {
-                target_features.push("+ext:SPV_KHR_non_semantic_info".into());
-                Some(format!(
-                    "debug-printf{}{}",
-                    if print_inputs { "+inputs" } else { "" },
-                    if print_backtrace { "+backtrace" } else { "" }
-                ))
-            }
+            } => Some(format!(
+                "debug-printf{}{}",
+                if print_inputs { "+inputs" } else { "" },
+                if print_backtrace { "+backtrace" } else { "" }
+            )),
             ShaderPanicStrategy::UNSOUND_DO_NOT_USE_UndefinedBehaviorViaUnreachable => {
                 Some("unreachable".into())
             }
         };
         llvm_args.extend(abort_strategy.map(|strategy| format!("--abort-strategy={strategy}")));
 
-        if let Ok(extra_codegen_args) = tracked_env_var_get("RUSTGPU_CODEGEN_ARGS") {
+        if let Some(extra_codegen_args) = &env_overrides.codegen_args {
             llvm_args.extend(extra_codegen_args.split_whitespace().map(|s| s.to_string()));
         } else {
             llvm_args.extend(self.extra_args.iter().cloned());
         }
+        llvm_args
+    }
 
-        let llvm_args = join_checking_for_separators(llvm_args, " ");
-        if !llvm_args.is_empty() {
-            rustflags.push(["-Cllvm-args=", &llvm_args].concat());
+    /// The target features (capabilities and extensions), passed via `-Ctarget-feature`.
+    fn target_features(&self) -> Vec<String> {
+        let mut target_features = vec![];
+        if let ShaderPanicStrategy::DebugPrintfThenExit { .. } = self.shader_panic_strategy {
+            target_features.push("+ext:SPV_KHR_non_semantic_info".into());
         }
-
         target_features.extend(self.capabilities.iter().map(|cap| format!("+{cap:?}")));
         target_features.extend(self.extensions.iter().map(|ext| format!("+ext:{ext}")));
-        let target_features = join_checking_for_separators(target_features, ",");
-        if !target_features.is_empty() {
-            rustflags.push(["-Ctarget-feature=", &target_features].concat());
+        target_features
+    }
+}
+
+pub struct RustcOutput {
+    pub compile_result: CompileResult,
+    pub deps: Vec<RawString>,
+}
+
+impl SpirvBuilder {
+    fn invoke_rustc(&self) -> Result<RustcOutput, SpirvBuilderError> {
+        let path_to_crate = self
+            .path_to_crate
+            .as_ref()
+            .ok_or(SpirvBuilderError::MissingCratePath)?;
+        let target;
+        {
+            let target_str = self
+                .target
+                .as_ref()
+                .ok_or(SpirvBuilderError::MissingTarget)?;
+            target = SpirvTarget::parse(target_str)?;
+            if !path_to_crate.is_dir() {
+                return Err(SpirvBuilderError::CratePathDoesntExist(
+                    path_to_crate.clone(),
+                ));
+            }
         }
 
-        if self.deny_warnings {
-            rustflags.push("-Dwarnings".to_string());
+        let toolchain_rustc_version =
+            if let Some(toolchain_rustc_version) = &self.toolchain_rustc_version {
+                toolchain_rustc_version.clone()
+            } else {
+                query_rustc_version(self.toolchain_overwrite.as_deref())?
+            };
+
+        // Okay, this is a little bonkers: in a normal world, we'd have the user clone
+        // rustc_codegen_spirv and pass in the path to it, and then we'd invoke cargo to build it, grab
+        // the resulting .so, and pass it into -Z codegen-backend. But that's really gross: the user
+        // needs to clone rustc_codegen_spirv and tell us its path! So instead, we *directly reference
+        // rustc_codegen_spirv in spirv-builder's Cargo.toml*, which means that it will get built
+        // alongside build.rs, and cargo will helpfully add it to LD_LIBRARY_PATH for us! However,
+        // rustc expects a full path, instead of a filename looked up via LD_LIBRARY_PATH, so we need
+        // to copy cargo's understanding of library lookup and find the library and its full path.
+        let rustc_codegen_spirv = Ok(self.rustc_codegen_spirv_location.clone())
+            .transpose()
+            .unwrap_or_else(find_rustc_codegen_spirv)?;
+        if !rustc_codegen_spirv.is_file() {
+            return Err(SpirvBuilderError::RustcCodegenSpirvDylibDoesNotExist(
+                rustc_codegen_spirv,
+            ));
         }
 
-        if let Ok(extra_rustflags) = tracked_env_var_get("RUSTGPU_RUSTFLAGS") {
-            rustflags.extend(extra_rustflags.split_whitespace().map(|s| s.to_string()));
+        // Wrapper for `env::var` that appropriately informs Cargo of the dependency.
+        let tracked_env_var_get = |name| {
+            if self.build_script.get_dependency_info() {
+                println!("cargo:rerun-if-env-changed={name}");
+            }
+            env::var(name).ok()
+        };
+        let env_overrides = EnvOverrides {
+            codegen_args: tracked_env_var_get("RUSTGPU_CODEGEN_ARGS"),
+            rustflags: tracked_env_var_get("RUSTGPU_RUSTFLAGS"),
+            cargoflags: tracked_env_var_get("RUSTGPU_CARGOFLAGS"),
+        };
+        if let Some(warning) = self.ignored_extra_args_warning(&env_overrides) {
+            if self.build_script.get_forward_rustc_warnings() {
+                println!("cargo::warning={warning}");
+            } else {
+                log::warn!("{warning}");
+            }
         }
+        let rustflags = self.rustflags(&rustc_codegen_spirv, &env_overrides);
 
         let target_dir_path = self
             .target_dir_path
@@ -1090,7 +1143,7 @@ impl SpirvBuilder {
             cargo.args(["--crate-type", "dylib"]);
         }
 
-        if let Ok(extra_cargoflags) = tracked_env_var_get("RUSTGPU_CARGOFLAGS") {
+        if let Some(extra_cargoflags) = &env_overrides.cargoflags {
             cargo.args(extra_cargoflags.split_whitespace());
         }
 

crates/spirv-builder/src/tests.rs

@@ -102,3 +102,106 @@ mod dylib_lookup {
         );
     }
 }
+
+mod rustflags {
+    use crate::{Capability, EnvOverrides, ShaderPanicStrategy, SpirvBuilder, SpirvMetadata};
+    use std::path::Path;
+
+    const BACKEND: &str = "/backend/librustc_codegen_spirv.so";
+
+    fn builder() -> SpirvBuilder {
+        SpirvBuilder::new("/shader", "spirv-unknown-vulkan1.2")
+    }
+
+    fn rustflags(builder: &SpirvBuilder, env: &EnvOverrides) -> Vec<String> {
+        builder.rustflags(Path::new(BACKEND), env)
+    }
+
+    /// A builder that emits no configurable flags at all (the default panic strategy does).
+    fn silent_builder() -> SpirvBuilder {
+        builder().shader_panic_strategy(ShaderPanicStrategy::SilentExit)
+    }
+
+    /// The flags after the fixed ones every build gets.
+    fn extra_rustflags(builder: &SpirvBuilder, env: &EnvOverrides) -> Vec<String> {
+        let fixed = rustflags(&silent_builder(), &EnvOverrides::default()).len();
+        rustflags(builder, env)[fixed..].to_vec()
+    }
+
+    #[test]
+    fn fixed_flags_start_with_the_backend() {
+        let flags = rustflags(&builder(), &EnvOverrides::default());
+        assert_eq!(flags[0], format!("-Zcodegen-backend={BACKEND}"));
+        assert!(flags.contains(&"-Zbinary-dep-depinfo".to_string()));
+        assert!(flags.contains(&"-Zshare-generics=off".to_string()));
+    }
+
+    #[test]
+    fn silent_exit_has_no_llvm_args_or_target_features() {
+        let flags = extra_rustflags(&silent_builder(), &EnvOverrides::default());
+        assert_eq!(flags, Vec::<String>::new());
+    }
+
+    #[test]
+    fn builder_options_map_to_flags() {
+        let builder = builder()
+            .multimodule(true)
+            .spirv_metadata(SpirvMetadata::Full)
+            .relax_block_layout(true)
+            .preserve_bindings(true)
+            .shader_panic_strategy(ShaderPanicStrategy::DebugPrintfThenExit {
+                print_inputs: true,
+                print_backtrace: false,
+            })
+            .capability(Capability::Int8)
+            .extension("SPV_KHR_foo")
+            .extra_arg("--dump-post-link=/tmp/x")
+            .deny_warnings(true);
+        assert_eq!(
+            extra_rustflags(&builder, &EnvOverrides::default()),
+            [
+                "-Cllvm-args=--module-output=multiple --spirv-metadata=full \
+                 --relax-block-layout --preserve-bindings \
+                 --abort-strategy=debug-printf+inputs --dump-post-link=/tmp/x",
+                "-Ctarget-feature=+ext:SPV_KHR_non_semantic_info,+Int8,+ext:SPV_KHR_foo",
+                "-Dwarnings",
+            ]
+        );
+    }
+
+    #[test]
+    fn warns_about_extra_args_replaced_by_env_codegen_args() {
+        let env = EnvOverrides {
+            codegen_args: Some("--from-env".into()),
+            ..EnvOverrides::default()
+        };
+        let with_extra_args = silent_builder().extra_arg("--from-builder");
+        assert_eq!(
+            with_extra_args.ignored_extra_args_warning(&env).as_deref(),
+            Some(
+                "`RUSTGPU_CODEGEN_ARGS` is set, ignoring the codegen args set via \
+                 `SpirvBuilder::extra_arg`: [\"--from-builder\"]"
+            )
+        );
+        // nothing ignored
+        assert_eq!(silent_builder().ignored_extra_args_warning(&env), None);
+        assert_eq!(
+            with_extra_args.ignored_extra_args_warning(&EnvOverrides::default()),
+            None
+        );
+    }
+
+    #[test]
+    fn env_codegen_args_replace_extra_args_and_env_rustflags_are_appended() {
+        let builder = silent_builder().extra_arg("--from-builder");
+        let env = EnvOverrides {
+            codegen_args: Some("--from-env  --and-more".into()),
+            rustflags: Some("-Cfoo -Cbar".into()),
+            cargoflags: None,
+        };
+        assert_eq!(
+            extra_rustflags(&builder, &env),
+            ["-Cllvm-args=--from-env --and-more", "-Cfoo", "-Cbar"]
+        );
+    }
+}