Trying to refactor config & metadata modules of cargo-gpu

Author: tuguzT

crates/cargo-gpu/src/build.rs

@@ -9,7 +9,7 @@ use cargo_gpu_build::{
     spirv_builder::{CompileResult, ModuleResult, SpirvBuilder},
 };
 
-use crate::{install::Install, linkage::Linkage, user_consent::ask_for_user_consent};
+use crate::{install::InstallArgs, linkage::Linkage, user_consent::ask_for_user_consent};
 
 /// Args for just a build.
 #[derive(Debug, Clone, clap::Parser, serde::Deserialize, serde::Serialize)]
@@ -47,12 +47,12 @@ impl Default for BuildArgs {
 }
 
 /// `cargo build` subcommands.
-#[derive(Clone, Debug, clap::Parser, serde::Deserialize, serde::Serialize)]
+#[derive(Clone, Debug, Default, clap::Parser, serde::Deserialize, serde::Serialize)]
 #[non_exhaustive]
 pub struct Build {
     /// CLI args for install the `rust-gpu` compiler and components.
     #[clap(flatten)]
-    pub install: Install,
+    pub install: InstallArgs,
 
     /// CLI args for configuring the build of the shader.
     #[clap(flatten)]
@@ -195,22 +195,25 @@ impl Build {
 mod test {
     use clap::Parser as _;
 
-    use crate::{Cli, Command};
+    use crate::{
+        test::{shader_crate_template_path, tests_teardown},
+        Cli, Command,
+    };
 
     #[test_log::test]
     fn builder_from_params() {
-        crate::test::tests_teardown();
+        tests_teardown();
 
-        let shader_crate_path = crate::test::shader_crate_template_path();
+        let shader_crate_path = shader_crate_template_path();
         let output_dir = shader_crate_path.join("shaders");
 
         let args = [
-            "target/debug/cargo-gpu",
-            "build",
-            "--shader-crate",
-            &format!("{}", shader_crate_path.display()),
-            "--output-dir",
-            &format!("{}", output_dir.display()),
+            "target/debug/cargo-gpu".as_ref(),
+            "build".as_ref(),
+            "--shader-crate".as_ref(),
+            shader_crate_path.as_os_str(),
+            "--output-dir".as_ref(),
+            output_dir.as_os_str(),
         ];
         if let Cli {
             command: Command::Build(build),

crates/cargo-gpu/src/config.rs

@@ -1,115 +1,61 @@
 //! Manage and merge the various sources of config: shader crate's `Cargo.toml`(s) and CLI args.
-use anyhow::Context as _;
-use clap::Parser as _;
 
-/// Config
-pub struct Config;
+use std::path::Path;
 
-impl Config {
-    /// Get all the config defaults as JSON.
-    pub fn defaults_as_json() -> anyhow::Result<serde_json::Value> {
-        let defaults_json = Self::cli_args_to_json(vec![String::new()])?;
-        Ok(defaults_json)
-    }
-
-    /// Convert CLI args to their serde JSON representation.
-    fn cli_args_to_json(env_args: Vec<String>) -> anyhow::Result<serde_json::Value> {
-        Ok(serde_json::to_value(crate::build::Build::parse_from(
-            env_args,
-        ))?)
-    }
-
-    /// Config for the `cargo gpu build` and `cargo gpu install` can be set in the shader crate's
-    /// `Cargo.toml`, so here we load that config first as the base config, and the CLI arguments can
-    /// then later override it.
-    pub fn clap_command_with_cargo_config(
-        shader_crate_path: &std::path::PathBuf,
-        mut env_args: Vec<String>,
-    ) -> anyhow::Result<crate::build::Build> {
-        let mut config = crate::metadata::Metadata::as_json(shader_crate_path)?;
-
-        env_args.retain(|arg| !(arg == "build" || arg == "install"));
-        let cli_args_json = Self::cli_args_to_json(env_args)?;
-        Self::json_merge(&mut config, cli_args_json, None)?;
-
-        let args = serde_json::from_value::<crate::build::Build>(config)?;
-        Ok(args)
-    }
+use serde::{de::DeserializeOwned, Serialize};
+use serde_json::{from_value, to_value};
 
-    /// Merge 2 JSON objects. But only if the incoming patch value isn't the default value.
-    /// Inspired by: <https://stackoverflow.com/a/47142105/575773>
-    pub fn json_merge(
-        left_in: &mut serde_json::Value,
-        right_in: serde_json::Value,
-        maybe_pointer: Option<&String>,
-    ) -> anyhow::Result<()> {
-        let defaults = Self::defaults_as_json()?;
-
-        match (left_in, right_in) {
-            (left @ &mut serde_json::Value::Object(_), serde_json::Value::Object(right)) => {
-                let left_as_object = left
-                    .as_object_mut()
-                    .context("Unreachable, we've already proved it's an object")?;
-                for (key, value) in right {
-                    let new_pointer = maybe_pointer.as_ref().map_or_else(
-                        || format!("/{}", key.clone()),
-                        |pointer| format!("{}/{}", pointer, key.clone()),
-                    );
-                    Self::json_merge(
-                        left_as_object
-                            .entry(key.clone())
-                            .or_insert(serde_json::Value::Null),
-                        value,
-                        Some(&new_pointer),
-                    )?;
-                }
-            }
-            (left, right) => {
-                if let Some(pointer) = maybe_pointer {
-                    let default = defaults.pointer(pointer).context(format!(
-                        "Configuration option with path `{pointer}` was not found in the default configuration, \
-                        which is:\ndefaults: {defaults:#?}"
-                    ))?;
-                    if &right != default {
-                        // Only overwrite if the new value differs from the defaults.
-                        *left = right;
-                    }
-                }
-            }
-        }
+use crate::{merge::json_merge, metadata::Metadata};
 
-        Ok(())
-    }
+/// Config for the subcommand can be set in the shader crate's `Cargo.toml`,
+/// so here we load that config first as the base config, and the CLI arguments can then later override it.
+pub fn clap_command_with_cargo_config<C>(shader_crate: &Path, config: &C) -> anyhow::Result<C>
+where
+    C: Default + Serialize + DeserializeOwned,
+{
+    let from_cargo_toml = Metadata::as_json(shader_crate)?;
+    let from_cli = to_value(config)?;
+    let default = to_value(C::default())?;
+    let final_config = json_merge(from_cargo_toml, from_cli, &default);
+    Ok(from_value(final_config)?)
 }
 
 #[cfg(test)]
 mod test {
-    use super::*;
+    use std::{io::Write as _, path::PathBuf};
 
-    use std::io::Write as _;
+    use clap::Parser as _;
+    use spirv_builder::Capability;
+
+    use crate::{
+        build::Build,
+        test::{overwrite_shader_cargo_toml, shader_crate_test_path},
+    };
+
+    use super::*;
 
     #[test_log::test]
     fn booleans_from_cli() {
-        let shader_crate_path = crate::test::shader_crate_test_path();
-
-        let args = Config::clap_command_with_cargo_config(
-            &shader_crate_path,
-            vec![
-                "gpu".to_owned(),
-                "build".to_owned(),
-                "--debug".to_owned(),
-                "--auto-install-rust-toolchain".to_owned(),
-            ],
-        )
-        .unwrap();
+        let shader_crate_path = shader_crate_test_path();
+        let config = Build::parse_from([
+            "gpu".as_ref(),
+            // "build".as_ref(),
+            "--shader-crate".as_ref(),
+            shader_crate_path.as_os_str(),
+            "--debug".as_ref(),
+            "--auto-install-rust-toolchain".as_ref(),
+        ]);
+
+        let args = clap_command_with_cargo_config(&shader_crate_path, &config).unwrap();
         assert!(!args.build.spirv_builder.release);
         assert!(args.install.auto_install_rust_toolchain);
     }
 
     #[test_log::test]
     fn booleans_from_cargo() {
-        let shader_crate_path = crate::test::shader_crate_test_path();
-        let mut file = crate::test::overwrite_shader_cargo_toml(&shader_crate_path);
+        let shader_crate_path = shader_crate_test_path();
+
+        let mut file = overwrite_shader_cargo_toml(&shader_crate_path);
         file.write_all(
             [
                 "[package.metadata.rust-gpu.build]",
@@ -122,14 +68,21 @@ mod test {
         )
         .unwrap();
 
-        let args = Config::clap_command_with_cargo_config(&shader_crate_path, vec![]).unwrap();
+        let config = Build::parse_from([
+            "gpu".as_ref(),
+            // "build".as_ref(),
+            "--shader-crate".as_ref(),
+            shader_crate_path.as_os_str(),
+        ]);
+
+        let args = clap_command_with_cargo_config(&shader_crate_path, &config).unwrap();
         assert!(!args.build.spirv_builder.release);
         assert!(args.install.auto_install_rust_toolchain);
     }
 
-    fn update_cargo_output_dir() -> std::path::PathBuf {
-        let shader_crate_path = crate::test::shader_crate_test_path();
-        let mut file = crate::test::overwrite_shader_cargo_toml(&shader_crate_path);
+    fn update_cargo_output_dir() -> PathBuf {
+        let shader_crate_path = shader_crate_test_path();
+        let mut file = overwrite_shader_cargo_toml(&shader_crate_path);
         file.write_all(
             [
                 "[package.metadata.rust-gpu.build]",
@@ -145,35 +98,41 @@ mod test {
     #[test_log::test]
     fn string_from_cargo() {
         let shader_crate_path = update_cargo_output_dir();
-
-        let args = Config::clap_command_with_cargo_config(&shader_crate_path, vec![]).unwrap();
+        let config = Build::parse_from([
+            "gpu".as_ref(),
+            // "build".as_ref(),
+            "--shader-crate".as_ref(),
+            shader_crate_path.as_os_str(),
+        ]);
+
+        let args = clap_command_with_cargo_config(&shader_crate_path, &config).unwrap();
         if cfg!(target_os = "windows") {
-            assert_eq!(args.build.output_dir, std::path::Path::new("C:/the/moon"));
+            assert_eq!(args.build.output_dir, Path::new("C:/the/moon"));
         } else {
-            assert_eq!(args.build.output_dir, std::path::Path::new("/the/moon"));
+            assert_eq!(args.build.output_dir, Path::new("/the/moon"));
         }
     }
 
     #[test_log::test]
     fn string_from_cargo_overwritten_by_cli() {
         let shader_crate_path = update_cargo_output_dir();
-
-        let args = Config::clap_command_with_cargo_config(
-            &shader_crate_path,
-            vec![
-                "gpu".to_owned(),
-                "build".to_owned(),
-                "--output-dir".to_owned(),
-                "/the/river".to_owned(),
-            ],
-        )
-        .unwrap();
-        assert_eq!(args.build.output_dir, std::path::Path::new("/the/river"));
+        let config = Build::parse_from([
+            "gpu".as_ref(),
+            // "build".as_ref(),
+            "--shader-crate".as_ref(),
+            shader_crate_path.as_os_str(),
+            "--output-dir".as_ref(),
+            "/the/river".as_ref(),
+        ]);
+
+        let args = clap_command_with_cargo_config(&shader_crate_path, &config).unwrap();
+        assert_eq!(args.build.output_dir, Path::new("/the/river"));
     }
 
     #[test_log::test]
     fn arrays_from_cargo() {
         let shader_crate_path = crate::test::shader_crate_test_path();
+
         let mut file = crate::test::overwrite_shader_cargo_toml(&shader_crate_path);
         file.write_all(
             [
@@ -185,30 +144,33 @@ mod test {
         )
         .unwrap();
 
-        let args = Config::clap_command_with_cargo_config(&shader_crate_path, vec![]).unwrap();
+        let config = Build::parse_from([
+            "gpu".as_ref(),
+            // "build".as_ref(),
+            "--shader-crate".as_ref(),
+            shader_crate_path.as_os_str(),
+        ]);
+
+        let args = clap_command_with_cargo_config(&shader_crate_path, &config).unwrap();
         assert_eq!(
             args.build.spirv_builder.capabilities,
-            vec![
-                spirv_builder::Capability::AtomicStorage,
-                spirv_builder::Capability::Matrix
-            ]
+            [Capability::AtomicStorage, Capability::Matrix]
         );
     }
 
     #[test_log::test]
     fn rename_manifest_parse() {
         let shader_crate_path = crate::test::shader_crate_test_path();
-
-        let args = Config::clap_command_with_cargo_config(
-            &shader_crate_path,
-            vec![
-                "gpu".to_owned(),
-                "build".to_owned(),
-                "--manifest-file".to_owned(),
-                "mymanifest".to_owned(),
-            ],
-        )
-        .unwrap();
+        let config = Build::parse_from([
+            "gpu".as_ref(),
+            // "build".as_ref(),
+            "--shader-crate".as_ref(),
+            shader_crate_path.as_os_str(),
+            "--manifest-file".as_ref(),
+            "mymanifest".as_ref(),
+        ]);
+
+        let args = clap_command_with_cargo_config(&shader_crate_path, &config).unwrap();
         assert_eq!(args.build.manifest_file, "mymanifest".to_owned());
     }
 }