Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions crates/spk-build/src/build/binary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,7 @@ where
package,
variant: full_variant,
environment_filesystem,
suppressed_requirements: self.recipe.suppressed_requirements(),
};
let mut report = BuildReport {
setup,
Expand Down
6 changes: 5 additions & 1 deletion crates/spk-build/src/report.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@
// SPDX-License-Identifier: Apache-2.0
// https://github.com/spkenv/spk

use std::collections::HashMap;
use std::collections::{HashMap, HashSet};

use spk_schema::foundation::ident_component::Component;
use spk_schema::foundation::name::OptNameBuf;
use spk_schema::{BuildIdent, Package, Variant};
use spk_solve::Solution;

Expand Down Expand Up @@ -39,6 +40,9 @@ where
/// where each entry is tagged with the package that owns
/// it within the build environment.
pub environment_filesystem: spfs::tracking::Manifest<BuildIdent>,
/// Var names that the recipe explicitly suppresses from strong
/// inheritance, used by the InheritRequirements validator.
pub suppressed_requirements: HashSet<OptNameBuf>,
}

/// Details about the generated files and resulting output of
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ async fn test_validate_build_changeset_modified() {
environment_filesystem: Manifest::new(
spfs::tracking::Entry::empty_dir_with_open_perms_with_data(package.ident().clone()),
),
suppressed_requirements: Default::default(),
package,
},
};
Expand Down
1 change: 1 addition & 0 deletions crates/spk-build/src/validation/collect_all_files_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ async fn test_validate_build_changeset_collected() {
environment_filesystem: Manifest::new(
spfs::tracking::Entry::empty_dir_with_open_perms_with_data(package.ident().clone()),
),
suppressed_requirements: Default::default(),
package,
},
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ async fn test_validate_build_changeset_collect_existing() {
environment: Solution::default(),
variant: package.option_values(),
environment_filesystem,
suppressed_requirements: Default::default(),
package,
},
};
Expand Down
1 change: 1 addition & 0 deletions crates/spk-build/src/validation/empty_package_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ async fn test_validate_build_changeset_nothing() {
environment_filesystem: Manifest::new(
spfs::tracking::Entry::empty_dir_with_open_perms_with_data(package.ident().clone()),
),
suppressed_requirements: Default::default(),
package,
},
output: Default::default(),
Expand Down
6 changes: 6 additions & 0 deletions crates/spk-build/src/validation/inherit_requirements.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// SPDX-License-Identifier: Apache-2.0
// https://github.com/spkenv/spk

use spk_schema::ident::RequestWithOptions;
use spk_schema::name::PkgNameBuf;
use spk_schema::validation::{
ValidationMatcherDiscriminants,
Expand Down Expand Up @@ -88,6 +89,11 @@ impl super::Validator for InheritRequirementsValidator<'_> {
.spec
.downstream_runtime_requirements([component]);
for request in downstream_runtime.iter() {
if let RequestWithOptions::Var(var) = request
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the main implementation hurdle, making it so if the var is suppressed it doesn't then violate the validation rules.

&& setup.suppressed_requirements.contains(&var.var)
{
continue;
}
let status = match (self.kind, runtime_requirements.contains_request(request)) {
(RuleKind::Allow, Compatibility::Compatible)
| (RuleKind::Allow, Compatibility::Incompatible(_))
Expand Down
2 changes: 2 additions & 0 deletions crates/spk-build/src/validation/inherit_requirements_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ async fn test_build_package_downstream_build_requests() {
environment_filesystem: Manifest::new(
spfs::tracking::Entry::empty_dir_with_open_perms_with_data(package.ident().clone()),
),
suppressed_requirements: Default::default(),
package,
};
let err = ValidationRule::Require {
Expand Down Expand Up @@ -124,6 +125,7 @@ async fn test_build_package_downstream_runtime_request() {
environment_filesystem: Manifest::new(
spfs::tracking::Entry::empty_dir_with_open_perms_with_data(package.ident().clone()),
),
suppressed_requirements: Default::default(),
package,
};
let err = ValidationRule::Require {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ async fn test_for_description_over_limit() {
environment_filesystem: Manifest::new(
spfs::tracking::Entry::empty_dir_with_open_perms_with_data(package.ident().clone()),
),
suppressed_requirements: Default::default(),
package,
};

Expand Down
1 change: 1 addition & 0 deletions crates/spk-build/src/validation/recursive_build_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ async fn test_build_with_circular_dependency() {
environment_filesystem: Manifest::new(
spfs::tracking::Entry::empty_dir_with_open_perms_with_data(new_build.ident().clone()),
),
suppressed_requirements: Default::default(),
package: new_build,
};
ValidationRule::Deny {
Expand Down
1 change: 1 addition & 0 deletions crates/spk-build/src/validation/spdx_license_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ macro_rules! basic_setup {
environment_filesystem: Manifest::new(
spfs::tracking::Entry::empty_dir_with_open_perms_with_data(package.ident().clone()),
),
suppressed_requirements: Default::default(),
package,
}
}};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ async fn test_strongly_inherited_vars_require_desc() {
environment_filesystem: Manifest::new(
spfs::tracking::Entry::empty_dir_with_open_perms_with_data(package.ident().clone()),
),
suppressed_requirements: Default::default(),
package,
};

Expand Down
97 changes: 96 additions & 1 deletion crates/spk-cli/cmd-build/src/cmd_build_test/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@
use clap::Parser;
use rstest::rstest;
use spfs::storage::prelude::*;
use spk_schema::RuntimeEnvironment;
use spk_schema::foundation::fixtures::*;
use spk_schema::foundation::{option_map, version_ident};
use spk_schema::ident_component::Component;
use spk_schema::option_map::HOST_OPTIONS;
use spk_schema::{Package, RuntimeEnvironment};
use spk_storage::fixtures::*;

use super::Build;
Expand Down Expand Up @@ -837,3 +837,98 @@ install:
"should have environment ops"
);
}

#[rstest]
#[case::cli("cli")]
#[case::checks("checks")]
#[case::resolvo("resolvo")]
#[tokio::test]
async fn test_package_can_opt_out_of_strongly_inherited_var(
tmpdir: tempfile::TempDir,
#[case] solver_to_run: &str,
) {
let rt = spfs_runtime().await;

build_package!(
tmpdir,
"base.spk.yaml",
br#"
api: v0/package
pkg: base/1.0.0
build:
options:
- var: inherit-me/1.2.3
inheritance: Strong
description: test content
script:
- true
"#,
solver_to_run
);

build_package!(
tmpdir,
"baseline.spk.yaml",
br#"
api: v0/package
pkg: baseline/1.0.0
build:
options:
- pkg: base
script:
- true
"#,
solver_to_run
);

let build = rt
.tmprepo
.list_package_builds(&version_ident!("baseline/1.0.0"))
.await
.unwrap()
.into_iter()
.find(|b| !b.is_source())
.unwrap();

let pkg = rt.tmprepo.read_package(&build).await.unwrap();

let requirements = pkg.runtime_requirements();
// As written, if the strongly-inherited var is not suppressed, then the
// package should have one entry in the runtime_requirements list
assert_eq!(1, requirements.len(), "expected one requirement");

build_package!(
tmpdir,
"downstream.spk.yaml",
br#"
api: v0/package
pkg: downstream/1.0.0
build:
options:
- pkg: base
script:
- true
install:
requirements:
- var: base.inherit-me
suppress: "don't inherit this!"
"#,
solver_to_run
);

let build = rt
.tmprepo
.list_package_builds(&version_ident!("downstream/1.0.0"))
.await
.unwrap()
.into_iter()
.find(|b| !b.is_source())
.unwrap();

let pkg = rt.tmprepo.read_package(&build).await.unwrap();

let requirements = pkg.runtime_requirements();
// As written, if the strongly-inherited var is suppressed, then the package
// should have an empty runtime_requirements list
assert_eq!(0, requirements.len(), "expected an empty list");
}
Original file line number Diff line number Diff line change
Expand Up @@ -195,17 +195,41 @@ impl<'de> Deserialize<'de> for PinPolicy {

/// Represents a constraint added to a resolved environment.
#[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd, Variantly)]
#[cfg_attr(feature = "parsedbuf-serde", derive(Serialize), serde(untagged))]
pub enum PinnableRequest {
Pkg(PkgRequest),
Var(VarRequest<PinnableValue>),
/// Prevent the package from inheriting the var named.
/// Contains the var name and an optional comment explaining
/// why the suppression is needed.
Suppress(OptNameBuf, String),
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I elected to add a 3rd arm here to not overload the VarRequest type and have that impact many more areas of the codebase. This feature should conceptually stay within the realm of recipes and building packages and not extend into the built package or solver territories.

}

#[cfg(feature = "parsedbuf-serde")]
impl Serialize for PinnableRequest {
fn serialize<S>(&self, serializer: S) -> std::result::Result<S::Ok, S::Error>
where
S: serde::Serializer,
{
match self {
PinnableRequest::Pkg(pkg) => pkg.serialize(serializer),
PinnableRequest::Var(var) => var.serialize(serializer),
PinnableRequest::Suppress(var, comment) => {
use serde::ser::SerializeMap;
let mut map = serializer.serialize_map(Some(2))?;
map.serialize_entry("var", var.as_str())?;
map.serialize_entry("suppress", comment)?;
map.end()
}
}
}
}

impl crate::spec_ops::Named<OptName> for PinnableRequest {
fn name(&self) -> &OptName {
match self {
PinnableRequest::Var(r) => &r.var,
PinnableRequest::Pkg(r) => r.pkg.name.as_opt_name(),
PinnableRequest::Suppress(var, _) => var,
}
}
}
Expand All @@ -215,6 +239,7 @@ impl std::fmt::Display for PinnableRequest {
match self {
Self::Pkg(p) => p.fmt(f),
Self::Var(v) => v.fmt(f),
Self::Suppress(v, _) => v.fmt(f),
}
}
}
Expand Down Expand Up @@ -281,6 +306,7 @@ impl<'de> Deserialize<'de> for PinnableRequest {
var: Option<OptNameBuf>,
value: Option<String>,
description: Option<String>,
suppress: Option<serde_yaml::Value>,

// Both
pin: Option<PinValue>,
Expand Down Expand Up @@ -319,6 +345,7 @@ impl<'de> Deserialize<'de> for PinnableRequest {
}
"value" => self.value = Some(map.next_value::<String>()?),
"description" => self.description = Some(map.next_value::<String>()?),
"suppress" => self.suppress = Some(map.next_value::<serde_yaml::Value>()?),
_ => {
// unrecognized fields are explicitly ignored in case
// they were added in a newer version of spk. We assume
Expand Down Expand Up @@ -348,6 +375,16 @@ impl<'de> Deserialize<'de> for PinnableRequest {
required_compat: None,
requested_by: Default::default(),
})),
(None, Some(var)) if self.suppress.is_some() => {
let comment = match self.suppress.unwrap() {
serde_yaml::Value::String(s) => s,
other => serde_yaml::to_string(&other)
.unwrap_or_default()
.trim()
.to_string(),
};
Ok(PinnableRequest::Suppress(var, comment))
}
(None, Some(var)) => {
let mut value = self
.pin
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,9 @@ impl TryFrom<PinnableRequest> for PinnedRequest {
match req {
PinnableRequest::Pkg(r) => Ok(PinnedRequest::Pkg(r)),
PinnableRequest::Var(r) => Ok(PinnedRequest::Var(r.try_into()?)),
PinnableRequest::Suppress(..) => {
Err("a var suppression rule cannot be converted into a pinned request".into())
}
}
}
}
Expand Down
13 changes: 12 additions & 1 deletion crates/spk-schema/src/recipe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@
// https://github.com/spkenv/spk

use std::borrow::Cow;
use std::collections::HashMap;
use std::collections::{HashMap, HashSet};
use std::path::Path;

use spk_schema_foundation::ident::{RequestWithOptions, VersionIdent};
use spk_schema_foundation::name::OptNameBuf;

use crate::foundation::ident_build::BuildId;
use crate::foundation::option_map::OptionMap;
Expand Down Expand Up @@ -113,6 +114,12 @@ pub trait Recipe:

/// Return the set of configured validators when building this package
fn validation(&self) -> &super::ValidationSpec;

/// Return the set of var names that this recipe explicitly suppresses
/// from being inherited via strong inheritance.
fn suppressed_requirements(&self) -> HashSet<OptNameBuf> {
HashSet::new()
}
}

forward_to_impl!(box = false, Recipe, {
Expand Down Expand Up @@ -183,4 +190,8 @@ forward_to_impl!(box = false, Recipe, {
fn validation(&self) -> &super::ValidationSpec {
(**self).validation()
}

fn suppressed_requirements(&self) -> HashSet<OptNameBuf> {
(**self).suppressed_requirements()
}
});
5 changes: 5 additions & 0 deletions crates/spk-schema/src/requirements_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,11 @@ impl RequirementsList<PinnableRequest> {
}
}
}
PinnableRequest::Suppress(..) => {
// Suppression rules are not carried over into built
// packages
None
}
}
}).collect::<Result<Vec<_>>>()?))
}
Expand Down
6 changes: 6 additions & 0 deletions crates/spk-schema/src/spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,12 @@ impl Recipe for SpecRecipe {
fn validation(&self) -> &ValidationSpec {
each_variant!(self, r, r.validation())
}

fn suppressed_requirements(
&self,
) -> std::collections::HashSet<spk_schema_foundation::name::OptNameBuf> {
each_variant!(self, r, r.suppressed_requirements())
}
}

impl HasVersion for SpecRecipe {
Expand Down
Loading
Loading