Auto merge of #8629 - EmbarkStudios:master, r=ehuss

Fix bug with PathAndArg config values

This fixes an issue I noticed when trying to specify a target runner via the [`CARGO_TARGET_{triplet}_RUNNER`](https://doc.rust-lang.org/cargo/reference/config.html#targettriplerunner) environment variable, expecting it to override the value in our `.cargo/config.toml` file, which was giving quite strange errors until I figured out that cargo was actually merging the config file's values with the environment's values.

This change adds a small hack to use and `UnmergedStringList` from `PathAndArgs` instead of just plain `StringList`, which uses the type in the deserializer to determine if `Config::get_list_or_string` should merge the values from the config file(s) with the environment as it was doing before, or else only use the environment to the exclusion of the config file(s) if the key was set in the environment.

I also added a test for this to confirm both the bug and the fix.

Author: bors

src/cargo/core/compiler/compilation.rs

@@ -346,6 +346,7 @@ fn target_runner(
 
     // try target.{}.runner
     let key = format!("target.{}.runner", target);
+
     if let Some(v) = bcx.config.get::<Option<config::PathAndArgs>>(&key)? {
         let path = v.path.resolve_program(bcx.config);
         return Ok(Some((path, v.args)));

src/cargo/util/config/de.rs

@@ -57,6 +57,7 @@ impl<'de, 'config> de::Deserializer<'de> for Deserializer<'config> {
             (None, Some(_)) => true,
             _ => false,
         };
+
         if use_env {
             // Future note: If you ever need to deserialize a non-self describing
             // map type, this should implement a starts_with check (similar to how
@@ -187,13 +188,17 @@ impl<'de, 'config> de::Deserializer<'de> for Deserializer<'config> {
     where
         V: de::Visitor<'de>,
     {
-        if name == "StringList" {
-            let vals = self.config.get_list_or_string(&self.key)?;
-            let vals: Vec<String> = vals.into_iter().map(|vd| vd.0).collect();
-            visitor.visit_newtype_struct(vals.into_deserializer())
+        let merge = if name == "StringList" {
+            true
+        } else if name == "UnmergedStringList" {
+            false
         } else {
-            visitor.visit_newtype_struct(self)
-        }
+            return visitor.visit_newtype_struct(self);
+        };
+
+        let vals = self.config.get_list_or_string(&self.key, merge)?;
+        let vals: Vec<String> = vals.into_iter().map(|vd| vd.0).collect();
+        visitor.visit_newtype_struct(vals.into_deserializer())
     }
 
     fn deserialize_enum<V>(

src/cargo/util/config/mod.rs

@@ -587,8 +587,21 @@ impl Config {
     }
 
     /// Helper for StringList type to get something that is a string or list.
-    fn get_list_or_string(&self, key: &ConfigKey) -> CargoResult<Vec<(String, Definition)>> {
+    fn get_list_or_string(
+        &self,
+        key: &ConfigKey,
+        merge: bool,
+    ) -> CargoResult<Vec<(String, Definition)>> {
         let mut res = Vec::new();
+
+        if !merge {
+            self.get_env_list(key, &mut res)?;
+
+            if !res.is_empty() {
+                return Ok(res);
+            }
+        }
+
         match self.get_cv(key)? {
             Some(CV::List(val, _def)) => res.extend(val),
             Some(CV::String(val, def)) => {
@@ -602,6 +615,7 @@ impl Config {
         }
 
         self.get_env_list(key, &mut res)?;
+
         Ok(res)
     }
 
@@ -1766,6 +1780,14 @@ impl StringList {
     }
 }
 
+/// StringList automatically merges config values with environment values,
+/// this instead follows the precedence rules, so that eg. a string list found
+/// in the environment will be used instead of one in a config file.
+///
+/// This is currently only used by `PathAndArgs`
+#[derive(Debug, Deserialize)]
+pub struct UnmergedStringList(Vec<String>);
+
 #[macro_export]
 macro_rules! __shell_print {
     ($config:expr, $which:ident, $newline:literal, $($arg:tt)*) => ({

src/cargo/util/config/path.rs

@@ -1,4 +1,4 @@
-use super::{Config, StringList, Value};
+use super::{Config, UnmergedStringList, Value};
 use serde::{de::Error, Deserialize};
 use std::path::PathBuf;
 
@@ -55,7 +55,7 @@ impl<'de> serde::Deserialize<'de> for PathAndArgs {
     where
         D: serde::Deserializer<'de>,
     {
-        let vsl = Value::<StringList>::deserialize(deserializer)?;
+        let vsl = Value::<UnmergedStringList>::deserialize(deserializer)?;
         let mut strings = vsl.val.0;
         if strings.is_empty() {
             return Err(D::Error::invalid_length(0, &"at least one element"));

tests/testsuite/tool_paths.rs

@@ -288,6 +288,35 @@ Caused by:
         .run();
 }
 
+#[cargo_test]
+fn custom_runner_env_overrides_config() {
+    let target = rustc_host();
+    let p = project()
+        .file("src/main.rs", "fn main() {}")
+        .file(
+            ".cargo/config.toml",
+            &format!(
+                r#"
+        [target.{}]
+        runner = "should-not-run -r"
+    "#,
+                target
+            ),
+        )
+        .build();
+
+    let key = format!(
+        "CARGO_TARGET_{}_RUNNER",
+        target.to_uppercase().replace('-', "_")
+    );
+
+    p.cargo("run")
+        .env(&key, "should-run --foo")
+        .with_status(101)
+        .with_stderr_contains("[RUNNING] `should-run --foo target/debug/foo[EXE]`")
+        .run();
+}
+
 #[cargo_test]
 #[cfg(unix)] // Assumes `true` is in PATH.
 fn custom_runner_env_true() {