From 6ca514f8b71581d44072ccd2277ab455d2f69949 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Tue, 22 Mar 2022 17:56:58 +0100 Subject: [PATCH] Propagate `.cargo/config.toml` `[env]` settings to the process environment (#16) Leaving the caller to read and merge config environment values with the process environment is not only cumbersome as seen in [rust-windowing/android-ndk-rs#233], but these values need to be propagated into the process environment anyway to be usable by any subprocess spawned by the caller (ie. `cargo-apk` spawning `cargo rustc`, which can internally spawn a bunch of compilers and linkers too). Instead, do this automatically when the caller invokes `Subcommand::new` while still leaving the door open for crates to read the environment from the public `subcommand.manifest.env` member. [rust-windowing/android-ndk-rs#233]: https://github.com/rust-windowing/android-ndk-rs/pull/233 --- src/config.rs | 121 +++++++++++++++++++++------------------------- src/error.rs | 5 +- src/subcommand.rs | 5 ++ src/utils.rs | 2 +- 4 files changed, 64 insertions(+), 69 deletions(-) diff --git a/src/config.rs b/src/config.rs index aad57e6..e674d9e 100644 --- a/src/config.rs +++ b/src/config.rs @@ -94,36 +94,26 @@ impl LocalizedConfig { config.map(LocalizedConfig::new).transpose() } - /// Read an environment variable from the `[env]` section in this `.cargo/config.toml`. + /// Propagate environment variables from this `.cargo/config.toml` to the process environment + /// using [`std::env::set_var()`]. /// - /// It is interpreted as path and canonicalized relative to [`Self::workspace`] if - /// [`EnvOption::Value::relative`] is set. - /// - /// Process environment variables (from [`std::env::var()`]) have [precedence] - /// unless [`EnvOption::Value::force`] is set. This value is also returned if - /// the given key was not set under `[env]`. - /// - /// [precedence]: https://doc.rust-lang.org/cargo/reference/config.html#env - pub fn resolve_env(&self, key: &str) -> Result> { - let config_var = self.config.env.as_ref().and_then(|env| env.get(key)); - - // Environment variables always have precedence unless - // the extended format is used to set `force = true`: - if let Some(env_option @ EnvOption::Value { force: true, .. }) = config_var { - // Errors iresolving (canonicalizing, really) the config variable take precedence, too: - return env_option.resolve_value(&self.workspace); - } - - let process_var = std::env::var(key); - if process_var != Err(VarError::NotPresent) { - // Errors from env::var() also have precedence here: - return Ok(process_var?.into()); + /// Note that this is automatically performed when calling [`Subcommand::new()`][super::Subcommand::new()]. + pub fn set_env_vars(&self) -> Result<()> { + if let Some(env) = &self.config.env { + for (key, env_option) in env { + // Existing environment variables always have precedence unless + // the extended format is used to set `force = true`: + if !matches!(env_option, EnvOption::Value { force: true, .. }) + && std::env::var_os(key).is_some() + { + continue; + } + + std::env::set_var(key, env_option.resolve_value(&self.workspace)?.as_ref()) + } } - // Finally, the value in `[env]` (if it exists) is taken into account - config_var - .ok_or(VarError::NotPresent)? - .resolve_value(&self.workspace) + Ok(()) } } @@ -226,52 +216,45 @@ CARGO_SUBCOMMAND_TEST_ENV_FORCED = { value = "forced", force = true }"#; workspace: PathBuf::new(), }; + // Check if all values are propagated to the environment + config.set_env_vars().unwrap(); + assert!(matches!( - config.resolve_env("CARGO_SUBCOMMAND_TEST_ENV_NOT_SET"), - Err(EnvError::Var(VarError::NotPresent)) + std::env::var("CARGO_SUBCOMMAND_TEST_ENV_NOT_SET"), + Err(VarError::NotPresent) )); assert_eq!( - config - .resolve_env("CARGO_SUBCOMMAND_TEST_ENV_NOT_FORCED") - .unwrap(), - Cow::from("not forced") + std::env::var("CARGO_SUBCOMMAND_TEST_ENV_NOT_FORCED").unwrap(), + "not forced" ); assert_eq!( - config - .resolve_env("CARGO_SUBCOMMAND_TEST_ENV_FORCED") - .unwrap(), - Cow::from("forced") + std::env::var("CARGO_SUBCOMMAND_TEST_ENV_FORCED").unwrap(), + "forced" ); - std::env::set_var("CARGO_SUBCOMMAND_TEST_ENV_NOT_SET", "set in env"); + // Set some environment values std::env::set_var( "CARGO_SUBCOMMAND_TEST_ENV_NOT_FORCED", - "not forced overridden", + "not forced process environment value", ); - std::env::set_var("CARGO_SUBCOMMAND_TEST_ENV_FORCED", "forced overridden"); - - assert_eq!( - config - .resolve_env("CARGO_SUBCOMMAND_TEST_ENV_NOT_SET") - .unwrap(), - // Even if the value isn't present in [env] it should still resolve to the - // value in the process environment - Cow::from("set in env") + std::env::set_var( + "CARGO_SUBCOMMAND_TEST_ENV_FORCED", + "forced process environment value", ); + + config.set_env_vars().unwrap(); + assert_eq!( - config - .resolve_env("CARGO_SUBCOMMAND_TEST_ENV_NOT_FORCED") - .unwrap(), - // Value changed now that it is set in the environment - Cow::from("not forced overridden") + std::env::var("CARGO_SUBCOMMAND_TEST_ENV_NOT_FORCED").unwrap(), + // Value remains what is set in the process environment, + // and is not overwritten by set_env_vars() + "not forced process environment value" ); assert_eq!( - config - .resolve_env("CARGO_SUBCOMMAND_TEST_ENV_FORCED") - .unwrap(), - // Value stays at how it was configured in [env] with force=true, despite + std::env::var("CARGO_SUBCOMMAND_TEST_ENV_FORCED").unwrap(), + // Value is overwritten thanks to force=true, despite // also being set in the process environment - Cow::from("forced") + "forced" ); } @@ -282,7 +265,6 @@ fn test_env_canonicalization() { let toml = r#" [env] CARGO_SUBCOMMAND_TEST_ENV_SRC_DIR = { value = "src", force = true, relative = true } -CARGO_SUBCOMMAND_TEST_ENV_INEXISTENT_DIR = { value = "blahblahthisfolderdoesntexist", force = true, relative = true } "#; let config = LocalizedConfig { @@ -290,15 +272,24 @@ CARGO_SUBCOMMAND_TEST_ENV_INEXISTENT_DIR = { value = "blahblahthisfolderdoesntex workspace: PathBuf::new(), }; - let path = config - .resolve_env("CARGO_SUBCOMMAND_TEST_ENV_SRC_DIR") + config.set_env_vars().unwrap(); + + let path = std::env::var("CARGO_SUBCOMMAND_TEST_ENV_SRC_DIR") .expect("Canonicalization for a known-to-exist ./src folder should not fail"); - let path = Path::new(path.as_ref()); + let path = PathBuf::from(path); assert!(path.is_absolute()); assert!(path.is_dir()); assert_eq!(path.file_name(), Some(OsStr::new("src"))); - assert!(config - .resolve_env("CARGO_SUBCOMMAND_TEST_ENV_INEXISTENT_DIR") - .is_err()); + let toml = r#" +[env] +CARGO_SUBCOMMAND_TEST_ENV_INEXISTENT_DIR = { value = "blahblahthisfolderdoesntexist", force = true, relative = true } +"#; + + let config = LocalizedConfig { + config: toml::from_str::(toml).unwrap(), + workspace: PathBuf::new(), + }; + + assert!(matches!(config.set_env_vars(), Err(EnvError::Io(..)))); } diff --git a/src/error.rs b/src/error.rs index c2b0221..d698fd1 100644 --- a/src/error.rs +++ b/src/error.rs @@ -17,7 +17,7 @@ pub enum Error { impl Display for Error { fn fmt(&self, f: &mut Formatter) -> Result { - let msg = match self { + f.write_str(match self { Self::InvalidArgs => "Invalid args.", Self::ManifestNotFound => "Didn't find Cargo.toml.", Self::RustcNotFound => "Didn't find rustc.", @@ -25,8 +25,7 @@ impl Display for Error { Self::Glob(error) => return error.fmt(f), Self::Io(path, error) => return write!(f, "{}: {}", path.display(), error), Self::Toml(file, error) => return write!(f, "{}: {}", file.display(), error), - }; - write!(f, "{}", msg) + }) } } diff --git a/src/subcommand.rs b/src/subcommand.rs index 47cd1ac..a25091e 100644 --- a/src/subcommand.rs +++ b/src/subcommand.rs @@ -104,7 +104,12 @@ impl Subcommand { } }); + // TODO: Find, parse, and merge _all_ config files following the hierarchical structure: + // https://doc.rust-lang.org/cargo/reference/config.html#hierarchical-structure let config = LocalizedConfig::find_cargo_config_for_workspace(&root_dir)?; + if let Some(config) = &config { + config.set_env_vars().unwrap(); + } let target_dir = target_dir.unwrap_or_else(|| { utils::find_workspace(&manifest, &package) diff --git a/src/utils.rs b/src/utils.rs index 5936177..ae61a04 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -80,7 +80,7 @@ pub fn find_workspace(manifest: &Path, name: &str) -> Result, Er /// Returns the [`target-dir`] configured in `.cargo/config.toml` or `"target"` if not set. /// -/// [`target-dir`](https://doc.rust-lang.org/cargo/reference/config.html#buildtarget-dir) +/// [`target-dir`]: https://doc.rust-lang.org/cargo/reference/config.html#buildtarget-dir pub fn get_target_dir_name(config: Option<&Config>) -> Result { if let Some(config) = config { if let Some(build) = config.build.as_ref() {