diff --git a/crates/spirv-builder/src/lib.rs b/crates/spirv-builder/src/lib.rs index cd19f00862e..8bb5ffbf17b 100644 --- a/crates/spirv-builder/src/lib.rs +++ b/crates/spirv-builder/src/lib.rs @@ -883,57 +883,23 @@ fn join_checking_for_separators(strings: Vec>, sep: &str) -> St strings.join(sep) } -pub struct RustcOutput { - pub compile_result: CompileResult, - pub deps: Vec, +/// Overrides read from `RUSTGPU_*` environment variables, see [`SpirvBuilder::rustflags`]. +#[derive(Default)] +struct EnvOverrides { + /// `RUSTGPU_CODEGEN_ARGS`: replaces [`SpirvBuilder::extra_args`]. + codegen_args: Option, + /// `RUSTGPU_RUSTFLAGS`: appended to the rustflags. + rustflags: Option, + /// `RUSTGPU_CARGOFLAGS`: appended to the cargo args. + cargoflags: Option, } impl SpirvBuilder { - fn invoke_rustc(&self) -> Result { - 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 { 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 { + (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 { 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 { + 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, +} + +impl SpirvBuilder { + fn invoke_rustc(&self) -> Result { + 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()); } diff --git a/crates/spirv-builder/src/tests.rs b/crates/spirv-builder/src/tests.rs index 6b37072be07..e3e6564edaf 100644 --- a/crates/spirv-builder/src/tests.rs +++ b/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 { + 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 { + 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::::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"] + ); + } +}