From ee2259e19fadd81096972f96a40ec0e883f91ad8 Mon Sep 17 00:00:00 2001 From: Ryan Bottriell Date: Thu, 17 Nov 2022 18:02:41 -0800 Subject: [PATCH 1/8] 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. Signed-off-by: Ryan Bottriell --- crates/progress_bar_derive_macro/src/lib.rs | 2 +- crates/spk-build/src/build/binary.rs | 56 +++++++- crates/spk-build/src/build/binary_test.rs | 122 +++++++++++------- crates/spk-build/src/error.rs | 20 +++ crates/spk-cli/common/src/flags.rs | 2 +- .../crates/foundation/src/version/compat.rs | 6 +- crates/spk-schema/crates/ident/src/request.rs | 74 ++++++++++- crates/spk-schema/src/package.rs | 116 ++++++++++------- crates/spk-schema/src/recipe.rs | 9 +- crates/spk-schema/src/requirements_list.rs | 44 ++++++- crates/spk-schema/src/spec.rs | 34 +++-- crates/spk-schema/src/v0/spec.rs | 116 +++++++++++------ crates/spk-solve/crates/graph/src/graph.rs | 9 +- .../validation/src/impossible_checks.rs | 4 +- crates/spk-solve/src/solver.rs | 6 +- crates/spk-solve/src/solver_test.rs | 7 +- 16 files changed, 464 insertions(+), 163 deletions(-) diff --git a/crates/progress_bar_derive_macro/src/lib.rs b/crates/progress_bar_derive_macro/src/lib.rs index 4e41ec4a6b..b620145291 100644 --- a/crates/progress_bar_derive_macro/src/lib.rs +++ b/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 { diff --git a/crates/spk-build/src/build/binary.rs b/crates/spk-build/src/build/binary.rs index 2b7533180c..00ce08c113 100644 --- a/crates/spk-build/src/build/binary.rs +++ b/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 + ) -> 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 diff --git a/crates/spk-build/src/build/binary_test.rs b/crates/spk-build/src/build/binary_test.rs index 3862ba262f..fa16690c87 100644 --- a/crates/spk-build/src/build/binary_test.rs +++ b/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,12 +309,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"), @@ -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 == "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 == "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"), } } diff --git a/crates/spk-build/src/error.rs b/crates/spk-build/src/error.rs index 573f98d6d8..aed67cf691 100644 --- a/crates/spk-build/src/error.rs +++ b/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 = std::result::Result; @@ -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)] diff --git a/crates/spk-cli/common/src/flags.rs b/crates/spk-cli/common/src/flags.rs index b6e7b5ba21..d02ef44f7e 100644 --- a/crates/spk-cli/common/src/flags.rs +++ b/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 => { diff --git a/crates/spk-schema/crates/foundation/src/version/compat.rs b/crates/spk-schema/crates/foundation/src/version/compat.rs index 4e1e28f7ae..00bc408211 100644 --- a/crates/spk-schema/crates/foundation/src/version/compat.rs +++ b/crates/spk-schema/crates/foundation/src/version/compat.rs @@ -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())) } diff --git a/crates/spk-schema/crates/ident/src/request.rs b/crates/spk-schema/crates/ident/src/request.rs index 677a9981a8..57ea4bd10a 100644 --- a/crates/spk-schema/crates/ident/src/request.rs +++ b/crates/spk-schema/crates/ident/src/request.rs @@ -3,7 +3,7 @@ // https://github.com/imageworks/spk use std::cmp::min; use std::collections::BTreeMap; -use std::fmt::{Pointer, Write}; +use std::fmt::Write; use std::marker::PhantomData; use std::str::FromStr; @@ -325,6 +325,56 @@ impl VarRequest { { spec.check_satisfies_request(self) } + + /// True if this request is as least as restrictive as the other. In other words, + /// if satisfying this request would undoubtedly satisfy the other. + pub fn contains(&self, other: &Self) -> Compatibility { + if self.var.base_name() != other.var.base_name() { + return Compatibility::incompatible(format!( + "request is for a different var altogether [{} != {}]", + self.var, other.var + )); + } + let ns = self.var.namespace(); + if ns.is_some() && ns != other.var.namespace() { + return Compatibility::incompatible(format!( + "request specifies a different namespace [{} != {}]", + self.var, other.var + )); + } + if self.pin || other.pin { + // we cannot consider a request that still needs to be pinned as + // containing any other because the ultimate value of this request + // is unknown + return Compatibility::incompatible( + "fromBuildEnv requests cannot be reasonably compared".to_string(), + ); + } + if !other.value.is_empty() && self.value != other.value { + return Compatibility::incompatible(format!( + "requests require different values [{:?} != {:?}]", + self.value, other.value + )); + } + Compatibility::Compatible + } +} + +impl std::fmt::Display for VarRequest { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + // break apart to ensure that new fields are incorporated into this + // function if they are added in the future + let Self { var, value, pin } = self; + f.write_str("var: ")?; + var.fmt(f)?; + if !value.is_empty() { + f.write_char('/')?; + value.fmt(f)?; + } else if *pin { + f.write_str("/")?; + } + Ok(()) + } } impl Serialize for VarRequest { @@ -637,6 +687,28 @@ impl PkgRequest { satisfy.check_satisfies_request(self) } + /// True if this request is as least as restrictive as the other. In other words, + /// if satisfying this request would undoubtedly satisfy the other. + pub fn contains(&self, other: &Self) -> Compatibility { + let compat = self.pkg.contains(&other.pkg); + if !compat.is_ok() { + return compat; + } + if self.prerelease_policy > other.prerelease_policy { + return Compatibility::incompatible(format!( + "prerelease policy {} is more inclusive than {}", + self.prerelease_policy, other.prerelease_policy + )); + } + if self.inclusion_policy > other.inclusion_policy { + return Compatibility::incompatible(format!( + "inclusion policy {} is more inclusive than {}", + self.inclusion_policy, other.inclusion_policy + )); + } + Compatibility::Compatible + } + /// Reduce the scope of this request to the intersection with another. pub fn restrict(&mut self, other: &PkgRequest) -> Result<()> { self.prerelease_policy = min(self.prerelease_policy, other.prerelease_policy); diff --git a/crates/spk-schema/src/package.rs b/crates/spk-schema/src/package.rs index 7230aaa55c..97d89c67d7 100644 --- a/crates/spk-schema/src/package.rs +++ b/crates/spk-schema/src/package.rs @@ -2,10 +2,13 @@ // SPDX-License-Identifier: Apache-2.0 // https://github.com/imageworks/spk +use std::borrow::Cow; + use spk_schema_foundation::ident_build::Build; use spk_schema_foundation::spec_ops::{Named, Versioned}; use spk_schema_ident::BuildIdent; +use super::RequirementsList; use crate::foundation::ident_component::Component; use crate::foundation::option_map::OptionMap; use crate::foundation::version::Compatibility; @@ -30,9 +33,6 @@ pub trait Package: /// The values for this packages options used for this build. fn option_values(&self) -> OptionMap; - /// The input options for this package - fn options(&self) -> &Vec; - /// Return the location of sources for this package fn sources(&self) -> &Vec; @@ -56,7 +56,27 @@ pub trait Package: fn runtime_environment(&self) -> &Vec; /// Requests that must be met to use this package - fn runtime_requirements(&self) -> &super::RequirementsList; + fn runtime_requirements(&self) -> Cow<'_, RequirementsList>; + + /// Requests that must be satisfied by the build + /// environment of any package built against this one + /// + /// These requirements are not injected downstream, instead + /// they need to be present in the downstream package itself + fn downstream_build_requirements<'a>( + &self, + components: impl IntoIterator, + ) -> Cow<'_, RequirementsList>; + + /// Requests that must be satisfied by the runtime + /// environment of any package built against this one + /// + /// These requirements are not injected downstream, instead + /// they need to be present in the downstream package itself + fn downstream_runtime_requirements<'a>( + &self, + components: impl IntoIterator, + ) -> Cow<'_, RequirementsList>; /// Return the set of configured validators when building this package fn validation(&self) -> &super::ValidationSpec; @@ -65,33 +85,7 @@ pub trait Package: fn build_script(&self) -> String; /// Validate the given options against the options in this spec. - fn validate_options(&self, given_options: &OptionMap) -> Compatibility { - let mut must_exist = given_options.package_options_without_global(self.name()); - let given_options = given_options.package_options(self.name()); - for option in self.options().iter() { - let value = given_options - .get(option.full_name().without_namespace()) - .map(String::as_str); - let compat = option.validate(value); - if !compat.is_ok() { - return Compatibility::incompatible(format!( - "invalid value for {}: {compat}", - option.full_name(), - )); - } - - must_exist.remove(option.full_name().without_namespace()); - } - - if !must_exist.is_empty() { - let missing = must_exist; - return Compatibility::incompatible(format!( - "Package does not define requested build options: {missing:?}", - )); - } - - Compatibility::Compatible - } + fn validate_options(&self, given_options: &OptionMap) -> Compatibility; } pub trait PackageMut: Package + DeprecateMut { @@ -110,10 +104,6 @@ impl Package for std::sync::Arc { (**self).option_values() } - fn options(&self) -> &Vec { - (**self).options() - } - fn sources(&self) -> &Vec { (**self).sources() } @@ -136,10 +126,24 @@ impl Package for std::sync::Arc { (**self).runtime_environment() } - fn runtime_requirements(&self) -> &super::RequirementsList { + fn runtime_requirements(&self) -> Cow<'_, RequirementsList> { (**self).runtime_requirements() } + fn downstream_build_requirements<'a>( + &self, + components: impl IntoIterator, + ) -> Cow<'_, RequirementsList> { + (**self).downstream_build_requirements(components) + } + + fn downstream_runtime_requirements<'a>( + &self, + components: impl IntoIterator, + ) -> Cow<'_, RequirementsList> { + (**self).downstream_build_requirements(components) + } + fn validation(&self) -> &super::ValidationSpec { (**self).validation() } @@ -164,10 +168,6 @@ impl Package for Box { (**self).option_values() } - fn options(&self) -> &Vec { - (**self).options() - } - fn sources(&self) -> &Vec { (**self).sources() } @@ -190,10 +190,24 @@ impl Package for Box { (**self).runtime_environment() } - fn runtime_requirements(&self) -> &super::RequirementsList { + fn runtime_requirements(&self) -> Cow<'_, RequirementsList> { (**self).runtime_requirements() } + fn downstream_build_requirements<'a>( + &self, + components: impl IntoIterator, + ) -> Cow<'_, RequirementsList> { + (**self).downstream_build_requirements(components) + } + + fn downstream_runtime_requirements<'a>( + &self, + components: impl IntoIterator, + ) -> Cow<'_, RequirementsList> { + (**self).downstream_build_requirements(components) + } + fn validation(&self) -> &super::ValidationSpec { (**self).validation() } @@ -218,10 +232,6 @@ impl Package for &T { (**self).option_values() } - fn options(&self) -> &Vec { - (**self).options() - } - fn sources(&self) -> &Vec { (**self).sources() } @@ -244,10 +254,24 @@ impl Package for &T { (**self).runtime_environment() } - fn runtime_requirements(&self) -> &super::RequirementsList { + fn runtime_requirements(&self) -> Cow<'_, RequirementsList> { (**self).runtime_requirements() } + fn downstream_build_requirements<'a>( + &self, + components: impl IntoIterator, + ) -> Cow<'_, RequirementsList> { + (**self).downstream_build_requirements(components) + } + + fn downstream_runtime_requirements<'a>( + &self, + components: impl IntoIterator, + ) -> Cow<'_, RequirementsList> { + (**self).downstream_build_requirements(components) + } + fn validation(&self) -> &super::ValidationSpec { (**self).validation() } diff --git a/crates/spk-schema/src/recipe.rs b/crates/spk-schema/src/recipe.rs index 47829abdbd..5c53d6331b 100644 --- a/crates/spk-schema/src/recipe.rs +++ b/crates/spk-schema/src/recipe.rs @@ -9,8 +9,7 @@ use spk_schema_ident::VersionIdent; use crate::foundation::option_map::OptionMap; use crate::foundation::spec_ops::{Named, Versioned}; -use crate::ident::Request; -use crate::{Package, Result, TestStage, Variant}; +use crate::{Package, RequirementsList, Result, TestStage, Variant}; /// Return the resolved packages from a solution. pub trait BuildEnv { @@ -49,7 +48,7 @@ pub trait Recipe: /// /// This should also validate and include the items specified /// by [`Variant::additional_requirements`]. - fn get_build_requirements(&self, variant: &V) -> Result> + fn get_build_requirements(&self, variant: &V) -> Result> where V: Variant; @@ -92,7 +91,7 @@ where (**self).resolve_options(variant) } - fn get_build_requirements(&self, variant: &V) -> Result> + fn get_build_requirements(&self, variant: &V) -> Result> where V: Variant, { @@ -143,7 +142,7 @@ where (**self).resolve_options(variant) } - fn get_build_requirements(&self, variant: &V) -> Result> + fn get_build_requirements(&self, variant: &V) -> Result> where V: Variant, { diff --git a/crates/spk-schema/src/requirements_list.rs b/crates/spk-schema/src/requirements_list.rs index b4db9627a9..286cd07634 100644 --- a/crates/spk-schema/src/requirements_list.rs +++ b/crates/spk-schema/src/requirements_list.rs @@ -5,6 +5,7 @@ use std::collections::HashSet; use serde::{Deserialize, Serialize}; +use spk_schema_foundation::version::Compatibility; use spk_schema_ident::BuildIdent; use crate::foundation::option_map::OptionMap; @@ -49,6 +50,25 @@ impl RequirementsList { self.push(request); } + /// Reports whether the provided request would be satisfied by + /// this list of requests. The provided request does not need to + /// exist in this list exactly, so long as there is a request in this + /// list that is at least as restrictive + pub fn contains_request(&self, theirs: &Request) -> Compatibility { + for ours in self.iter() { + match (ours, theirs) { + (Request::Pkg(ours), Request::Pkg(theirs)) => { + return ours.contains(theirs); + } + (Request::Var(ours), Request::Var(theirs)) => { + return ours.contains(theirs); + } + _ => continue, + } + } + Compatibility::incompatible("No request exists for this") + } + /// Render all requests with a package pin using the given resolved packages. pub fn render_all_pins<'a>( &mut self, @@ -103,11 +123,29 @@ impl RequirementsList { } } -impl std::iter::IntoIterator for RequirementsList { - type IntoIter = std::vec::IntoIter; +impl Extend for RequirementsList +where + Vec: Extend, +{ + fn extend>(&mut self, iter: T) { + self.0.extend(iter) + } +} + +impl FromIterator for RequirementsList +where + Vec: FromIterator, +{ + fn from_iter>(iter: I) -> Self { + Self(Vec::from_iter(iter)) + } +} + +impl IntoIterator for RequirementsList { type Item = Request; + type IntoIter = std::vec::IntoIter; - fn into_iter(self) -> std::vec::IntoIter { + fn into_iter(self) -> Self::IntoIter { self.0.into_iter() } } diff --git a/crates/spk-schema/src/spec.rs b/crates/spk-schema/src/spec.rs index 7c2db3e8ee..5d38bd59b5 100644 --- a/crates/spk-schema/src/spec.rs +++ b/crates/spk-schema/src/spec.rs @@ -264,7 +264,7 @@ impl Recipe for SpecRecipe { } } - fn get_build_requirements(&self, variant: &V) -> Result> + fn get_build_requirements(&self, variant: &V) -> Result> where V: Variant, { @@ -487,12 +487,6 @@ impl Package for Spec { } } - fn options(&self) -> &Vec { - match self { - Spec::V0Package(spec) => spec.options(), - } - } - fn sources(&self) -> &Vec { match self { Spec::V0Package(spec) => spec.sources(), @@ -527,7 +521,7 @@ impl Package for Spec { } } - fn runtime_requirements(&self) -> &super::RequirementsList { + fn runtime_requirements(&self) -> Cow<'_, crate::RequirementsList> { match self { Spec::V0Package(spec) => spec.runtime_requirements(), } @@ -544,6 +538,30 @@ impl Package for Spec { Spec::V0Package(spec) => spec.build_script(), } } + + fn downstream_build_requirements<'a>( + &self, + components: impl IntoIterator, + ) -> Cow<'_, crate::RequirementsList> { + match self { + Spec::V0Package(spec) => spec.downstream_build_requirements(components), + } + } + + fn downstream_runtime_requirements<'a>( + &self, + components: impl IntoIterator, + ) -> Cow<'_, crate::RequirementsList> { + match self { + Spec::V0Package(spec) => spec.downstream_runtime_requirements(components), + } + } + + fn validate_options(&self, given_options: &OptionMap) -> Compatibility { + match self { + Spec::V0Package(spec) => spec.validate_options(given_options), + } + } } impl PackageMut for Spec { diff --git a/crates/spk-schema/src/v0/spec.rs b/crates/spk-schema/src/v0/spec.rs index b270f5daa7..db286e8599 100644 --- a/crates/spk-schema/src/v0/spec.rs +++ b/crates/spk-schema/src/v0/spec.rs @@ -193,10 +193,6 @@ impl Package for Spec { opts } - fn options(&self) -> &Vec { - &self.build.options - } - fn sources(&self) -> &Vec { &self.sources } @@ -229,8 +225,55 @@ impl Package for Spec { &self.install.environment } - fn runtime_requirements(&self) -> &RequirementsList { - &self.install.requirements + fn runtime_requirements(&self) -> Cow<'_, RequirementsList> { + Cow::Borrowed(&self.install.requirements) + } + + fn downstream_build_requirements<'a>( + &self, + _components: impl IntoIterator, + ) -> Cow<'_, RequirementsList> { + let requests = self + .build + .options + .iter() + .filter_map(|opt| match opt { + Opt::Var(v) => Some(v), + Opt::Pkg(_) => None, + }) + .filter(|o| o.inheritance != Inheritance::Weak) + .map(|o| { + let var = o.var.with_default_namespace(self.name()); + VarRequest { + var, + // we are assuming that the var here will have a value because + // this is a built binary package + value: o.get_value(None).unwrap_or_default(), + pin: false, + } + }) + .map(Request::Var) + .collect(); + Cow::Owned(requests) + } + + fn downstream_runtime_requirements<'a>( + &self, + _components: impl IntoIterator, + ) -> Cow<'_, RequirementsList> { + let requests = self + .build + .options + .iter() + .filter_map(|opt| match opt { + Opt::Var(v) => Some(v), + Opt::Pkg(_) => None, + }) + .filter(|o| o.inheritance == Inheritance::Strong) + .map(|o| VarRequest::new(o.var.with_default_namespace(self.name()))) + .map(Request::Var) + .collect(); + Cow::Owned(requests) } fn validation(&self) -> &ValidationSpec { @@ -240,6 +283,34 @@ impl Package for Spec { fn build_script(&self) -> String { self.build.script.join("\n") } + + fn validate_options(&self, given_options: &OptionMap) -> Compatibility { + let mut must_exist = given_options.package_options_without_global(self.name()); + let given_options = given_options.package_options(self.name()); + for option in self.build.options.iter() { + let value = given_options + .get(option.full_name().without_namespace()) + .map(String::as_str); + let compat = option.validate(value); + if !compat.is_ok() { + return Compatibility::incompatible(format!( + "invalid value for {}: {compat}", + option.full_name(), + )); + } + + must_exist.remove(option.full_name().without_namespace()); + } + + if !must_exist.is_empty() { + let missing = must_exist; + return Compatibility::incompatible(format!( + "Package does not define requested build options: {missing:?}", + )); + } + + Compatibility::Compatible + } } impl PackageMut for Spec { @@ -289,14 +360,14 @@ impl Recipe for Spec { Ok(resolved) } - fn get_build_requirements(&self, variant: &V) -> Result> + fn get_build_requirements(&self, variant: &V) -> Result> where V: Variant, { let opts = self.build.opts_for_variant(variant)?; let options = self.resolve_options(variant)?; let build_digest = Build::Digest(options.digest()); - let mut requests = Vec::new(); + let mut requests = RequirementsList::default(); for opt in opts { match opt { Opt::Pkg(opt) => { @@ -323,7 +394,7 @@ impl Recipe for Spec { } } } - Ok(requests) + Ok(Cow::Owned(requests)) } fn get_tests(&self, stage: TestStage, variant: &V) -> Result> @@ -379,33 +450,6 @@ impl Recipe for Spec { .into_iter() .map(|p| (p.name().to_owned(), p)) .collect(); - for (dep_name, dep_spec) in specs.iter() { - for opt in dep_spec.options().iter() { - if let Opt::Var(opt) = opt { - if let Inheritance::Weak = opt.inheritance { - continue; - } - let mut inherited_opt = opt.clone(); - if inherited_opt.var.namespace().is_none() { - inherited_opt.var = inherited_opt.var.with_namespace(dep_name); - } - inherited_opt.inheritance = Inheritance::Weak; - if let Inheritance::Strong = opt.inheritance { - let mut req = VarRequest::new(inherited_opt.var.clone()); - req.pin = true; - updated.install.upsert_requirement(Request::Var(req)); - } - updated.build.upsert_opt(Opt::Var(inherited_opt)); - } - } - } - - for e in updated.install.embedded.iter() { - updated - .build - .options - .extend(e.options().clone().into_iter()); - } for opt in updated.build.options.iter_mut() { match opt { diff --git a/crates/spk-solve/crates/graph/src/graph.rs b/crates/spk-solve/crates/graph/src/graph.rs index fb8e4bdb97..688b62ea6e 100644 --- a/crates/spk-solve/crates/graph/src/graph.rs +++ b/crates/spk-solve/crates/graph/src/graph.rs @@ -263,7 +263,7 @@ impl<'state, 'cmpt> DecisionBuilder<'state, 'cmpt> { let requester_ident: &BuildIdent = spec.ident(); let requested_by = RequestedBy::PackageBuild(requester_ident.clone()); changes - .extend(self.requirements_to_changes(spec.runtime_requirements(), &requested_by)); + .extend(self.requirements_to_changes(&spec.runtime_requirements(), &requested_by)); changes.extend(self.components_to_changes(spec.components(), requester_ident)); changes.extend(self.embedded_to_changes(spec.embedded(), requester_ident)); changes.push(Self::options_to_change(spec)); @@ -286,7 +286,7 @@ impl<'state, 'cmpt> DecisionBuilder<'state, 'cmpt> { let requester_ident: &BuildIdent = spec.ident(); let requested_by = RequestedBy::PackageBuild(requester_ident.clone()); - changes.extend(self.requirements_to_changes(spec.runtime_requirements(), &requested_by)); + changes.extend(self.requirements_to_changes(&spec.runtime_requirements(), &requested_by)); changes.extend(self.components_to_changes(spec.components(), requester_ident)); changes.extend(self.embedded_to_changes(spec.embedded(), requester_ident)); changes.push(Self::options_to_change(&spec)); @@ -447,10 +447,9 @@ impl<'state, 'cmpt> DecisionBuilder<'state, 'cmpt> { spec.name().as_opt_name().to_owned(), spec.compat().render(spec.version()), ); - for opt in spec.options() { - let value = opt.get_value(None); + for (name, value) in spec.option_values() { if !value.is_empty() { - let name = opt.full_name().with_default_namespace(spec.name()); + let name = name.with_default_namespace(spec.name()); opts.insert(name, value); } } diff --git a/crates/spk-solve/crates/validation/src/impossible_checks.rs b/crates/spk-solve/crates/validation/src/impossible_checks.rs index 3faeb7319b..18b534a4b5 100644 --- a/crates/spk-solve/crates/validation/src/impossible_checks.rs +++ b/crates/spk-solve/crates/validation/src/impossible_checks.rs @@ -14,7 +14,7 @@ use spk_schema::foundation::name::{PkgName, PkgNameBuf}; use spk_schema::foundation::version::Compatibility; use spk_schema::ident::{InclusionPolicy, PkgRequest, RangeIdent, Request}; use spk_schema::spec_ops::Versioned; -use spk_schema::{AnyIdent, BuildIdent, Package, RequirementsList, Spec}; +use spk_schema::{AnyIdent, BuildIdent, Package, Spec}; use spk_solve_graph::{GetMergedRequestError, GetMergedRequestResult}; use spk_solve_solution::PackageSource; use spk_storage::RepositoryHandle; @@ -265,7 +265,7 @@ impl ImpossibleRequestsChecker { unresolved_requests: &HashMap, repos: &[Arc], ) -> Result { - let requirements: &RequirementsList = package.runtime_requirements(); + let requirements = package.runtime_requirements(); if requirements.is_empty() { return Ok(Compatibility::Compatible); } diff --git a/crates/spk-solve/src/solver.rs b/crates/spk-solve/src/solver.rs index efc8b616b1..849b86874a 100644 --- a/crates/spk-solve/src/solver.rs +++ b/crates/spk-solve/src/solver.rs @@ -986,7 +986,11 @@ impl Solver { let state = self.get_initial_state(); let build_options = recipe.resolve_options(state.get_option_map())?; - for req in recipe.get_build_requirements(&build_options)? { + for req in recipe + .get_build_requirements(&build_options)? + .iter() + .cloned() + { self.add_request(req) } diff --git a/crates/spk-solve/src/solver_test.rs b/crates/spk-solve/src/solver_test.rs index 9e82b3080c..0b37154b90 100644 --- a/crates/spk-solve/src/solver_test.rs +++ b/crates/spk-solve/src/solver_test.rs @@ -721,8 +721,11 @@ async fn test_solver_option_compatibility(mut solver: Solver) { let solution = run_and_print_resolve_for_tests(&solver).await.unwrap(); let resolved = solution.get("vnp3").unwrap(); - let opt = resolved.spec.options().get(0).unwrap(); - let value = opt.get_value(None); + let value = resolved + .spec + .option_values() + .remove(opt_name!("python")) + .unwrap(); // Check the first digit component of the pyver value let expected = if pyver.starts_with('~') { From 70c7040024e6359c3399466cdd06d0852d0cb37e Mon Sep 17 00:00:00 2001 From: Ryan Bottriell Date: Tue, 13 Jun 2023 12:35:18 -0700 Subject: [PATCH 2/8] Remove unused function from install spec Signed-off-by: Ryan Bottriell --- crates/spk-schema/src/install_spec.rs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/crates/spk-schema/src/install_spec.rs b/crates/spk-schema/src/install_spec.rs index 7687898c1f..d9af044a22 100644 --- a/crates/spk-schema/src/install_spec.rs +++ b/crates/spk-schema/src/install_spec.rs @@ -6,7 +6,6 @@ use spk_schema_ident::BuildIdent; use super::{ComponentSpecList, EmbeddedPackagesList, EnvOp, RequirementsList}; use crate::foundation::option_map::OptionMap; -use crate::ident::Request; use crate::Result; #[cfg(test)] @@ -27,14 +26,6 @@ pub struct InstallSpec { } impl InstallSpec { - /// Add or update a requirement to the set of installation requirements. - /// - /// If a request exists for the same name, it is replaced with the given - /// one. Otherwise the new request is appended to the list. - pub fn upsert_requirement(&mut self, request: Request) { - self.requirements.upsert(request); - } - pub fn is_default(&self) -> bool { self.requirements.is_empty() && self.embedded.is_empty() && self.components.is_default() } From ee89d8f8cf7df548e3370793575b0b95d329007b Mon Sep 17 00:00:00 2001 From: Ryan Bottriell Date: Tue, 13 Jun 2023 12:35:53 -0700 Subject: [PATCH 3/8] Revamp RequirementsList to ensure no duplicate request names invariant Signed-off-by: Ryan Bottriell --- crates/spk-schema/src/requirements_list.rs | 84 ++++++++++++++-------- crates/spk-schema/src/v0/spec.rs | 18 ++--- crates/spk-schema/src/v0/variant.rs | 6 +- 3 files changed, 68 insertions(+), 40 deletions(-) diff --git a/crates/spk-schema/src/requirements_list.rs b/crates/spk-schema/src/requirements_list.rs index 286cd07634..d14e9598b4 100644 --- a/crates/spk-schema/src/requirements_list.rs +++ b/crates/spk-schema/src/requirements_list.rs @@ -17,6 +17,10 @@ use crate::{Error, Result}; mod requirements_list_test; /// A set of installation requirements. +/// +/// Requirements lists cannot contain multiple requests with the +/// same name, requiring instead that they be combined into a single +/// request as needed. #[derive(Clone, Debug, Default, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)] #[serde(transparent)] pub struct RequirementsList(Vec); @@ -28,26 +32,46 @@ impl std::ops::Deref for RequirementsList { } } -impl std::ops::DerefMut for RequirementsList { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.0 - } -} - impl RequirementsList { /// Add or update a requirement in this list. /// - /// If a request exists for the same name, it is replaced with the given - /// one. Otherwise the new request is appended to the list. - pub fn upsert(&mut self, request: Request) { + /// If a request exists for the same name, it is replaced with the + /// given one. Otherwise the new request is appended to the list. + /// Returns the replaced request, if any. + pub fn insert_or_replace(&mut self, request: Request) -> Option { let name = request.name(); - for other in self.iter_mut() { - if other.name() == name { - let _ = std::mem::replace(other, request); - return; + for existing in self.0.iter_mut() { + if existing.name() == name { + return Some(std::mem::replace(existing, request)); } } - self.push(request); + self.0.push(request); + None + } + + /// Add a requirement in this list, or merge it in. + /// + /// If a request exists for the same name, it is updated with the + /// restrictions of this one one. Otherwise the new request is + /// appended to the list. Returns the newly inserted or updated request. + pub fn insert_or_merge(&mut self, request: Request) -> Result<()> { + let name = request.name(); + for existing in self.0.iter_mut() { + if existing.name() != name { + continue; + } + match (existing, &request) { + (Request::Pkg(existing), Request::Pkg(request)) => { + existing.restrict(request)?; + } + (existing, _) => { + return Err(Error::String(format!("Cannot insert requirement: one already exists and only pkg requests can be merged: {existing} + {request}"))) + } + } + return Ok(()); + } + self.0.push(request); + Ok(()) } /// Reports whether the provided request would be satisfied by @@ -79,7 +103,7 @@ impl RequirementsList { for pkg in resolved { by_name.insert(pkg.name(), pkg); } - for request in self.iter_mut() { + for request in self.0.iter_mut() { match request { Request::Pkg(request) => { if request.pin.is_none() { @@ -121,23 +145,25 @@ impl RequirementsList { } Ok(()) } -} -impl Extend for RequirementsList -where - Vec: Extend, -{ - fn extend>(&mut self, iter: T) { - self.0.extend(iter) + /// Attempt to build a requirements list from a set of requests. + /// + /// Duplicate requests will be merged. Any error during this process + /// will cause this process to fail. + pub fn try_from_iter(value: I) -> Result + where + I: IntoIterator, + { + let mut out = Self::default(); + for item in value.into_iter() { + out.insert_or_merge(item)?; + } + Ok(out) } -} -impl FromIterator for RequirementsList -where - Vec: FromIterator, -{ - fn from_iter>(iter: I) -> Self { - Self(Vec::from_iter(iter)) + /// Remove all requests from this list + pub fn clear(&mut self) { + self.0.clear() } } diff --git a/crates/spk-schema/src/v0/spec.rs b/crates/spk-schema/src/v0/spec.rs index db286e8599..1e9aafb1db 100644 --- a/crates/spk-schema/src/v0/spec.rs +++ b/crates/spk-schema/src/v0/spec.rs @@ -252,9 +252,10 @@ impl Package for Spec { pin: false, } }) - .map(Request::Var) - .collect(); - Cow::Owned(requests) + .map(Request::Var); + RequirementsList::try_from_iter(requests) + .map(Cow::Owned) + .expect("build opts do not contain duplicates") } fn downstream_runtime_requirements<'a>( @@ -271,9 +272,10 @@ impl Package for Spec { }) .filter(|o| o.inheritance == Inheritance::Strong) .map(|o| VarRequest::new(o.var.with_default_namespace(self.name()))) - .map(Request::Var) - .collect(); - Cow::Owned(requests) + .map(Request::Var); + RequirementsList::try_from_iter(requests) + .map(Cow::Owned) + .expect("build opts do not contain duplicates") } fn validation(&self) -> &ValidationSpec { @@ -380,7 +382,7 @@ impl Recipe for Spec { // inject the default component for this context if needed req.pkg.components.insert(Component::default_for_build()); } - requests.push(req.into()); + requests.insert_or_merge(req.into())?; } Opt::Var(opt) => { // If no value was specified in the spec, there's @@ -388,7 +390,7 @@ impl Recipe for Spec { // find a var with an empty value. if let Some(value) = options.get(&opt.var) { if !value.is_empty() { - requests.push(opt.to_request(Some(value)).into()); + requests.insert_or_merge(opt.to_request(Some(value)).into())?; } } } diff --git a/crates/spk-schema/src/v0/variant.rs b/crates/spk-schema/src/v0/variant.rs index 10b3933839..bc499235c8 100644 --- a/crates/spk-schema/src/v0/variant.rs +++ b/crates/spk-schema/src/v0/variant.rs @@ -47,19 +47,19 @@ impl Variant { // // If it is not a valid package name, assume it is a var. let Ok(pkg_name) = PkgName::new(name) else { - requirements.push(VarRequest::new_with_value(name.clone(), value).into()); + requirements.insert_or_replace(VarRequest::new_with_value(name.clone(), value).into()); continue; }; // If the value is not a legal version range, assume it is // a var. let Ok(version_range) = VersionRange::from_str(value) else { - requirements.push(VarRequest::new_with_value(name.clone(), value).into()); + requirements.insert_or_replace(VarRequest::new_with_value(name.clone(), value).into()); continue; }; // It is a valid package name and the value is a legal // version range expression, and it doesn't match any // declared options. Treat as a new package request - requirements.push( + requirements.insert_or_replace( PkgRequest::new( RangeIdent { name: pkg_name.to_owned(), From ebccd0a24457e647175f039f7c396e9528aef7e7 Mon Sep 17 00:00:00 2001 From: Ryan Bottriell Date: Wed, 14 Jun 2023 08:58:35 -0700 Subject: [PATCH 4/8] Replace pin bool with enum The VarRequest had a pin field used to denote when fromBuildEnv was given and this field was mutually exclusive to a provided value. An enum works better in this case. Ideally, the package would not allow this field to be pinned, only the recipe. For now, I have not made that change only this first one to introduce an enum. Signed-off-by: Ryan Bottriell --- crates/spk-build/src/build/binary_test.rs | 8 +- crates/spk-schema/crates/ident/src/lib.rs | 1 + crates/spk-schema/crates/ident/src/request.rs | 181 ++++++++++++------ .../crates/ident/src/request_test.rs | 2 +- crates/spk-schema/src/option.rs | 9 +- crates/spk-schema/src/requirements_list.rs | 2 +- crates/spk-schema/src/spec_test.rs | 2 +- crates/spk-schema/src/v0/spec.rs | 14 +- crates/spk-schema/src/v0/variant.rs | 4 +- crates/spk-solve/crates/graph/src/graph.rs | 18 +- .../src/validators/var_requirements.rs | 7 +- crates/spk-solve/src/io.rs | 2 +- crates/spk-solve/src/solver.rs | 9 +- crates/spk-solve/src/solver_test.rs | 4 +- 14 files changed, 171 insertions(+), 92 deletions(-) diff --git a/crates/spk-build/src/build/binary_test.rs b/crates/spk-build/src/build/binary_test.rs index fa16690c87..3651273792 100644 --- a/crates/spk-build/src/build/binary_test.rs +++ b/crates/spk-build/src/build/binary_test.rs @@ -311,12 +311,12 @@ async fn test_build_var_pinning() { let spec = rt.tmprepo.read_package(spec.ident()).await.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().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"), } } @@ -560,7 +560,7 @@ async fn test_build_package_downstream_build_requests() { }) => { assert_eq!(&required_by, base_pkg.ident()); assert!( - matches!(&request, Request::Var(v) if v.var.as_str() == "base.inherited" && v.value == "val"), + matches!(&request, Request::Var(v) if v.var.as_str() == "base.inherited" && v.value.as_pinned() == Some("val")), "{request}" ); } @@ -620,7 +620,7 @@ async fn test_build_package_downstream_runtime_request() { }) => { assert_eq!(&required_by, base_pkg.ident()); assert!( - matches!(&request, Request::Var(v) if v.var.as_str() == "base.inherited" && v.value == "val"), + matches!(&request, Request::Var(v) if v.var.as_str() == "base.inherited" && v.value.as_pinned() == Some("val")), "{request}" ); } diff --git a/crates/spk-schema/crates/ident/src/lib.rs b/crates/spk-schema/crates/ident/src/lib.rs index 47ffddfa1f..0426ccc185 100644 --- a/crates/spk-schema/crates/ident/src/lib.rs +++ b/crates/spk-schema/crates/ident/src/lib.rs @@ -28,6 +28,7 @@ pub use request::{ is_false, InclusionPolicy, NameAndValue, + PinnableValue, PkgRequest, PreReleasePolicy, Request, diff --git a/crates/spk-schema/crates/ident/src/request.rs b/crates/spk-schema/crates/ident/src/request.rs index 57ea4bd10a..28846152d5 100644 --- a/crates/spk-schema/crates/ident/src/request.rs +++ b/crates/spk-schema/crates/ident/src/request.rs @@ -95,7 +95,7 @@ impl std::str::FromStr for InclusionPolicy { #[serde(untagged)] pub enum Request { Pkg(PkgRequest), - Var(VarRequest), + Var(VarRequest), } impl Request { @@ -235,21 +235,11 @@ impl<'de> Deserialize<'de> for Request { requested_by: Default::default(), })), (None, Some(var)) => { - match self.value.as_ref() { - Some(value) if self.pin.as_ref().map(PinValue::is_some).unwrap_or_default() => { - Err(serde::de::Error::custom( - format!("request for `{var}` cannot specify a value `/{value}` when `fromBuildEnv` is true") - )) - } - None if self.pin.is_none() => Err(serde::de::Error::custom( - format!("request for `{var}` must specify a value (eg: {var}/) when `fromBuildEnv` is false or omitted") - )), - _ => Ok(Request::Var(VarRequest { - var, - pin: self.pin.unwrap_or_default().into_var_pin()?, - value: self.value.unwrap_or_default(), - })) - } + let value = self.pin.unwrap_or_default().into_var_pin(&var, self.value.take())?; + Ok(Request::Var(VarRequest { + var, + value, + })) }, (Some(_), Some(_)) => Err(serde::de::Error::custom( "could not determine request type, it may only contain one of the `pkg` or `var` fields" @@ -268,54 +258,62 @@ impl<'de> Deserialize<'de> for Request { /// A set of restrictions placed on selected packages' build options. #[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] -pub struct VarRequest { +pub struct VarRequest { pub var: OptNameBuf, - pub pin: bool, - pub value: String, -} - -#[derive(Serialize, Deserialize)] -struct VarRequestSchema { - var: String, - #[serde(rename = "fromBuildEnv", default, skip_serializing_if = "is_false")] - pin: bool, + pub value: T, } -impl VarRequest { +impl VarRequest { /// Create a new empty request for the named variable pub fn new>(name: S) -> Self { Self { var: name.into(), - pin: false, value: Default::default(), } } +} +impl VarRequest { /// Create a new request for the named variable at the specified value pub fn new_with_value(name: N, value: V) -> Self where N: Into, - V: Into, + V: Into, { Self { var: name.into(), - pin: false, value: value.into(), } } +} +impl VarRequest { /// Create a copy of this request with it's pin rendered out using 'var'. pub fn render_pin>(&self, value: S) -> Result { - if !self.pin { + if !self.value.is_from_build_env() { return Err(Error::String( "Request has no pin to be rendered".to_string(), )); } - let mut new = self.clone(); - new.pin = false; - new.value = value.into(); - Ok(new) + Ok(VarRequest { + var: self.var.clone(), + value: PinnableValue::Pinned(value.into()), + }) + } + + /// Create a copy of this request with it's pin rendered out using 'var'. + pub fn into_pinned>(self, value: S) -> Result> { + if !self.value.is_from_build_env() { + return Err(Error::String( + "Request has no pin to be rendered".to_string(), + )); + } + + Ok(VarRequest { + var: self.var, + value: value.into(), + }) } /// Check if this package spec satisfies the given var request. @@ -342,18 +340,17 @@ impl VarRequest { self.var, other.var )); } - if self.pin || other.pin { + let (Some(self_value), Some(other_value)) = (&self.value.as_pinned(), &other.value.as_pinned()) else{ // we cannot consider a request that still needs to be pinned as // containing any other because the ultimate value of this request // is unknown return Compatibility::incompatible( "fromBuildEnv requests cannot be reasonably compared".to_string(), ); - } - if !other.value.is_empty() && self.value != other.value { + }; + if !other_value.is_empty() && self_value != other_value { return Compatibility::incompatible(format!( - "requests require different values [{:?} != {:?}]", - self.value, other.value + "requests require different values [{self_value:?} != {other_value:?}]", )); } Compatibility::Compatible @@ -364,32 +361,80 @@ impl std::fmt::Display for VarRequest { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { // break apart to ensure that new fields are incorporated into this // function if they are added in the future - let Self { var, value, pin } = self; + let Self { var, value } = self; f.write_str("var: ")?; var.fmt(f)?; - if !value.is_empty() { - f.write_char('/')?; - value.fmt(f)?; - } else if *pin { - f.write_str("/")?; + match value.as_pinned() { + Some(v) => { + f.write_char('/')?; + v.fmt(f)?; + } + None => { + f.write_str("/")?; + } } Ok(()) } } -impl Serialize for VarRequest { +impl Serialize for VarRequest { fn serialize(&self, serializer: S) -> std::result::Result where S: serde::Serializer, { - let mut var = self.var.to_string(); - if !self.value.is_empty() || !self.pin { - // serialize an empty value if not pinning, otherwise it - // wont be valid to load back in - var = format!("{}/{}", var, self.value); + use serde::ser::SerializeMap; + + let mut map = serializer.serialize_map(Some(2))?; + + match &self.value { + PinnableValue::FromBuildEnv => { + map.serialize_entry("var", &self.var)?; + map.serialize_entry("fromBuildEnv", &true)?; + } + PinnableValue::Pinned(v) => { + let var = format!("{}/{v}", self.var); + map.serialize_entry("var", &var)?; + } } - let out = VarRequestSchema { var, pin: self.pin }; - out.serialize(serializer) + map.end() + } +} + +/// A value that is either set to a string or requested +/// to be pinned using the value at build time +#[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] +pub enum PinnableValue { + FromBuildEnv, + Pinned(String), +} + +impl PinnableValue { + pub fn is_from_build_env(&self) -> bool { + matches!(self, Self::FromBuildEnv) + } + + pub fn is_pinned(&self) -> bool { + matches!(self, Self::FromBuildEnv) + } + + /// The current pinned value, if any + pub fn as_pinned(&self) -> Option<&str> { + match self { + Self::FromBuildEnv => None, + Self::Pinned(v) => Some(v.as_str()), + } + } +} + +impl Default for PinnableValue { + fn default() -> Self { + Self::Pinned(String::new()) + } +} + +impl From for PinnableValue { + fn from(value: String) -> Self { + Self::Pinned(value) } } @@ -902,16 +947,30 @@ impl PinValue { } /// Transform this pin into the appropriate value for a var request, if possible - fn into_var_pin(self) -> std::result::Result + fn into_var_pin( + self, + var: &OptName, + value: Option, + ) -> std::result::Result where E: serde::de::Error, { - match self { - Self::None => Ok(false), - Self::True => Ok(true), - Self::String(s) => Err(E::custom(format!( - "`fromBuildEnv` for var requests must be a boolean, found `{s}`" - ))), + match (value, self) { + (Some(value), Self::True) => { + Err(E::custom( + format!("request for `{var}` cannot specify a value `/{value}` when `fromBuildEnv` is true") + )) + } + (None, Self::None) => Err(E::custom( + format!("request for `{var}` must specify a value (eg: {var}/) when `fromBuildEnv` is false or omitted") + )), + (Some(value), Self::None) => { + Ok(PinnableValue::Pinned(value)) + } + (None, Self::True) => Ok(PinnableValue::FromBuildEnv), + (_, Self::String(s)) => Err(E::custom(format!( + "`fromBuildEnv` for var request `{var}` must be a boolean, found `{s}`" + ))) } } diff --git a/crates/spk-schema/crates/ident/src/request_test.rs b/crates/spk-schema/crates/ident/src/request_test.rs index d95ce09637..6eb99a5fb7 100644 --- a/crates/spk-schema/crates/ident/src/request_test.rs +++ b/crates/spk-schema/crates/ident/src/request_test.rs @@ -173,7 +173,7 @@ fn test_var_request_pinned_roundtrip() { "should be able to round-trip serialize a var request with pin" ); assert!( - res.unwrap().into_var().unwrap().pin, + res.unwrap().into_var().unwrap().value.is_from_build_env(), "should preserve pin value" ); } diff --git a/crates/spk-schema/src/option.rs b/crates/spk-schema/src/option.rs index fd640385f7..2b0f7ea36d 100644 --- a/crates/spk-schema/src/option.rs +++ b/crates/spk-schema/src/option.rs @@ -9,7 +9,7 @@ use indexmap::set::IndexSet; use serde::{Deserialize, Serialize}; use spk_schema_foundation::ident_component::ComponentBTreeSet; use spk_schema_foundation::option_map::Stringified; -use spk_schema_ident::{NameAndValue, RangeIdent}; +use spk_schema_ident::{NameAndValue, PinnableValue, RangeIdent}; use crate::foundation::name::{OptName, OptNameBuf, PkgName, PkgNameBuf}; use crate::foundation::version::{CompatRule, Compatibility}; @@ -175,9 +175,9 @@ impl TryFrom for Opt { required_compat: request.required_compat, })) } - Request::Var(VarRequest { var, pin: _, value }) => Ok(Opt::Var(VarOpt { + Request::Var(VarRequest { var, value }) => Ok(Opt::Var(VarOpt { var, - default: value, + default: value.as_pinned().map(str::to_string).unwrap_or_default(), choices: Default::default(), inheritance: Default::default(), value: None, @@ -438,8 +438,7 @@ impl VarOpt { let value = self.get_value(given_value).unwrap_or_default(); VarRequest { var: self.var.clone(), - value, - pin: false, + value: PinnableValue::Pinned(value), } } } diff --git a/crates/spk-schema/src/requirements_list.rs b/crates/spk-schema/src/requirements_list.rs index d14e9598b4..411fe47bba 100644 --- a/crates/spk-schema/src/requirements_list.rs +++ b/crates/spk-schema/src/requirements_list.rs @@ -122,7 +122,7 @@ impl RequirementsList { } } Request::Var(request) => { - if !request.pin { + if !request.value.is_from_build_env() { continue; } let opts = match request.var.namespace() { diff --git a/crates/spk-schema/src/spec_test.rs b/crates/spk-schema/src/spec_test.rs index 4ff4df60c6..3a2362c449 100644 --- a/crates/spk-schema/src/spec_test.rs +++ b/crates/spk-schema/src/spec_test.rs @@ -151,7 +151,7 @@ macro_rules! assert_requests_contains { if !$requests .iter() .enumerate() - .any(|(index, r)| matches!(r, $crate::Request::Var(var) if &var.var == $expected_key && var.value == $expected_value && ($expected_index.is_none() || $expected_index.unwrap() == index))) + .any(|(index, r)| matches!(r, $crate::Request::Var(var) if &var.var == $expected_key && var.value.as_pinned().as_deref() == Some($expected_value) && ($expected_index.is_none() || $expected_index.unwrap() == index))) { panic!( "requests did not contain var with {} and {}{}", diff --git a/crates/spk-schema/src/v0/spec.rs b/crates/spk-schema/src/v0/spec.rs index 1e9aafb1db..95033c8379 100644 --- a/crates/spk-schema/src/v0/spec.rs +++ b/crates/spk-schema/src/v0/spec.rs @@ -11,7 +11,7 @@ use itertools::Itertools; use serde::{Deserialize, Serialize}; use spk_schema_foundation::name::PkgNameBuf; use spk_schema_foundation::option_map::Stringified; -use spk_schema_ident::{AnyIdent, BuildIdent, Ident, VersionIdent}; +use spk_schema_ident::{AnyIdent, BuildIdent, Ident, PinnableValue, VersionIdent}; use super::TestSpec; use crate::build_spec::UncheckedBuildSpec; @@ -248,8 +248,7 @@ impl Package for Spec { var, // we are assuming that the var here will have a value because // this is a built binary package - value: o.get_value(None).unwrap_or_default(), - pin: false, + value: PinnableValue::Pinned(o.get_value(None).unwrap_or_default()), } }) .map(Request::Var); @@ -605,15 +604,16 @@ where } Compatibility::Compatible } - Some(Opt::Pkg(opt)) => opt.validate(Some(&var_request.value)), + Some(Opt::Pkg(opt)) => opt.validate(var_request.value.as_pinned()), Some(Opt::Var(opt)) => { - let exact = opt.get_value(Some(&var_request.value)); - if exact.as_deref() != Some(&var_request.value) { + let request_value = var_request.value.as_pinned(); + let exact = opt.get_value(request_value); + if exact.as_deref() != request_value { Compatibility::incompatible(format!( "Incompatible build option '{}': '{}' != '{}'", var_request.var, exact.unwrap_or_else(|| "None".to_string()), - var_request.value + request_value.unwrap_or_default() )) } else { Compatibility::Compatible diff --git a/crates/spk-schema/src/v0/variant.rs b/crates/spk-schema/src/v0/variant.rs index bc499235c8..0fb99ce445 100644 --- a/crates/spk-schema/src/v0/variant.rs +++ b/crates/spk-schema/src/v0/variant.rs @@ -47,13 +47,13 @@ impl Variant { // // If it is not a valid package name, assume it is a var. let Ok(pkg_name) = PkgName::new(name) else { - requirements.insert_or_replace(VarRequest::new_with_value(name.clone(), value).into()); + requirements.insert_or_replace(VarRequest::new_with_value(name.clone(), value.clone()).into()); continue; }; // If the value is not a legal version range, assume it is // a var. let Ok(version_range) = VersionRange::from_str(value) else { - requirements.insert_or_replace(VarRequest::new_with_value(name.clone(), value).into()); + requirements.insert_or_replace(VarRequest::new_with_value(name.clone(), value.clone()).into()); continue; }; // It is a valid package name and the value is a legal diff --git a/crates/spk-solve/crates/graph/src/graph.rs b/crates/spk-solve/crates/graph/src/graph.rs index 688b62ea6e..afd9cc1cc5 100644 --- a/crates/spk-solve/crates/graph/src/graph.rs +++ b/crates/spk-solve/crates/graph/src/graph.rs @@ -119,10 +119,13 @@ impl FormatChange for Change { format!( "{} {}{}", Self::get_request_change_label(format_settings.level).blue(), - option_map! {c.request.var.clone() => c.request.value.clone()} + option_map! {c.request.var.clone() => c.request.value.as_pinned().unwrap_or_default()} .format_option_map(), if format_settings.verbosity > PkgRequest::SHOW_REQUEST_DETAILS { - format!(" fromBuildEnv: {}", c.request.pin.to_string().cyan()) + format!( + " fromBuildEnv: {}", + c.request.value.is_from_build_env().to_string().cyan() + ) } else { "".to_string() } @@ -890,7 +893,16 @@ impl RequestVar { } let options = SetOptions::compute_new_options( base, - vec![(&self.request.var, &self.request.value)].into_iter(), + vec![( + &self.request.var, + &self + .request + .value + .as_pinned() + .map(str::to_string) + .unwrap_or_default(), + )] + .into_iter(), true, ); Arc::new(base.with_var_requests_and_options(parent, new_requests, options)) diff --git a/crates/spk-solve/crates/validation/src/validators/var_requirements.rs b/crates/spk-solve/crates/validation/src/validators/var_requirements.rs index ef4eec1f27..4843e54fc3 100644 --- a/crates/spk-solve/crates/validation/src/validators/var_requirements.rs +++ b/crates/spk-solve/crates/validation/src/validators/var_requirements.rs @@ -29,10 +29,11 @@ impl ValidatorT for VarRequirementsValidator { // empty option values do not provide a valuable opinion on the resolve continue; } - if request.value != *value { + let requested = request.value.as_pinned().unwrap_or_default(); + if requested != value.as_str() { return Ok(Compatibility::incompatible(format!( - "package wants {}={}, resolve has {}={}", - request.var, request.value, name, value, + "package wants {}={requested}, resolve has {name}={value}", + request.var ))); } } diff --git a/crates/spk-solve/src/io.rs b/crates/spk-solve/src/io.rs index 9c87236b4d..6a82a8828d 100644 --- a/crates/spk-solve/src/io.rs +++ b/crates/spk-solve/src/io.rs @@ -267,7 +267,7 @@ where node.state .get_var_requests() .iter() - .map(|v| format!("{}: {}", v.var, v.value)) + .map(|v| format!("{}: {}", v.var, v.value.as_pinned().unwrap_or_default())) .collect::>() .join(", ") )); diff --git a/crates/spk-solve/src/solver.rs b/crates/spk-solve/src/solver.rs index 849b86874a..aa5e34d80e 100644 --- a/crates/spk-solve/src/solver.rs +++ b/crates/spk-solve/src/solver.rs @@ -361,7 +361,14 @@ impl Solver { } for var_request in state.get_var_requests() { if !opts.contains_key(&var_request.var) { - opts.insert(var_request.var.clone(), var_request.value.clone()); + opts.insert( + var_request.var.clone(), + var_request + .value + .as_pinned() + .unwrap_or_default() + .to_string(), + ); } } diff --git a/crates/spk-solve/src/solver_test.rs b/crates/spk-solve/src/solver_test.rs index 0b37154b90..1f171743ac 100644 --- a/crates/spk-solve/src/solver_test.rs +++ b/crates/spk-solve/src/solver_test.rs @@ -13,6 +13,7 @@ use spk_schema::ident::{ build_ident, parse_ident_range, version_ident, + PinnableValue, PkgRequest, RangeIdent, Request, @@ -712,8 +713,7 @@ async fn test_solver_option_compatibility(mut solver: Solver) { solver.add_request( VarRequest { var: opt_name!("python").to_owned(), - pin: false, - value: pyver.to_string(), + value: PinnableValue::Pinned(pyver.to_string()), } .into(), ); From 3ec323e4e2856cbe9be32f11933f90b8b0ae6f7b Mon Sep 17 00:00:00 2001 From: Ryan Bottriell Date: Mon, 19 Jun 2023 15:16:58 -0700 Subject: [PATCH 5/8] Fix typos in old doc comments Signed-off-by: Ryan Bottriell --- crates/spk-schema/crates/ident/src/request.rs | 8 ++++---- crates/spk-schema/src/requirements_list.rs | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/spk-schema/crates/ident/src/request.rs b/crates/spk-schema/crates/ident/src/request.rs index 28846152d5..7e291bae85 100644 --- a/crates/spk-schema/crates/ident/src/request.rs +++ b/crates/spk-schema/crates/ident/src/request.rs @@ -288,7 +288,7 @@ impl VarRequest { } impl VarRequest { - /// Create a copy of this request with it's pin rendered out using 'var'. + /// Create a copy of this request with its pin rendered out using 'var'. pub fn render_pin>(&self, value: S) -> Result { if !self.value.is_from_build_env() { return Err(Error::String( @@ -302,7 +302,7 @@ impl VarRequest { }) } - /// Create a copy of this request with it's pin rendered out using 'var'. + /// Create a copy of this request with its pin rendered out using 'var'. pub fn into_pinned>(self, value: S) -> Result> { if !self.value.is_from_build_env() { return Err(Error::String( @@ -340,7 +340,7 @@ impl VarRequest { self.var, other.var )); } - let (Some(self_value), Some(other_value)) = (&self.value.as_pinned(), &other.value.as_pinned()) else{ + let (Some(self_value), Some(other_value)) = (&self.value.as_pinned(), &other.value.as_pinned()) else { // we cannot consider a request that still needs to be pinned as // containing any other because the ultimate value of this request // is unknown @@ -674,7 +674,7 @@ impl PkgRequest { Ok(new) } - /// Create a copy of this request with it's pin rendered out using 'pkg'. + /// Create a copy of this request with its pin rendered out using 'pkg'. pub fn render_pin(&self, pkg: &BuildIdent) -> Result { match &self.pin { None => Err(Error::String( diff --git a/crates/spk-schema/src/requirements_list.rs b/crates/spk-schema/src/requirements_list.rs index 411fe47bba..00ecf1146f 100644 --- a/crates/spk-schema/src/requirements_list.rs +++ b/crates/spk-schema/src/requirements_list.rs @@ -52,7 +52,7 @@ impl RequirementsList { /// Add a requirement in this list, or merge it in. /// /// If a request exists for the same name, it is updated with the - /// restrictions of this one one. Otherwise the new request is + /// restrictions of this one. Otherwise the new request is /// appended to the list. Returns the newly inserted or updated request. pub fn insert_or_merge(&mut self, request: Request) -> Result<()> { let name = request.name(); @@ -74,7 +74,7 @@ impl RequirementsList { Ok(()) } - /// Reports whether the provided request would be satisfied by + /// Reports whether the provided requests would be satisfied by /// this list of requests. The provided request does not need to /// exist in this list exactly, so long as there is a request in this /// list that is at least as restrictive From 8d0532754e6c469dc11e96e35c27462135af5eb2 Mon Sep 17 00:00:00 2001 From: Ryan Bottriell Date: Mon, 19 Jun 2023 15:18:18 -0700 Subject: [PATCH 6/8] Use Arc rather than String for pinned value Signed-off-by: Ryan Bottriell --- crates/spk-schema/crates/ident/src/request.rs | 22 +++++++++++-------- crates/spk-schema/src/option.rs | 2 +- crates/spk-schema/src/v0/spec.rs | 4 ++-- crates/spk-solve/src/solver_test.rs | 3 +-- 4 files changed, 17 insertions(+), 14 deletions(-) diff --git a/crates/spk-schema/crates/ident/src/request.rs b/crates/spk-schema/crates/ident/src/request.rs index 7e291bae85..1fca751d4a 100644 --- a/crates/spk-schema/crates/ident/src/request.rs +++ b/crates/spk-schema/crates/ident/src/request.rs @@ -6,6 +6,7 @@ use std::collections::BTreeMap; use std::fmt::Write; use std::marker::PhantomData; use std::str::FromStr; +use std::sync::Arc; use colored::Colorize; use serde::{Deserialize, Serialize}; @@ -289,7 +290,7 @@ impl VarRequest { impl VarRequest { /// Create a copy of this request with its pin rendered out using 'var'. - pub fn render_pin>(&self, value: S) -> Result { + pub fn render_pin>>(&self, value: S) -> Result { if !self.value.is_from_build_env() { return Err(Error::String( "Request has no pin to be rendered".to_string(), @@ -405,7 +406,7 @@ impl Serialize for VarRequest { #[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] pub enum PinnableValue { FromBuildEnv, - Pinned(String), + Pinned(Arc), } impl PinnableValue { @@ -414,27 +415,30 @@ impl PinnableValue { } pub fn is_pinned(&self) -> bool { - matches!(self, Self::FromBuildEnv) + matches!(self, Self::Pinned(_)) } /// The current pinned value, if any pub fn as_pinned(&self) -> Option<&str> { match self { Self::FromBuildEnv => None, - Self::Pinned(v) => Some(v.as_str()), + Self::Pinned(v) => Some(v), } } } impl Default for PinnableValue { fn default() -> Self { - Self::Pinned(String::new()) + Self::Pinned(Arc::from("")) } } -impl From for PinnableValue { - fn from(value: String) -> Self { - Self::Pinned(value) +impl From for PinnableValue +where + T: Into>, +{ + fn from(value: T) -> Self { + Self::Pinned(value.into()) } } @@ -965,7 +969,7 @@ impl PinValue { format!("request for `{var}` must specify a value (eg: {var}/) when `fromBuildEnv` is false or omitted") )), (Some(value), Self::None) => { - Ok(PinnableValue::Pinned(value)) + Ok(PinnableValue::Pinned(Arc::from(value))) } (None, Self::True) => Ok(PinnableValue::FromBuildEnv), (_, Self::String(s)) => Err(E::custom(format!( diff --git a/crates/spk-schema/src/option.rs b/crates/spk-schema/src/option.rs index 2b0f7ea36d..3ca9a46f07 100644 --- a/crates/spk-schema/src/option.rs +++ b/crates/spk-schema/src/option.rs @@ -438,7 +438,7 @@ impl VarOpt { let value = self.get_value(given_value).unwrap_or_default(); VarRequest { var: self.var.clone(), - value: PinnableValue::Pinned(value), + value: PinnableValue::Pinned(value.into()), } } } diff --git a/crates/spk-schema/src/v0/spec.rs b/crates/spk-schema/src/v0/spec.rs index 95033c8379..07521c3669 100644 --- a/crates/spk-schema/src/v0/spec.rs +++ b/crates/spk-schema/src/v0/spec.rs @@ -11,7 +11,7 @@ use itertools::Itertools; use serde::{Deserialize, Serialize}; use spk_schema_foundation::name::PkgNameBuf; use spk_schema_foundation::option_map::Stringified; -use spk_schema_ident::{AnyIdent, BuildIdent, Ident, PinnableValue, VersionIdent}; +use spk_schema_ident::{AnyIdent, BuildIdent, Ident, VersionIdent}; use super::TestSpec; use crate::build_spec::UncheckedBuildSpec; @@ -248,7 +248,7 @@ impl Package for Spec { var, // we are assuming that the var here will have a value because // this is a built binary package - value: PinnableValue::Pinned(o.get_value(None).unwrap_or_default()), + value: o.get_value(None).unwrap_or_default().into(), } }) .map(Request::Var); diff --git a/crates/spk-solve/src/solver_test.rs b/crates/spk-solve/src/solver_test.rs index 1f171743ac..89d747853c 100644 --- a/crates/spk-solve/src/solver_test.rs +++ b/crates/spk-solve/src/solver_test.rs @@ -13,7 +13,6 @@ use spk_schema::ident::{ build_ident, parse_ident_range, version_ident, - PinnableValue, PkgRequest, RangeIdent, Request, @@ -713,7 +712,7 @@ async fn test_solver_option_compatibility(mut solver: Solver) { solver.add_request( VarRequest { var: opt_name!("python").to_owned(), - value: PinnableValue::Pinned(pyver.to_string()), + value: pyver.into(), } .into(), ); From 8aaeee2f09ff0fe9930e36197a8e9803e3ac8325 Mon Sep 17 00:00:00 2001 From: Ryan Bottriell Date: Mon, 19 Jun 2023 16:50:12 -0700 Subject: [PATCH 7/8] Make Compatibility enums comparable Signed-off-by: Ryan Bottriell --- crates/spk-schema/crates/foundation/src/version/compat.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/spk-schema/crates/foundation/src/version/compat.rs b/crates/spk-schema/crates/foundation/src/version/compat.rs index 00bc408211..e834d79b6a 100644 --- a/crates/spk-schema/crates/foundation/src/version/compat.rs +++ b/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), From d07e31960a4bf2e182dd3cb749aa2c983d532a37 Mon Sep 17 00:00:00 2001 From: Ryan Bottriell Date: Mon, 19 Jun 2023 16:52:00 -0700 Subject: [PATCH 8/8] Check names in ReqList::contains_request Signed-off-by: Ryan Bottriell --- crates/spk-schema/src/requirements_list.rs | 49 ++++++++++++++--- .../spk-schema/src/requirements_list_test.rs | 54 +++++++++++++++++++ 2 files changed, 96 insertions(+), 7 deletions(-) diff --git a/crates/spk-schema/src/requirements_list.rs b/crates/spk-schema/src/requirements_list.rs index 00ecf1146f..e6d8e163bf 100644 --- a/crates/spk-schema/src/requirements_list.rs +++ b/crates/spk-schema/src/requirements_list.rs @@ -3,6 +3,7 @@ // https://github.com/imageworks/spk use std::collections::HashSet; +use std::fmt::Write; use serde::{Deserialize, Serialize}; use spk_schema_foundation::version::Compatibility; @@ -79,18 +80,39 @@ impl RequirementsList { /// exist in this list exactly, so long as there is a request in this /// list that is at least as restrictive pub fn contains_request(&self, theirs: &Request) -> Compatibility { + let mut global_opt_request = None; for ours in self.iter() { match (ours, theirs) { - (Request::Pkg(ours), Request::Pkg(theirs)) => { + (Request::Pkg(ours), Request::Pkg(theirs)) if ours.pkg.name == theirs.pkg.name => { return ours.contains(theirs); } - (Request::Var(ours), Request::Var(theirs)) => { + // a var request satisfy another if they have the same opt name or + // if our request is package-less and has the same base name, eg: + // name/value [contains] name/value + // pkg.name/value [contains] pkg.name/value + // name/value [contains] pkg.name/value + // + // We only exit early when we find a complete match. The last case + // above is saved and only evaluated if no more specific request is found + (Request::Var(ours), Request::Var(theirs)) if ours.var == theirs.var => { return ours.contains(theirs); } - _ => continue, + (Request::Var(ours), Request::Var(theirs)) + if theirs.var.namespace().is_some() + && ours.var.as_str() == theirs.var.base_name() => + { + global_opt_request = Some((ours, theirs)); + } + _ => { + tracing::trace!("skip {ours}, not {theirs}"); + continue; + } } } - Compatibility::incompatible("No request exists for this") + if let Some((ours, theirs)) = global_opt_request { + return ours.contains(theirs); + } + Compatibility::incompatible(format!("No request exists for {}", theirs.name())) } /// Render all requests with a package pin using the given resolved packages. @@ -136,7 +158,7 @@ impl RequirementsList { )); } Some(opt) => { - let rendered = request.render_pin(opt)?; + let rendered = request.render_pin(opt.as_str())?; let _ = std::mem::replace(request, rendered); } } @@ -167,6 +189,20 @@ impl RequirementsList { } } +impl std::fmt::Display for RequirementsList { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_char('[')?; + let mut entries = self.0.iter().peekable(); + while let Some(i) = entries.next() { + i.fmt(f)?; + if entries.peek().is_some() { + f.write_str(", ")?; + } + } + f.write_char(']') + } +} + impl IntoIterator for RequirementsList { type Item = Request; type IntoIter = std::vec::IntoIter; @@ -199,12 +235,11 @@ impl<'de> Deserialize<'de> for RequirementsList { let mut requirement_names = HashSet::with_capacity(size_hint); while let Some(request) = seq.next_element::()? { let name = request.name(); - if requirement_names.contains(name) { + if !requirement_names.insert(name.to_owned()) { return Err(serde::de::Error::custom(format!( "found multiple install requirements for '{name}'" ))); } - requirement_names.insert(name.to_owned()); requirements.push(request); } Ok(RequirementsList(requirements)) diff --git a/crates/spk-schema/src/requirements_list_test.rs b/crates/spk-schema/src/requirements_list_test.rs index f12a47233c..0ea591e6ff 100644 --- a/crates/spk-schema/src/requirements_list_test.rs +++ b/crates/spk-schema/src/requirements_list_test.rs @@ -2,6 +2,10 @@ // SPDX-License-Identifier: Apache-2.0 // https://github.com/imageworks/spk use rstest::rstest; +use serde_json::json; +use spk_schema_foundation::fixtures::*; +use spk_schema_foundation::version::Compatibility; +use spk_schema_ident::Request; use super::RequirementsList; @@ -12,3 +16,53 @@ fn test_deserialize_no_duplicates() { serde_yaml::from_str::("[{pkg: python}, {pkg: python}]") .expect_err("should fail to deserialize with the same package twice"); } + +#[rstest] +#[case::simple_pkg( + json!([ + {"pkg": "pkg-a"}, + {"pkg": "pkg-b"}, + ]), + json!({"pkg": "pkg-a"}) +)] +#[case::global_var( + json!([ + {"var": "global/value"}, + ]), + json!({"var": "global/value"}) +)] +#[case::global_matches_namespaced( + json!([ + {"var": "local/value"}, + ]), + json!({"var": "pkg.local/value"}) +)] +#[case::two_namespaced_vars( + json!([ + {"var": "pkg.local/value"}, + ]), + json!({"var": "pkg.local/value"}) +)] +#[should_panic] +#[case::different_namespaces( + json!([ + {"var": "ns1.var/value"}, + ]), + json!({"var": "ns2.var/value"}) +)] +#[should_panic] +#[case::separate_pkg( + json!([ + {"pkg": "pkg-a"}, + {"pkg": "pkg-b"}, + ]), + json!({"pkg": "pkg-c"}) +)] +fn test_contains_request(#[case] requests: serde_json::Value, #[case] contains: serde_json::Value) { + init_logging(); + + let reqs: RequirementsList = serde_json::from_value(requests).unwrap(); + let contains: Request = serde_json::from_value(contains).unwrap(); + tracing::debug!("is {contains} contained within this? {reqs}"); + assert_eq!(reqs.contains_request(&contains), Compatibility::Compatible); +}