Merge pull request #563 from imageworks/296-embedded-stub-trait

Remove v0::Opt from the Package/Recipe Traits

Author: rydrman

crates/progress_bar_derive_macro/src/lib.rs

@@ -146,7 +146,7 @@ fn impl_proc_macro_derive(ast: &syn::DeriveInput) -> TokenStream {
                 // or nothing will be shown in the terminal
                 let renderer = Some(std::thread::spawn(move || {
                     if let Err(err) = bars.join() {
-                        tracing::error!("Failed to render commit progress: {err}");
+                        tracing::error!("Failed to render commit progress: {}", err);
                     }
                 }));
                 Self {

crates/spk-build/src/build/binary.rs

@@ -25,12 +25,14 @@ use spk_schema::foundation::ident_component::Component;
 use spk_schema::foundation::option_map::OptionMap;
 use spk_schema::foundation::version::VERSION_SEP;
 use spk_schema::ident::{PkgRequest, PreReleasePolicy, RangeIdent, RequestedBy, VersionIdent};
+use spk_schema::version::Compatibility;
 use spk_schema::{
     BuildIdent,
     ComponentFileMatchMode,
     ComponentSpecList,
     Package,
     PackageMut,
+    RequirementsList,
     Variant,
     VariantExt,
 };
@@ -266,7 +268,7 @@ where
         };
 
         tracing::debug!("Resolving build environment");
-        let solution = self
+        let (build_requirements, solution) = self
             .resolve_build_environment(&all_options, &variant)
             .await?;
         self.environment
@@ -362,6 +364,7 @@ where
         let package = self
             .recipe
             .generate_binary_build(&full_variant, &solution)?;
+        self.validate_generated_package(&build_requirements, &solution, &package)?;
         let components = self
             .build_and_commit_artifacts(&package, full_variant.options())
             .await?;
@@ -430,7 +433,7 @@ where
         &mut self,
         options: &OptionMap,
         variant: &V,
-    ) -> Result<Solution>
+    ) -> Result<(RequirementsList, Solution)>
     where
         V: Variant,
     {
@@ -441,13 +444,56 @@ where
             self.solver.add_repository(repo);
         }
 
-        for request in self.recipe.get_build_requirements(variant)? {
-            self.solver.add_request(request);
+        let build_requirements = self.recipe.get_build_requirements(variant)?.into_owned();
+        for request in build_requirements.iter() {
+            self.solver.add_request(request.clone());
         }
 
         let (solution, graph) = self.build_resolver.solve(&self.solver).await?;
         self.last_solve_graph = graph;
-        Ok(solution)
+        Ok((build_requirements, solution))
+    }
+
+    fn validate_generated_package(
+        &self,
+        build_requirements: &RequirementsList,
+        solution: &Solution,
+        package: &Recipe::Output,
+    ) -> Result<()> {
+        let runtime_requirements = package.runtime_requirements();
+        let solved_packages = solution.items().map(|r| Arc::clone(&r.spec));
+        let all_components = package.components();
+        for spec in solved_packages {
+            for component in all_components.names() {
+                let downstream_build = spec.downstream_build_requirements([component]);
+                for request in downstream_build.iter() {
+                    match build_requirements.contains_request(request) {
+                        Compatibility::Compatible => continue,
+                        Compatibility::Incompatible(reason) => {
+                            return Err(Error::MissingDownstreamBuildRequest {
+                                required_by: spec.ident().to_owned(),
+                                request: request.clone(),
+                                problem: reason.to_string(),
+                            })
+                        }
+                    }
+                }
+                let downstream_runtime = spec.downstream_build_requirements([component]);
+                for request in downstream_runtime.iter() {
+                    match runtime_requirements.contains_request(request) {
+                        Compatibility::Compatible => continue,
+                        Compatibility::Incompatible(reason) => {
+                            return Err(Error::MissingDownstreamRuntimeRequest {
+                                required_by: spec.ident().to_owned(),
+                                request: request.clone(),
+                                problem: reason.to_string(),
+                            })
+                        }
+                    }
+                }
+            }
+        }
+        Ok(())
     }
 
     /// Helper for constructing more useful error messages from schema validator errors

crates/spk-build/src/build/binary_test.rs

@@ -10,22 +10,14 @@ use spk_schema::foundation::fixtures::*;
 use spk_schema::foundation::ident_component::Component;
 use spk_schema::foundation::{opt_name, option_map};
 use spk_schema::ident::{PkgRequest, RangeIdent, Request};
-use spk_schema::{
-    recipe,
-    ComponentSpecList,
-    FromYaml,
-    Inheritance,
-    Opt,
-    Package,
-    Recipe,
-    SpecRecipe,
-};
+use spk_schema::{recipe, ComponentSpecList, FromYaml, Package, Recipe, SpecRecipe};
 use spk_solve::Solution;
 use spk_storage::fixtures::*;
 use spk_storage::{self as storage, Repository};
 
 use super::{BinaryPackageBuilder, BuildSource};
 use crate::build::SourcePackageBuilder;
+use crate::Error;
 
 #[rstest]
 fn test_split_manifest_permissions() {
@@ -317,14 +309,14 @@ async fn test_build_var_pinning() {
         .unwrap();
 
     let spec = rt.tmprepo.read_package(spec.ident()).await.unwrap();
-    let top_req = spec.runtime_requirements().get(0).unwrap();
+    let top_req = spec.runtime_requirements().get(0).unwrap().clone();
     match top_req {
-        Request::Var(r) => assert_eq!(&r.value, "topvalue"),
+        Request::Var(r) => assert_eq!(r.value.as_pinned(), Some("topvalue")),
         _ => panic!("expected var request"),
     }
-    let depreq = spec.runtime_requirements().get(1).unwrap();
+    let depreq = spec.runtime_requirements().get(1).unwrap().clone();
     match depreq {
-        Request::Var(r) => assert_eq!(&r.value, "depvalue"),
+        Request::Var(r) => assert_eq!(r.value.as_pinned(), Some("depvalue")),
         _ => panic!("expected var request"),
     }
 }
@@ -519,14 +511,14 @@ async fn test_build_filters_unmodified_files() {
 
 #[rstest]
 #[tokio::test]
-async fn test_build_package_requirement_propagation() {
+async fn test_build_package_downstream_build_requests() {
     let rt = spfs_runtime().await;
     let base_spec = recipe!(
         {
             "pkg": "base/1.0.0",
             "sources": [],
             "build": {
-                "options": [{"var": "inherited/val", "inheritance": "Strong"}],
+                "options": [{"var": "inherited/val", "inheritance": "StrongForBuildOnly"}],
                 "script": "echo building...",
             },
         }
@@ -538,14 +530,14 @@ async fn test_build_package_requirement_propagation() {
             "build": {"options": [{"pkg": "base"}], "script": "echo building..."},
         }
     );
-    rt.tmprepo.publish_recipe(&base_spec).await.unwrap();
     rt.tmprepo.publish_recipe(&top_spec).await.unwrap();
+    rt.tmprepo.publish_recipe(&base_spec).await.unwrap();
 
     SourcePackageBuilder::from_recipe(base_spec.clone())
         .build_and_publish(".", &*rt.tmprepo)
         .await
         .unwrap();
-    let _base_pkg = BinaryPackageBuilder::from_recipe(base_spec)
+    let (base_pkg, _) = BinaryPackageBuilder::from_recipe(base_spec)
         .with_repository(rt.tmprepo.clone())
         .build_and_publish(option_map! {}, &*rt.tmprepo)
         .await
@@ -555,45 +547,85 @@ async fn test_build_package_requirement_propagation() {
         .build_and_publish(".", &*rt.tmprepo)
         .await
         .unwrap();
-    let (top_pkg, _) = BinaryPackageBuilder::from_recipe(top_spec)
+    let result = BinaryPackageBuilder::from_recipe(top_spec)
         .with_repository(rt.tmprepo.clone())
         .build_and_publish(option_map! {}, &*rt.tmprepo)
-        .await
-        .unwrap();
+        .await;
 
-    assert_eq!(top_pkg.options().len(), 2, "should get option added");
-    let opt = top_pkg.options().get(1).unwrap();
-    match opt {
-        Opt::Var(opt) => {
-            assert_eq!(
-                &*opt.var, "base.inherited",
-                "should be inherited as package option"
-            );
-            assert_eq!(
-                opt.inheritance,
-                Inheritance::Weak,
-                "inherited option should have weak inheritance"
+    match result {
+        Err(Error::MissingDownstreamBuildRequest {
+            required_by,
+            request,
+            ..
+        }) => {
+            assert_eq!(&required_by, base_pkg.ident());
+            assert!(
+                matches!(&request, Request::Var(v) if v.var.as_str() == "base.inherited" && v.value.as_pinned() == Some("val")),
+                "{request}"
             );
         }
-        _ => panic!("should be given inherited option"),
+        Err(err) => panic!("Expected Error::MissingDownstreamBuildRequest, got {err:?}"),
+        Ok(_) => panic!("should error when downstream package does not define inherited opt"),
     }
+}
 
-    assert_eq!(
-        top_pkg.runtime_requirements().len(),
-        1,
-        "should get install requirement"
+#[rstest]
+#[tokio::test]
+async fn test_build_package_downstream_runtime_request() {
+    let rt = spfs_runtime().await;
+    let base_spec = recipe!(
+        {
+            "pkg": "base/1.0.0",
+            "sources": [],
+            "build": {
+                "options": [{"var": "inherited/val", "inheritance": "Strong"}],
+                "script": "echo building...",
+            },
+        }
     );
-    let req = top_pkg.runtime_requirements().get(0).unwrap();
-    match req {
-        Request::Var(req) => {
-            assert_eq!(
-                &*req.var, "base.inherited",
-                "should be inherited with package namespace"
+    let top_spec = recipe!(
+        {
+            "pkg": "top/1.0.0",
+            "sources": [],
+            "build": {"options": [{"pkg": "base"}, {"var": "inherited/val"}], "script": "echo building..."},
+        }
+    );
+    rt.tmprepo.publish_recipe(&top_spec).await.unwrap();
+    rt.tmprepo.publish_recipe(&base_spec).await.unwrap();
+
+    SourcePackageBuilder::from_recipe(base_spec.clone())
+        .build_and_publish(".", &*rt.tmprepo)
+        .await
+        .unwrap();
+    let (base_pkg, _) = BinaryPackageBuilder::from_recipe(base_spec)
+        .with_repository(rt.tmprepo.clone())
+        .build_and_publish(&option_map! {}, &*rt.tmprepo)
+        .await
+        .unwrap();
+
+    SourcePackageBuilder::from_recipe(top_spec.clone())
+        .build_and_publish(".", &*rt.tmprepo)
+        .await
+        .unwrap();
+    let result = BinaryPackageBuilder::from_recipe(top_spec)
+        .with_repository(rt.tmprepo.clone())
+        .build_and_publish(&option_map! {}, &*rt.tmprepo)
+        .await;
+
+    match result {
+        Err(Error::MissingDownstreamRuntimeRequest {
+            required_by,
+            request,
+            ..
+        }) => {
+            assert_eq!(&required_by, base_pkg.ident());
+            assert!(
+                matches!(&request, Request::Var(v) if v.var.as_str() == "base.inherited" && v.value.as_pinned() == Some("val")),
+                "{request}"
             );
-            assert!(!req.pin, "should not be pinned after build");
-            assert_eq!(req.value, "val", "should be rendered to build time var");
         }
-        _ => panic!("should be given var request"),
+        Err(err) => panic!("Expected Error::MissingDownstreamRuntimeRequest, got {err}"),
+        Ok(_) => panic!("should error when downstream package does not define inherited opt"),
     }
 }
 

crates/spk-build/src/error.rs

@@ -2,6 +2,8 @@
 // SPDX-License-Identifier: Apache-2.0
 // https://github.com/imageworks/spk
 
+use spk_schema::BuildIdent;
+use spk_solve::Request;
 use thiserror::Error;
 
 pub type Result<T> = std::result::Result<T, Error>;
@@ -20,6 +22,24 @@ pub enum Error {
     FileWriteError(std::path::PathBuf, #[source] std::io::Error),
     #[error(transparent)]
     ProcessSpawnError(spfs::Error),
+    #[error("Package must include a build requirement for {request}, because it's being built against {required_by}, but {problem}")]
+    MissingDownstreamBuildRequest {
+        /// The package that was in the build environment and created the need for this request
+        required_by: BuildIdent,
+        /// The minimum request that is required downstream
+        request: Request,
+        /// Additional reasoning why an existing request was not sufficient
+        problem: String,
+    },
+    #[error("Package must include a runtime requirement for {request}, because it's being built against {required_by}, but {problem}")]
+    MissingDownstreamRuntimeRequest {
+        /// The package that was in the build environment and created the need for this request
+        required_by: BuildIdent,
+        /// The minimum request that is required downstream
+        request: Request,
+        /// Additional reasoning why an existing request was not sufficient
+        problem: String,
+    },
     #[error(transparent)]
     SPFS(#[from] spfs::Error),
     #[error(transparent)]

crates/spk-cli/common/src/flags.rs

@@ -415,7 +415,7 @@ impl Requests {
                             }
                             None => recipe.get_build_requirements(&options)?,
                         };
-                        out.extend(requirements);
+                        out.extend(requirements.into_owned());
                     }
 
                     TestStage::Install => {

crates/spk-schema/crates/foundation/src/version/compat.rs

@@ -87,7 +87,7 @@ impl Ord for CompatRule {
 }
 
 /// Denotes whether or not something is compatible.
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Eq, PartialEq)]
 pub enum IncompatibleReason {
     ConflictingEmbeddedPackage(PkgNameBuf),
     Other(String),
@@ -109,7 +109,7 @@ impl std::fmt::Display for IncompatibleReason {
 
 /// Denotes whether or not something is compatible.
 #[must_use = "this `Compatibility` may be an `Incompatible` variant, which should be handled"]
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Eq, PartialEq)]
 pub enum Compatibility {
     Compatible,
     Incompatible(IncompatibleReason),
@@ -129,8 +129,8 @@ impl std::ops::Not for &'_ Compatibility {
 
     fn not(self) -> Self::Output {
         match self {
-            super::Compatibility::Compatible => false,
-            super::Compatibility::Incompatible(_) => true,
+            Compatibility::Compatible => false,
+            Compatibility::Incompatible(_) => true,
         }
     }
 }
@@ -142,6 +142,8 @@ impl Compatibility {
         ))
     }
 
+    /// Creates a compatibility instance denoting incompatibility
+    /// for the provided reason
     pub fn incompatible(message: impl ToString) -> Self {
         Compatibility::Incompatible(IncompatibleReason::Other(message.to_string()))
     }

crates/spk-schema/crates/ident/src/lib.rs

@@ -28,6 +28,7 @@ pub use request::{
     is_false,
     InclusionPolicy,
     NameAndValue,
+    PinnableValue,
     PkgRequest,
     PreReleasePolicy,
     Request,