Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DRAFT: Add host_compat lints (requires spk-lint-update) #929

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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
59 changes: 58 additions & 1 deletion crates/spk-schema/src/build_spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,13 @@ const ARCH_ADDS: &[&OptName] = &[OptName::os(), OptName::arch()];
const OS_ADDS: &[&OptName] = &[OptName::os()];
const NONE_ADDS: &[&OptName] = &[];

// Each HostCompat value disallows certain var names when host_compat
// validation is enabled in the config file.
const DISTRO_DISALLOWS: &[&OptName] = &[];
const ARCH_DISALLOWS: &[&OptName] = &[OptName::distro()];
const OS_DISALLOWS: &[&OptName] = &[OptName::distro(), OptName::arch()];
const NONE_DISALLOWS: &[&OptName] = &[OptName::distro(), OptName::arch(), OptName::os()];

/// Describes what level of cross-platform compatibility the built package
/// should have.
#[derive(
Expand Down Expand Up @@ -60,6 +67,16 @@ impl HostCompat {
names.iter().copied().collect::<HashSet<&OptName>>()
}

// TODO: should this be a hashset too?
fn names_disallowed(&self) -> &[&OptName] {
match self {
HostCompat::Distro => DISTRO_DISALLOWS,
HostCompat::Arch => ARCH_DISALLOWS,
HostCompat::Os => OS_DISALLOWS,
HostCompat::None => NONE_DISALLOWS,
}
}

/// Get host_options after filtering based on the cross Os
/// compatibility setting.
pub fn host_options(&self) -> Result<Vec<Opt>> {
Expand Down Expand Up @@ -279,6 +296,8 @@ impl<'de> Deserialize<'de> for UncheckedBuildSpec {
{
let mut variants = Vec::<OptionMap>::new();
let mut unchecked = BuildSpec::default();
let has_auto_host_vars_field = false;

while let Some(key) = map.next_key::<Stringified>()? {
match key.as_str() {
"script" => unchecked.script = map.next_value::<Script>()?,
Expand All @@ -302,7 +321,8 @@ impl<'de> Deserialize<'de> for UncheckedBuildSpec {
unchecked.validation = map.next_value::<ValidationSpec>()?
}
"auto_host_vars" => {
unchecked.host_compat = map.next_value::<HostCompat>()?
unchecked.host_compat = map.next_value::<HostCompat>()?;
has_auto_host_vars_field = true;
}
_ => {
// for forwards compatibility we ignore any unrecognized
Expand All @@ -324,6 +344,43 @@ impl<'de> Deserialize<'de> for UncheckedBuildSpec {
.map(|o| v0::Variant::from_options(o, &unchecked.options))
.collect();

if !has_auto_host_vars_field {
// TODO: might want to make a MissingKey or something lint
// instead of reusing the one in the linting PR as a placeholder
//let lint = UnknownKey::new("host_compat", vec!["host_compat"]);
self.lints.push(format!(
"Missing build spec field: host_compat\nWill default to 'host_compat: {}'",
HostCompat::default().to_string()
));
}

// Check if any variants set a value on a var that the
// host_compat value would normally set, or one it
// would normally disallow.
let added_names = unchecked.host_compat.names_added();
let disallowed_names = unchecked.host_compat.names_disallowed();

// TODO: to variant numbers start at 1 or 0?
let number = 1;
for variant_opts in variants {
for add_name in added_names {
// Caompre btreemp vs hashset, and ones a vec, make it a hashset too?
if let Some(value) = variant_opts.get(&*add_name) {
// This variant will override a host
// option added by the host_compat.
self.lints.push(format!("Variant {number} sets {add_name}={value} which overrides the {} host_compat's setting for {add_name}", unchecked.host_compat));
}
}
for bad_name in disallowed_names {
if let Some(value) = variant_opts.get(*bad_name) {
// This variant will set a host option
// the host_compat disallows.
self.lints.push(format!("Variant {number} sets {bad_name}={value} but the 'host_compat' value of {} disallows setting {bad_name}", unchecked.host_compat));
}
}
number += 1;
}

Ok(UncheckedBuildSpec(unchecked))
}
}
Expand Down