From f1ae8538c8af97d46756f2b6e9c0d0af3748f9a0 Mon Sep 17 00:00:00 2001
From: J Robert Ray <jrray@jrray.org>
Date: Tue, 18 Jul 2023 19:34:46 -0700
Subject: [PATCH 1/2] Fix cyclic crate dependency #1

Move the macros in spk-solve to its own crate, to break the cycle of

   spk-solve -> spk-solve-validation -> spk-solve (dev)

The macro code was changed to remove many uses of re-exported symbols
through `$crate`, since adding these dependencies would reintroduce a
cyclic dependency.

Signed-off-by: J Robert Ray <jrray@jrray.org>
---
 Cargo.lock                                    | 16 ++++-
 crates/spk-cli/group1/Cargo.toml              |  2 +
 .../spk-cli/group1/src/cmd_deprecate_test.rs  |  2 +-
 .../group1/src/cmd_undeprecate_test.rs        |  2 +-
 crates/spk-solve/Cargo.toml                   |  1 +
 crates/spk-solve/crates/macros/Cargo.toml     | 13 ++++
 .../macros.rs => crates/macros/src/lib.rs}    | 67 +++++++++++--------
 .../crates/package-iterator/Cargo.toml        |  2 +-
 .../src/package_iterator_test.rs              |  2 +-
 crates/spk-solve/crates/validation/Cargo.toml |  2 +-
 .../validation/src/impossible_checks_test.rs  |  2 +-
 .../crates/validation/src/validation_test.rs  |  2 +-
 crates/spk-solve/src/lib.rs                   |  1 -
 crates/spk-solve/src/solver_test.rs           | 12 +---
 14 files changed, 77 insertions(+), 49 deletions(-)
 create mode 100644 crates/spk-solve/crates/macros/Cargo.toml
 rename crates/spk-solve/{src/macros.rs => crates/macros/src/lib.rs} (72%)

diff --git a/Cargo.lock b/Cargo.lock
index 12113a733..3a3026399 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -3477,6 +3477,8 @@ dependencies = [
  "spk-cli-common",
  "spk-schema",
  "spk-solve",
+ "spk-solve-macros",
+ "spk-solve-solution",
  "spk-storage",
  "strip-ansi-escapes",
  "tokio",
@@ -3911,6 +3913,7 @@ dependencies = [
  "spk-config",
  "spk-schema",
  "spk-solve-graph",
+ "spk-solve-macros",
  "spk-solve-package-iterator",
  "spk-solve-solution",
  "spk-solve-validation",
@@ -3949,6 +3952,15 @@ dependencies = [
  "tracing",
 ]
 
+[[package]]
+name = "spk-solve-macros"
+version = "0.36.0"
+dependencies = [
+ "serde_json",
+ "spfs",
+ "spk-schema",
+]
+
 [[package]]
 name = "spk-solve-package-iterator"
 version = "0.36.0"
@@ -3960,7 +3972,7 @@ dependencies = [
  "once_cell",
  "rstest",
  "spk-schema",
- "spk-solve",
+ "spk-solve-macros",
  "spk-solve-solution",
  "spk-storage",
  "thiserror",
@@ -3994,8 +4006,8 @@ dependencies = [
  "serde_yaml",
  "spfs",
  "spk-schema",
- "spk-solve",
  "spk-solve-graph",
+ "spk-solve-macros",
  "spk-solve-solution",
  "spk-storage",
  "thiserror",
diff --git a/crates/spk-cli/group1/Cargo.toml b/crates/spk-cli/group1/Cargo.toml
index 070b202a9..ec97f98e5 100644
--- a/crates/spk-cli/group1/Cargo.toml
+++ b/crates/spk-cli/group1/Cargo.toml
@@ -28,3 +28,5 @@ tracing = { workspace = true }
 
 [dev-dependencies]
 rstest = { workspace = true }
+spk-solve-macros = { path = '../../spk-solve/crates/macros' }
+spk-solve-solution = { path = '../../spk-solve/crates/solution' }
diff --git a/crates/spk-cli/group1/src/cmd_deprecate_test.rs b/crates/spk-cli/group1/src/cmd_deprecate_test.rs
index 31994eb28..4de9f9075 100644
--- a/crates/spk-cli/group1/src/cmd_deprecate_test.rs
+++ b/crates/spk-cli/group1/src/cmd_deprecate_test.rs
@@ -5,7 +5,7 @@
 use rstest::rstest;
 use spk_schema::ident::parse_version_ident;
 use spk_schema::Deprecate;
-use spk_solve::make_repo;
+use spk_solve_macros::make_repo;
 
 use super::{change_deprecation_state, ChangeAction};
 
diff --git a/crates/spk-cli/group1/src/cmd_undeprecate_test.rs b/crates/spk-cli/group1/src/cmd_undeprecate_test.rs
index 2c48a2ffd..f299a4ad8 100644
--- a/crates/spk-cli/group1/src/cmd_undeprecate_test.rs
+++ b/crates/spk-cli/group1/src/cmd_undeprecate_test.rs
@@ -5,7 +5,7 @@
 use rstest::rstest;
 use spk_schema::ident::parse_version_ident;
 use spk_schema::Deprecate;
-use spk_solve::make_repo;
+use spk_solve_macros::make_repo;
 
 use super::{change_deprecation_state, ChangeAction};
 
diff --git a/crates/spk-solve/Cargo.toml b/crates/spk-solve/Cargo.toml
index a90ce5aa3..ad9ea66e4 100644
--- a/crates/spk-solve/Cargo.toml
+++ b/crates/spk-solve/Cargo.toml
@@ -49,4 +49,5 @@ tracing = { workspace = true }
 
 [dev-dependencies]
 rstest = { workspace = true }
+spk-solve-macros = { path = "./crates/macros" }
 strip-ansi-escapes = { workspace = true }
diff --git a/crates/spk-solve/crates/macros/Cargo.toml b/crates/spk-solve/crates/macros/Cargo.toml
new file mode 100644
index 000000000..9c0124ae2
--- /dev/null
+++ b/crates/spk-solve/crates/macros/Cargo.toml
@@ -0,0 +1,13 @@
+[package]
+authors = { workspace = true }
+edition = { workspace = true }
+name = "spk-solve-macros"
+version = { workspace = true }
+
+[features]
+migration-to-components = []
+
+[dependencies]
+serde_json = { workspace = true }
+spfs = { path = "../../../spfs" }
+spk-schema = { path = "../../../spk-schema" }
diff --git a/crates/spk-solve/src/macros.rs b/crates/spk-solve/crates/macros/src/lib.rs
similarity index 72%
rename from crates/spk-solve/src/macros.rs
rename to crates/spk-solve/crates/macros/src/lib.rs
index 3df751b21..e707bc51b 100644
--- a/crates/spk-solve/src/macros.rs
+++ b/crates/spk-solve/crates/macros/src/lib.rs
@@ -2,6 +2,12 @@
 // SPDX-License-Identifier: Apache-2.0
 // https://github.com/imageworks/spk
 
+#![deny(unsafe_op_in_unsafe_fn)]
+#![warn(clippy::fn_params_excessive_bools)]
+
+pub use spk_schema::recipe;
+pub use {serde_json, spfs};
+
 /// Creates a repository containing a set of provided package specs.
 /// It will take care of publishing the spec, and creating a build for
 /// each provided package so that it can be resolved.
@@ -14,16 +20,16 @@ macro_rules! make_repo {
         make_repo!([ $( $spec ),* ], options={})
     }};
     ( [ $( $spec:tt ),+ $(,)? ], options={ $($k:expr => $v:expr),* } ) => {{
-        let options = $crate::option_map!{$($k => $v),*};
+        let options = spk_schema::foundation::option_map!{$($k => $v),*};
         make_repo!([ $( $spec ),* ], options=options)
     }};
     ( [ $( $spec:tt ),+ $(,)? ], options=$options:expr ) => {{
         tracing::debug!("creating in-memory repository");
-        let repo = $crate::RepositoryHandle::new_mem();
+        let repo = spk_storage::RepositoryHandle::new_mem();
         let _opts = $options;
         $(
             let (s, cmpts) = $crate::make_package!(repo, $spec, &_opts);
-            tracing::trace!(pkg=%$crate::Package::ident(&s), cmpts=?cmpts.keys(), "adding package to repo");
+            tracing::trace!(pkg=%spk_schema::Package::ident(&s), cmpts=?cmpts.keys(), "adding package to repo");
             repo.publish_package(&s, &cmpts).await.unwrap();
         )*
         repo
@@ -36,7 +42,7 @@ macro_rules! make_package {
         ($build_spec, $components)
     }};
     ($repo:ident, $build_spec:ident, $opts:expr) => {{
-        use $crate::Package;
+        use spk_schema::Package;
         let s = $build_spec.clone();
         let cmpts: std::collections::HashMap<_, $crate::spfs::encoding::Digest> = s
             .components()
@@ -47,7 +53,7 @@ macro_rules! make_package {
     }};
     ($repo:ident, $spec:tt, $opts:expr) => {{
         let json = $crate::serde_json::json!($spec);
-        let spec: $crate::v0::Spec<$crate::AnyIdent> =
+        let spec: spk_schema::v0::Spec<spk_schema::ident::AnyIdent> =
             $crate::serde_json::from_value(json).expect("Invalid spec json");
         match spec.pkg.build().map(|b| b.clone()) {
             None => {
@@ -55,10 +61,13 @@ macro_rules! make_package {
                 $repo.force_publish_recipe(&recipe).await.unwrap();
                 make_build_and_components!(recipe, [], $opts, [])
             }
-            Some($crate::Build::Source) => {
+            Some(spk_schema::foundation::ident_build::Build::Source) => {
                 let recipe = spec.clone().map_ident(|i| i.into_base());
                 $repo.force_publish_recipe(&recipe.into()).await.unwrap();
-                let build = spec.map_ident(|i| i.into_base().into_build($crate::Build::Source));
+                let build = spec.map_ident(|i| {
+                    i.into_base()
+                        .into_build(spk_schema::foundation::ident_build::Build::Source)
+                });
                 make_build_and_components!(build, [], $opts, [])
             }
             Some(b) => {
@@ -108,31 +117,31 @@ macro_rules! make_build_and_components {
         make_build_and_components!($spec, [$($dep),*], $opts, [])
     };
     ($spec:tt, [$($dep:expr),*], { $($k:expr => $v:expr),* }, [$($component:expr),*]) => {{
-        let opts = $crate::option_map!{$($k => $v),*};
+        let opts = spk_schema::foundation::option_map!{$($k => $v),*};
         make_build_and_components!($spec, [$($dep),*], opts, [$($component),*])
     }};
     ($spec:tt, [$($dep:expr),*], $opts:expr, [$($component:expr),*]) => {{
         #[allow(unused_imports)]
-        use $crate::{Package, Recipe};
+        use spk_schema::{Package, Recipe};
         let json = $crate::serde_json::json!($spec);
-        let spec: $crate::v0::Spec<$crate::AnyIdent> =
+        let spec: spk_schema::v0::Spec<spk_schema::ident::AnyIdent> =
             $crate::serde_json::from_value(json).expect("Invalid spec json");
-        let mut components = std::collections::HashMap::<$crate::Component, $crate::spfs::encoding::Digest>::new();
+        let mut components = std::collections::HashMap::<spk_schema::foundation::ident_component::Component, $crate::spfs::encoding::Digest>::new();
         match spec.pkg.build().map(|b| b.clone()) {
             None => {
                 let recipe = spec.clone().map_ident(|i| i.into_base());
                 let mut build_opts = $opts.clone();
                 #[allow(unused_mut)]
-                let mut solution = $crate::Solution::new(build_opts.clone());
+                let mut solution = spk_solve_solution::Solution::new(build_opts.clone());
                 $(
                 let dep = Arc::new($dep.clone());
                 solution.add(
-                    $crate::PkgRequest::from_ident(
+                    spk_schema::ident::PkgRequest::from_ident(
                         $dep.ident().to_any(),
-                        $crate::RequestedBy::SpkInternalTest,
+                        spk_schema::ident::RequestedBy::SpkInternalTest,
                     ),
                     Arc::clone(&dep),
-                    $crate::PackageSource::SpkInternalTest,
+                    spk_solve_solution::PackageSource::SpkInternalTest,
                 );
                 )*
                 let mut resolved_opts = recipe.resolve_options(&build_opts).unwrap().into_iter();
@@ -145,15 +154,15 @@ macro_rules! make_build_and_components {
                     names = build.components().iter().map(|c| c.name.to_string()).collect();
                 }
                 for name in names {
-                    let name = $crate::Component::parse(name).expect("invalid component name");
+                    let name = spk_schema::foundation::ident_component::Component::parse(name).expect("invalid component name");
                     components.insert(name, $crate::spfs::encoding::EMPTY_DIGEST.into());
                 }
-                ($crate::Spec::V0Package(build), components)
+                (spk_schema::Spec::V0Package(build), components)
             }
-            Some(b @ $crate::Build::Source) => {
+            Some(b @ spk_schema::foundation::ident_build::Build::Source) => {
                 let build = spec.map_ident(|i| i.into_base().into_build(b));
-                components.insert($crate::Component::Source, $crate::spfs::encoding::EMPTY_DIGEST.into());
-                ($crate::Spec::V0Package(build), components)
+                components.insert(spk_schema::foundation::ident_component::Component::Source, $crate::spfs::encoding::EMPTY_DIGEST.into());
+                (spk_schema::Spec::V0Package(build), components)
             }
             Some(b) => {
                 let build = spec.map_ident(|i| i.into_base().into_build(b));
@@ -162,10 +171,10 @@ macro_rules! make_build_and_components {
                     names = build.components().iter().map(|c| c.name.to_string()).collect();
                 }
                 for name in names {
-                    let name = $crate::Component::parse(name).expect("invalid component name");
+                    let name = spk_schema::foundation::ident_component::Component::parse(name).expect("invalid component name");
                     components.insert(name, $crate::spfs::encoding::EMPTY_DIGEST.into());
                 }
-                ($crate::Spec::V0Package(build), components)
+                (spk_schema::Spec::V0Package(build), components)
             }
         }
     }}
@@ -187,20 +196,20 @@ macro_rules! make_spec {
 #[macro_export(local_inner_macros)]
 macro_rules! request {
     ($req:literal) => {
-        $crate::Request::Pkg($crate::PkgRequest::new(
-            $crate::parse_ident_range($req).unwrap(),
-            RequestedBy::SpkInternalTest,
+        spk_schema::ident::Request::Pkg(spk_schema::ident::PkgRequest::new(
+            spk_schema::ident::parse_ident_range($req).unwrap(),
+            spk_schema::ident::RequestedBy::SpkInternalTest,
         ))
     };
     ($req:ident) => {
-        $crate::Request::Pkg($crate::PkgRequest::new(
-            $crate::parse_ident_range($req).unwrap(),
-            RequestedBy::SpkInternalTest,
+        spk_schema::ident::Request::Pkg(spk_schema::ident::PkgRequest::new(
+            spk_schema::ident::parse_ident_range($req).unwrap(),
+            spk_schema::ident::RequestedBy::SpkInternalTest,
         ))
     };
     ($req:tt) => {{
         let value = serde_json::json!($req);
-        let req: $crate::Request = serde_json::from_value(value).unwrap();
+        let req: spk_schema::ident::Request = serde_json::from_value(value).unwrap();
         req
     }};
 }
diff --git a/crates/spk-solve/crates/package-iterator/Cargo.toml b/crates/spk-solve/crates/package-iterator/Cargo.toml
index dae52fd31..ecf676213 100644
--- a/crates/spk-solve/crates/package-iterator/Cargo.toml
+++ b/crates/spk-solve/crates/package-iterator/Cargo.toml
@@ -26,4 +26,4 @@ tracing = { workspace = true }
 [dev-dependencies]
 itertools = "0.10"
 rstest = { workspace = true }
-spk-solve = { path = "../.." }
+spk-solve-macros = { path = "../macros" }
diff --git a/crates/spk-solve/crates/package-iterator/src/package_iterator_test.rs b/crates/spk-solve/crates/package-iterator/src/package_iterator_test.rs
index 3e6b310aa..863a47295 100644
--- a/crates/spk-solve/crates/package-iterator/src/package_iterator_test.rs
+++ b/crates/spk-solve/crates/package-iterator/src/package_iterator_test.rs
@@ -11,7 +11,7 @@ use spk_schema::foundation::option_map;
 use spk_schema::foundation::option_map::OptionMap;
 use spk_schema::foundation::version::Compatibility;
 use spk_schema::{recipe, spec, BuildIdent, Package, Spec};
-use spk_solve::{make_build, make_repo};
+use spk_solve_macros::{make_build, make_repo};
 
 use super::{BuildIterator, PackageIterator, RepositoryPackageIterator, SortedBuildIterator};
 
diff --git a/crates/spk-solve/crates/validation/Cargo.toml b/crates/spk-solve/crates/validation/Cargo.toml
index c80213fe4..966b40b78 100644
--- a/crates/spk-solve/crates/validation/Cargo.toml
+++ b/crates/spk-solve/crates/validation/Cargo.toml
@@ -31,4 +31,4 @@ tracing = "0.1.35"
 [dev-dependencies]
 rstest = { workspace = true }
 serde_yaml = "0.8.17"
-spk-solve = { path = "../.." }
+spk-solve-macros = { path = "../macros" }
diff --git a/crates/spk-solve/crates/validation/src/impossible_checks_test.rs b/crates/spk-solve/crates/validation/src/impossible_checks_test.rs
index aa67609c7..31ecd6191 100644
--- a/crates/spk-solve/crates/validation/src/impossible_checks_test.rs
+++ b/crates/spk-solve/crates/validation/src/impossible_checks_test.rs
@@ -10,7 +10,7 @@ use spk_schema::foundation::fixtures::*;
 use spk_schema::ident::{version_ident, PkgRequest, RequestedBy};
 use spk_schema::name::PkgNameBuf;
 use spk_schema::spec;
-use spk_solve::{make_build, make_repo};
+use spk_solve_macros::{make_build, make_repo};
 
 use super::ImpossibleRequestsChecker;
 
diff --git a/crates/spk-solve/crates/validation/src/validation_test.rs b/crates/spk-solve/crates/validation/src/validation_test.rs
index df05729b1..9b358a290 100644
--- a/crates/spk-solve/crates/validation/src/validation_test.rs
+++ b/crates/spk-solve/crates/validation/src/validation_test.rs
@@ -8,8 +8,8 @@ use spk_schema::foundation::fixtures::*;
 use spk_schema::foundation::opt_name;
 use spk_schema::ident::{build_ident, version_ident, PkgRequest, Request, RequestedBy};
 use spk_schema::{spec, FromYaml};
-use spk_solve::recipe;
 use spk_solve_graph::State;
+use spk_solve_macros::recipe;
 use spk_solve_solution::PackageSource;
 
 use super::{default_validators, OptionsValidator, ValidatorT, VarRequirementsValidator};
diff --git a/crates/spk-solve/src/lib.rs b/crates/spk-solve/src/lib.rs
index efe3ef7ac..b3afbe0a3 100644
--- a/crates/spk-solve/src/lib.rs
+++ b/crates/spk-solve/src/lib.rs
@@ -7,7 +7,6 @@
 
 mod error;
 mod io;
-mod macros;
 #[cfg(feature = "statsd")]
 mod metrics;
 mod search_space;
diff --git a/crates/spk-solve/src/solver_test.rs b/crates/spk-solve/src/solver_test.rs
index 89d747853..63883be3e 100644
--- a/crates/spk-solve/src/solver_test.rs
+++ b/crates/spk-solve/src/solver_test.rs
@@ -22,21 +22,13 @@ use spk_schema::ident::{
 use spk_schema::ident_build::EmbeddedSource;
 use spk_schema::prelude::*;
 use spk_schema::{recipe, v0};
+use spk_solve_macros::{make_build, make_build_and_components, make_repo, request};
 use spk_solve_solution::PackageSource;
 use spk_storage::RepositoryHandle;
 
 use super::{ErrorDetails, Solver};
 use crate::io::DecisionFormatterBuilder;
-use crate::{
-    make_build,
-    make_build_and_components,
-    make_repo,
-    option_map,
-    request,
-    spec,
-    Error,
-    Result,
-};
+use crate::{option_map, spec, Error, Result};
 
 #[fixture]
 fn solver() -> Solver {

From db541a38c0767df290089785dfdc9b81aa6bba91 Mon Sep 17 00:00:00 2001
From: J Robert Ray <jrray@jrray.org>
Date: Tue, 18 Jul 2023 19:44:51 -0700
Subject: [PATCH 2/2] Fix cyclic crate dependency #2

Relocate a tests file to avoid a cyclic dependency:

    spk_schema -> spk_schema_validators -> spk_schema (dev)

Signed-off-by: J Robert Ray <jrray@jrray.org>
---
 Cargo.lock                                                   | 1 -
 crates/spk-schema/crates/validators/Cargo.toml               | 3 ---
 crates/spk-schema/crates/validators/src/validators.rs        | 5 ++---
 crates/spk-schema/src/v0/mod.rs                              | 4 ++++
 .../{crates/validators/src => src/v0}/validators_test.rs     | 4 ++--
 5 files changed, 8 insertions(+), 9 deletions(-)
 rename crates/spk-schema/{crates/validators/src => src/v0}/validators_test.rs (93%)

diff --git a/Cargo.lock b/Cargo.lock
index 3a3026399..6d72d6a05 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -3879,7 +3879,6 @@ dependencies = [
  "serde_json",
  "serde_yaml",
  "spfs",
- "spk-schema",
  "spk-schema-foundation",
  "spk-schema-ident",
  "sys-info",
diff --git a/crates/spk-schema/crates/validators/Cargo.toml b/crates/spk-schema/crates/validators/Cargo.toml
index 37e01e490..6e4c4b2f5 100644
--- a/crates/spk-schema/crates/validators/Cargo.toml
+++ b/crates/spk-schema/crates/validators/Cargo.toml
@@ -29,6 +29,3 @@ sys-info = "0.9.0"
 tempdir = "0.3.7"
 thiserror = { workspace = true }
 tracing = { workspace = true }
-
-[dev-dependencies]
-spk-schema = { path = "../.." }
diff --git a/crates/spk-schema/crates/validators/src/validators.rs b/crates/spk-schema/crates/validators/src/validators.rs
index 13e94f380..4fba90c72 100644
--- a/crates/spk-schema/crates/validators/src/validators.rs
+++ b/crates/spk-schema/crates/validators/src/validators.rs
@@ -11,9 +11,8 @@ use spk_schema_ident::BuildIdent;
 
 use crate::{Error, Result};
 
-#[cfg(test)]
-#[path = "./validators_test.rs"]
-mod validators_test;
+// Tests for this module are in spk-schema/src/v0/validators_test.rs to avoid
+// a cyclic crate dependency (the tests need spk_schema::v0).
 
 /// Validates that all remaining build files are collected into at least one component
 pub fn must_collect_all_files<'a, Files>(
diff --git a/crates/spk-schema/src/v0/mod.rs b/crates/spk-schema/src/v0/mod.rs
index 1768289d4..cee7255ce 100644
--- a/crates/spk-schema/src/v0/mod.rs
+++ b/crates/spk-schema/src/v0/mod.rs
@@ -9,3 +9,7 @@ mod variant;
 pub use spec::Spec;
 pub use test_spec::TestSpec;
 pub use variant::Variant;
+
+#[cfg(test)]
+#[path = "./validators_test.rs"]
+mod validators_test;
diff --git a/crates/spk-schema/crates/validators/src/validators_test.rs b/crates/spk-schema/src/v0/validators_test.rs
similarity index 93%
rename from crates/spk-schema/crates/validators/src/validators_test.rs
rename to crates/spk-schema/src/v0/validators_test.rs
index 5b95f3d8c..43642fad1 100644
--- a/crates/spk-schema/crates/validators/src/validators_test.rs
+++ b/crates/spk-schema/src/v0/validators_test.rs
@@ -2,9 +2,9 @@
 // SPDX-License-Identifier: Apache-2.0
 // https://github.com/imageworks/spk
 
-use spk_schema::v0;
+use spk_schema_validators::{must_install_something, must_not_alter_existing_files};
 
-use super::{must_install_something, must_not_alter_existing_files};
+use crate::v0;
 use crate::validators::must_collect_all_files;
 
 #[test]