Merge #897

897: Fix build/target config file ordering r=Emilgardis a=Alexhuszagh Fix the order of values for string (as well as the pre-build vec) values when parsing from config files and environment variables. The old behavior was: 1. Envvar build. 2. Envvar target 3. Config target 4. Config build The new behavior is: 1. Envvar target 2. Config target 3. Envvar build. 4. Config build Some functions were also made more general. Co-authored-by: Alex Huszagh <[email protected]>
cross-rs · Jul 1, 2022 · 5bb63ae · 5bb63ae
2 parents 9f814c6 + 6fd1255
commit 5bb63ae
Show file tree

Hide file tree

Showing 3 changed files with 91 additions and 49 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -19,6 +19,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 
 ### Fixed
 
+- #897 - ensure `target.$(...)` config options override `build` ones when parsing strings and vecs.
 - #895 - convert filenames in docker tags to ASCII lowercase and ignore invalid characters
 - #885 - handle symlinks when using remote docker.
 - #868 - ignore the `CARGO` environment variable.

diff --git a/src/config.rs b/src/config.rs
@@ -69,21 +69,18 @@ impl Environment {
         self.get_target_var(target, "IMAGE")
     }
 
-    fn dockerfile(&self, target: &Target) -> Option<String> {
-        let res = self.get_values_for("DOCKERFILE", target, |s| s.to_string());
-        res.0.or(res.1)
+    fn dockerfile(&self, target: &Target) -> (Option<String>, Option<String>) {
+        self.get_values_for("DOCKERFILE", target, |s| s.to_string())
     }
 
-    fn dockerfile_context(&self, target: &Target) -> Option<String> {
-        let res = self.get_values_for("DOCKERFILE_CONTEXT", target, |s| s.to_string());
-        res.0.or(res.1)
+    fn dockerfile_context(&self, target: &Target) -> (Option<String>, Option<String>) {
+        self.get_values_for("DOCKERFILE_CONTEXT", target, |s| s.to_string())
     }
 
-    fn pre_build(&self, target: &Target) -> Option<Vec<String>> {
-        let res = self.get_values_for("PRE_BUILD", target, |v| {
+    fn pre_build(&self, target: &Target) -> (Option<Vec<String>>, Option<Vec<String>>) {
+        self.get_values_for("PRE_BUILD", target, |v| {
             v.split('\n').map(String::from).collect()
-        });
-        res.0.or(res.1)
+        })
     }
 
     fn runner(&self, target: &Target) -> Option<String> {
@@ -205,11 +202,29 @@ impl Config {
         config: impl for<'a> Fn(&'a CrossToml, &Target) -> (Option<&'a [String]>, Option<&'a [String]>),
         sum: bool,
     ) -> Result<Option<Vec<String>>> {
+        if sum {
+            let (mut env_build, env_target) = env(&self.env, target);
+            env_build
+                .as_mut()
+                .map(|b| env_target.map(|mut t| b.append(&mut t)));
+            self.sum_of_env_toml_values(env_build, |t| config(t, target))
+        } else {
+            self.get_from_ref(target, env, config)
+        }
+    }
+
+    fn get_from_ref<T, U>(
+        &self,
+        target: &Target,
+        env: impl for<'a> Fn(&'a Environment, &Target) -> (Option<T>, Option<T>),
+        config: impl for<'a> Fn(&'a CrossToml, &Target) -> (Option<&'a U>, Option<&'a U>),
+    ) -> Result<Option<T>>
+    where
+        U: ToOwned<Owned = T> + ?Sized,
+    {
         let (env_build, env_target) = env(&self.env, target);
 
-        if sum {
-            return self.sum_of_env_toml_values(env_target, |t| config(t, target));
-        } else if let Some(env_target) = env_target {
+        if let Some(env_target) = env_target {
             return Ok(Some(env_target));
         }
 
@@ -262,7 +277,7 @@ impl Config {
     }
 
     pub fn env_volumes(&self, target: &Target) -> Result<Option<Vec<String>>> {
-        self.vec_from_config(target, Environment::volumes, CrossToml::env_volumes, false)
+        self.get_from_ref(target, Environment::volumes, CrossToml::env_volumes)
     }
 
     pub fn target(&self, target_list: &TargetList) -> Option<Target> {
@@ -275,11 +290,11 @@ impl Config {
     }
 
     pub fn dockerfile(&self, target: &Target) -> Result<Option<String>> {
-        self.string_from_config(target, Environment::dockerfile, CrossToml::dockerfile)
+        self.get_from_ref(target, Environment::dockerfile, CrossToml::dockerfile)
     }
 
     pub fn dockerfile_context(&self, target: &Target) -> Result<Option<String>> {
-        self.string_from_config(
+        self.get_from_ref(
             target,
             Environment::dockerfile_context,
             CrossToml::dockerfile_context,
@@ -297,14 +312,10 @@ impl Config {
     }
 
     pub fn pre_build(&self, target: &Target) -> Result<Option<Vec<String>>> {
-        self.vec_from_config(
-            target,
-            |e, t| (None, e.pre_build(t)),
-            CrossToml::pre_build,
-            false,
-        )
+        self.get_from_ref(target, Environment::pre_build, CrossToml::pre_build)
     }
 
+    // FIXME: remove when we disable sums in 0.3.0.
     fn sum_of_env_toml_values<'a>(
         &'a self,
         env_values: Option<impl AsRef<[String]>>,
@@ -369,6 +380,11 @@ mod tests {
         Target::from("aarch64-unknown-linux-gnu", &target_list)
     }
 
+    fn target2() -> Target {
+        let target_list = target_list();
+        Target::from("armv7-unknown-linux-musleabihf", &target_list)
+    }
+
     mod test_environment {
 
         use super::*;
@@ -514,6 +530,29 @@ mod tests {
             Ok(())
         }
 
+        #[test]
+        pub fn env_target_then_toml_target_then_env_build_then_toml_build() -> Result<()> {
+            let mut map = HashMap::new();
+            map.insert("CROSS_BUILD_DOCKERFILE", "Dockerfile3");
+            map.insert(
+                "CROSS_TARGET_AARCH64_UNKNOWN_LINUX_GNU_DOCKERFILE",
+                "Dockerfile4",
+            );
+
+            let env = Environment::new(Some(map));
+            let config = Config::new_with(Some(toml(TOML_BUILD_DOCKERFILE)?), env);
+            assert_eq!(config.dockerfile(&target())?, Some(s!("Dockerfile4")));
+            assert_eq!(config.dockerfile(&target2())?, Some(s!("Dockerfile3")));
+
+            let map = HashMap::new();
+            let env = Environment::new(Some(map));
+            let config = Config::new_with(Some(toml(TOML_BUILD_DOCKERFILE)?), env);
+            assert_eq!(config.dockerfile(&target())?, Some(s!("Dockerfile2")));
+            assert_eq!(config.dockerfile(&target2())?, Some(s!("Dockerfile1")));
+
+            Ok(())
+        }
+
         #[test]
         pub fn toml_build_passthrough_then_use_target_passthrough_both() -> Result<()> {
             let map = HashMap::new();
@@ -527,7 +566,6 @@ mod tests {
                 config.env_volumes(&target())?,
                 Some(vec![s!("VOLUME3"), s!("VOLUME4")])
             );
-            // TODO(ahuszagh) Need volumes
 
             Ok(())
         }
@@ -640,6 +678,13 @@ mod tests {
         static TOML_BUILD_PRE_BUILD: &str = r#"
     [build]
     pre-build = ["apt-get update && apt-get install zlib-dev"]
+    "#;
+
+        static TOML_BUILD_DOCKERFILE: &str = r#"
+    [build]
+    dockerfile = "Dockerfile1"
+    [target.aarch64-unknown-linux-gnu]
+    dockerfile = "Dockerfile2"
     "#;
 
         static TOML_TARGET_XARGO_FALSE: &str = r#"

diff --git a/src/cross_toml.rs b/src/cross_toml.rs
@@ -220,17 +220,17 @@ impl CrossToml {
     }
 
     /// Returns the `{}.dockerfile` or `{}.dockerfile.file` part of `Cross.toml`
-    pub fn dockerfile(&self, target: &Target) -> Option<String> {
-        self.get_string(
+    pub fn dockerfile(&self, target: &Target) -> (Option<&String>, Option<&String>) {
+        self.get_ref(
             target,
             |b| b.dockerfile.as_ref().map(|c| &c.file),
             |t| t.dockerfile.as_ref().map(|c| &c.file),
         )
     }
 
     /// Returns the `target.{}.dockerfile.context` part of `Cross.toml`
-    pub fn dockerfile_context(&self, target: &Target) -> Option<String> {
-        self.get_string(
+    pub fn dockerfile_context(&self, target: &Target) -> (Option<&String>, Option<&String>) {
+        self.get_ref(
             target,
             |b| b.dockerfile.as_ref().and_then(|c| c.context.as_ref()),
             |t| t.dockerfile.as_ref().and_then(|c| c.context.as_ref()),
@@ -255,7 +255,7 @@ impl CrossToml {
 
     /// Returns the `build.dockerfile.pre-build` and `target.{}.dockerfile.pre-build` part of `Cross.toml`
     pub fn pre_build(&self, target: &Target) -> (Option<&[String]>, Option<&[String]>) {
-        self.get_vec(
+        self.get_ref(
             target,
             |b| b.pre_build.as_deref(),
             |t| t.pre_build.as_deref(),
@@ -269,17 +269,17 @@ impl CrossToml {
 
     /// Returns the `build.xargo` or the `target.{}.xargo` part of `Cross.toml`
     pub fn xargo(&self, target: &Target) -> (Option<bool>, Option<bool>) {
-        self.get_bool(target, |b| b.xargo, |t| t.xargo)
+        self.get_value(target, |b| b.xargo, |t| t.xargo)
     }
 
     /// Returns the `build.build-std` or the `target.{}.build-std` part of `Cross.toml`
     pub fn build_std(&self, target: &Target) -> (Option<bool>, Option<bool>) {
-        self.get_bool(target, |b| b.build_std, |t| t.build_std)
+        self.get_value(target, |b| b.build_std, |t| t.build_std)
     }
 
     /// Returns the list of environment variables to pass through for `build` and `target`
     pub fn env_passthrough(&self, target: &Target) -> (Option<&[String]>, Option<&[String]>) {
-        self.get_vec(
+        self.get_ref(
             target,
             |build| build.env.passthrough.as_deref(),
             |t| t.env.passthrough.as_deref(),
@@ -288,7 +288,7 @@ impl CrossToml {
 
     /// Returns the list of environment variables to pass through for `build` and `target`
     pub fn env_volumes(&self, target: &Target) -> (Option<&[String]>, Option<&[String]>) {
-        self.get_vec(
+        self.get_ref(
             target,
             |build| build.env.volumes.as_deref(),
             |t| t.env.volumes.as_deref(),
@@ -320,30 +320,26 @@ impl CrossToml {
             .map(ToOwned::to_owned)
     }
 
-    fn get_bool(
+    fn get_value<T>(
         &self,
-        target: &Target,
-        get_build: impl Fn(&CrossBuildConfig) -> Option<bool>,
-        get_target: impl Fn(&CrossTargetConfig) -> Option<bool>,
-    ) -> (Option<bool>, Option<bool>) {
+        target_triple: &Target,
+        get_build: impl Fn(&CrossBuildConfig) -> Option<T>,
+        get_target: impl Fn(&CrossTargetConfig) -> Option<T>,
+    ) -> (Option<T>, Option<T>) {
         let build = get_build(&self.build);
-        let target = self.get_target(target).and_then(get_target);
-
+        let target = self.get_target(target_triple).and_then(get_target);
         (build, target)
     }
 
-    fn get_vec(
+    fn get_ref<T: ?Sized>(
         &self,
         target_triple: &Target,
-        build: impl Fn(&CrossBuildConfig) -> Option<&[String]>,
-        target: impl Fn(&CrossTargetConfig) -> Option<&[String]>,
-    ) -> (Option<&[String]>, Option<&[String]>) {
-        let target = if let Some(t) = self.get_target(target_triple) {
-            target(t)
-        } else {
-            None
-        };
-        (build(&self.build), target)
+        get_build: impl Fn(&CrossBuildConfig) -> Option<&T>,
+        get_target: impl Fn(&CrossTargetConfig) -> Option<&T>,
+    ) -> (Option<&T>, Option<&T>) {
+        let build = get_build(&self.build);
+        let target = self.get_target(target_triple).and_then(get_target);
+        (build, target)
     }
 }