Propagate .cargo/config.toml [env] settings to the process enviro…

…nment (#16)

Leaving the caller to read and merge config environment values with the
process environment is not only cumbersome as seen in
[rust-mobile/ndk#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-mobile/ndk#233]: rust-mobile/ndk#233

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<Cow<'_, str>> {
-        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,23 +265,31 @@ 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 {
         config: toml::from_str::<Config>(toml).unwrap(),
         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::<Config>(toml).unwrap(),
+        workspace: PathBuf::new(),
+    };
+
+    assert!(matches!(config.set_env_vars(), Err(EnvError::Io(..))));
 }

src/error.rs

@@ -17,16 +17,15 @@ 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.",
             Self::GlobPatternError(error) => 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)
+        })
     }
 }
 

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)

src/utils.rs

@@ -80,7 +80,7 @@ pub fn find_workspace(manifest: &Path, name: &str) -> Result<Option<PathBuf>, 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<String, Error> {
     if let Some(config) = config {
         if let Some(build) = config.build.as_ref() {