Remove use of v0::Opt from binary builder

This changes the current inheritance mechanism from being calculated
based on the specific v0::Opt::inheritance field, instead expecting the
package itself to identify downstream requests that need to be present.
In short, this means that users need to add explicit requests to
satisfy the upstream packages. As much as this is more work for users,
it ensures that the final nature of each one remains up to the package
definition so that they can be based on when conditions and/or attached
only to specific components which is not possible now.

Signed-off-by: Ryan Bottriell <rbottriell@ilm.com>

Author: rydrman

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

@@ -19,7 +19,15 @@ use spk_schema::foundation::name::OptNameBuf;
 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::{BuildIdent, ComponentFileMatchMode, ComponentSpecList, Package, PackageMut};
+use spk_schema::version::Compatibility;
+use spk_schema::{
+    BuildIdent,
+    ComponentFileMatchMode,
+    ComponentSpecList,
+    Package,
+    PackageMut,
+    RequirementsList,
+};
 use spk_solve::graph::Graph;
 use spk_solve::solution::Solution;
 use spk_solve::{BoxedResolverCallback, ResolverCallback, Solver};
@@ -264,7 +272,7 @@ where
         };
 
         tracing::debug!("Resolving build environment");
-        let solution = self.resolve_build_environment(&all_options).await?;
+        let (build_requirements, solution) = self.resolve_build_environment(&all_options).await?;
         self.environment
             .extend(solution.to_environment(Some(std::env::vars())));
 
@@ -285,6 +293,7 @@ where
         spfs::remount_runtime(&runtime).await?;
 
         let package = self.recipe.generate_binary_build(&all_options, &solution)?;
+        self.validate_generated_package(&build_requirements, &solution, &package)?;
         let components = self
             .build_and_commit_artifacts(&package, &all_options)
             .await?;
@@ -350,22 +359,68 @@ where
         Ok(solution?)
     }
 
-    async fn resolve_build_environment(&mut self, options: &OptionMap) -> Result<Solution> {
+    async fn resolve_build_environment(
+        &mut self,
+        options: &OptionMap,
+    ) -> Result<(RequirementsList, Solution)> {
         self.solver.reset();
         self.solver.update_options(options.clone());
         self.solver.set_binary_only(true);
         for repo in self.repos.iter().cloned() {
             self.solver.add_repository(repo);
         }
 
-        for request in self.recipe.get_build_requirements(options)? {
-            self.solver.add_request(request);
+        let build_requirements = self.recipe.get_build_requirements(options)?.into_owned();
+        for request in build_requirements.iter() {
+            self.solver.add_request(request.clone());
         }
 
         let mut runtime = self.solver.run();
         let solution = self.build_resolver.solve(&mut runtime).await;
         self.last_solve_graph = runtime.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().into_iter().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(problem) => {
+                            return Err(Error::MissingDownstreamBuildRequest {
+                                required_by: spec.ident().to_owned(),
+                                request: request.clone(),
+                                problem,
+                            })
+                        }
+                    }
+                }
+                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(problem) => {
+                            return Err(Error::MissingDownstreamRuntimeRequest {
+                                required_by: spec.ident().to_owned(),
+                                request: request.clone(),
+                                problem,
+                            })
+                        }
+                    }
+                }
+            }
+        }
+        Ok(())
     }
 
     async fn build_and_commit_artifacts(

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() {
@@ -314,12 +306,12 @@ 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"),
         _ => 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"),
         _ => panic!("expected var request"),
@@ -509,14 +501,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...",
             },
         }
@@ -528,14 +520,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(&*rt.tmprepo)
         .await
@@ -545,45 +537,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(&*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 == "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(&*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(&*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 == "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

@@ -350,7 +350,7 @@ impl Requests {
 
                     TestStage::Build => {
                         let requirements = recipe.get_build_requirements(&options)?;
-                        out.extend(requirements);
+                        out.extend(requirements.into_owned());
                     }
                     TestStage::Install => out.push(
                         PkgRequest::from_ident_exact(

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

@@ -113,6 +113,12 @@ impl std::ops::Not for &'_ Compatibility {
 }
 
 impl Compatibility {
+    /// Creates a compatibility instance denoting incompatibility
+    /// for the provided reason
+    pub fn incompatible<S: Into<String>>(reason: S) -> Self {
+        Self::Incompatible(reason.into())
+    }
+
     pub fn is_ok(&self) -> bool {
         matches!(self, &Compatibility::Compatible)
     }