Merge pull request #278805 from tweag/by-name-enforce-preparation

check-by-name: Refactor to prepare for enforcing `pkgs/by-name`, make `--base` required
This commit is contained in:
Silvan Mosberger 2024-01-09 22:58:53 +01:00 committed by GitHub
commit da3e72b915
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 280 additions and 213 deletions

View File

@ -28,6 +28,8 @@ These checks are performed by this tool:
- Each package directory must not refer to files outside itself using symlinks or Nix path expressions.
### Nix evaluation checks
Evaluate Nixpkgs with `system` set to `x86_64-linux` and check that:
- For each package directory, the `pkgs.${name}` attribute must be defined as `callPackage pkgs/by-name/${shard}/${name}/package.nix args` for some `args`.
- For each package directory, `pkgs.lib.isDerivation pkgs.${name}` must be `true`.

View File

@ -1,11 +1,7 @@
# Takes a path to nixpkgs and a path to the json-encoded list of attributes to check.
# Returns an attribute set containing information on each requested attribute.
# If the attribute is missing from Nixpkgs it's also missing from the result.
#
# The returned information is an attribute set with:
# - call_package_path: The <path> from `<attr> = callPackage <path> { ... }`,
# or null if it's not defined as with callPackage, or if the <path> is not a path
# - is_derivation: The result of `lib.isDerivation <attr>`
# Returns an value containing information on each requested attribute,
# which is decoded on the Rust side.
# See ./eval.rs for the meaning of the returned values
{
attrsPath,
nixpkgsPath,
@ -13,70 +9,85 @@
let
attrs = builtins.fromJSON (builtins.readFile attrsPath);
# This overlay mocks callPackage to persist the path of the first argument
callPackageOverlay = self: super: {
nixpkgsPathLength = builtins.stringLength (toString nixpkgsPath) + 1;
removeNixpkgsPrefix = builtins.substring nixpkgsPathLength (-1);
# We need access to the `callPackage` arguments of each attribute.
# The only way to do so is to override `callPackage` with our own version that adds this information to the result,
# and then try to access this information.
overlay = final: prev: {
# Information for attributes defined using `callPackage`
callPackage = fn: args:
let
result = super.callPackage fn args;
variantInfo._attributeVariant = {
# These names are used by the deserializer on the Rust side
CallPackage.path =
addVariantInfo (prev.callPackage fn args) {
Manual = {
path =
if builtins.isPath fn then
toString fn
removeNixpkgsPrefix (toString fn)
else
null;
CallPackage.empty_arg =
empty_arg =
args == { };
};
in
if builtins.isAttrs result then
# If this was the last overlay to be applied, we could just only return the `_callPackagePath`,
# but that's not the case because stdenv has another overlays on top of user-provided ones.
# So to not break the stdenv build we need to return the mostly proper result here
result // variantInfo
else
# It's very rare that callPackage doesn't return an attribute set, but it can occur.
variantInfo;
};
# Information for attributes that are auto-called from pkgs/by-name.
# This internal attribute is only used by pkgs/by-name
_internalCallByNamePackageFile = file:
let
result = super._internalCallByNamePackageFile file;
variantInfo._attributeVariant = {
# This name is used by the deserializer on the Rust side
AutoCalled = null;
};
in
if builtins.isAttrs result then
# If this was the last overlay to be applied, we could just only return the `_callPackagePath`,
# but that's not the case because stdenv has another overlays on top of user-provided ones.
# So to not break the stdenv build we need to return the mostly proper result here
result // variantInfo
else
# It's very rare that callPackage doesn't return an attribute set, but it can occur.
variantInfo;
addVariantInfo (prev._internalCallByNamePackageFile file) {
Auto = null;
};
};
# We can't just replace attribute values with their info in the overlay,
# because attributes can depend on other attributes, so this would break evaluation.
addVariantInfo = value: variant:
if builtins.isAttrs value then
value // {
_callPackageVariant = variant;
}
else
# It's very rare that callPackage doesn't return an attribute set, but it can occur.
# In such a case we can't really return anything sensible that would include the info,
# so just don't return the info and let the consumer handle it.
value;
pkgs = import nixpkgsPath {
# Don't let the users home directory influence this result
config = { };
overlays = [ callPackageOverlay ];
overlays = [ overlay ];
# We check evaluation and callPackage only for x86_64-linux.
# Not ideal, but hard to fix
system = "x86_64-linux";
};
attrInfo = attr:
let
value = pkgs.${attr};
in
{
# These names are used by the deserializer on the Rust side
variant = value._attributeVariant or { Other = null; };
is_derivation = pkgs.lib.isDerivation value;
};
attrInfo = name: value:
if ! builtins.isAttrs value then
{
NonAttributeSet = null;
}
else if ! value ? _callPackageVariant then
{
NonCallPackage = null;
}
else
{
CallPackage = {
call_package_variant = value._callPackageVariant;
is_derivation = pkgs.lib.isDerivation value;
};
};
attrInfos = builtins.listToAttrs (map (name: {
inherit name;
value = attrInfo name;
}) attrs);
attrInfos = map (name: [
name
(
if ! pkgs ? ${name} then
{ Missing = null; }
else
{ Existing = attrInfo name pkgs.${name}; }
)
]) attrs;
in
# Filter out attributes not in Nixpkgs
builtins.intersectAttrs pkgs attrInfos
attrInfos

View File

@ -6,33 +6,48 @@ use std::path::Path;
use anyhow::Context;
use serde::Deserialize;
use std::collections::HashMap;
use std::path::PathBuf;
use std::process;
use tempfile::NamedTempFile;
/// Attribute set of this structure is returned by eval.nix
#[derive(Deserialize)]
struct AttributeInfo {
variant: AttributeVariant,
enum ByNameAttribute {
/// The attribute doesn't exist at all
Missing,
Existing(AttributeInfo),
}
#[derive(Deserialize)]
enum AttributeInfo {
/// The attribute exists, but its value isn't an attribute set
NonAttributeSet,
/// The attribute exists, but its value isn't defined using callPackage
NonCallPackage,
/// The attribute exists and its value is an attribute set
CallPackage(CallPackageInfo),
}
#[derive(Deserialize)]
struct CallPackageInfo {
call_package_variant: CallPackageVariant,
/// Whether the attribute is a derivation (`lib.isDerivation`)
is_derivation: bool,
}
#[derive(Deserialize)]
enum AttributeVariant {
enum CallPackageVariant {
/// The attribute is auto-called as pkgs.callPackage using pkgs/by-name,
/// and it is not overridden by a definition in all-packages.nix
AutoCalled,
Auto,
/// The attribute is defined as a pkgs.callPackage <path> <args>,
/// and overridden by all-packages.nix
CallPackage {
Manual {
/// The <path> argument or None if it's not a path
path: Option<PathBuf>,
/// true if <args> is { }
empty_arg: bool,
},
/// The attribute is not defined as pkgs.callPackage
Other,
}
/// Check that the Nixpkgs attribute values corresponding to the packages in pkgs/by-name are
@ -45,20 +60,22 @@ pub fn check_values(
) -> validation::Result<ratchet::Nixpkgs> {
// Write the list of packages we need to check into a temporary JSON file.
// This can then get read by the Nix evaluation.
let attrs_file = NamedTempFile::new().context("Failed to create a temporary file")?;
let attrs_file = NamedTempFile::new().with_context(|| "Failed to create a temporary file")?;
// We need to canonicalise this path because if it's a symlink (which can be the case on
// Darwin), Nix would need to read both the symlink and the target path, therefore need 2
// NIX_PATH entries for restrict-eval. But if we resolve the symlinks then only one predictable
// entry is needed.
let attrs_file_path = attrs_file.path().canonicalize()?;
serde_json::to_writer(&attrs_file, &package_names).context(format!(
"Failed to serialise the package names to the temporary path {}",
attrs_file_path.display()
))?;
serde_json::to_writer(&attrs_file, &package_names).with_context(|| {
format!(
"Failed to serialise the package names to the temporary path {}",
attrs_file_path.display()
)
})?;
let expr_path = std::env::var("NIX_CHECK_BY_NAME_EXPR_PATH")
.context("Could not get environment variable NIX_CHECK_BY_NAME_EXPR_PATH")?;
.with_context(|| "Could not get environment variable NIX_CHECK_BY_NAME_EXPR_PATH")?;
// With restrict-eval, only paths in NIX_PATH can be accessed, so we explicitly specify the
// ones needed needed
let mut command = process::Command::new("nix-instantiate");
@ -97,80 +114,96 @@ pub fn check_values(
let result = command
.output()
.context(format!("Failed to run command {command:?}"))?;
.with_context(|| format!("Failed to run command {command:?}"))?;
if !result.status.success() {
anyhow::bail!("Failed to run command {command:?}");
}
// Parse the resulting JSON value
let actual_files: HashMap<String, AttributeInfo> = serde_json::from_slice(&result.stdout)
.context(format!(
"Failed to deserialise {}",
String::from_utf8_lossy(&result.stdout)
))?;
let attributes: Vec<(String, ByNameAttribute)> = serde_json::from_slice(&result.stdout)
.with_context(|| {
format!(
"Failed to deserialise {}",
String::from_utf8_lossy(&result.stdout)
)
})?;
Ok(
validation::sequence(package_names.into_iter().map(|package_name| {
let relative_package_file = structure::relative_file_for_package(&package_name);
let absolute_package_file = nixpkgs_path.join(&relative_package_file);
let check_result = validation::sequence(attributes.into_iter().map(
|(attribute_name, attribute_value)| {
let relative_package_file = structure::relative_file_for_package(&attribute_name);
if let Some(attribute_info) = actual_files.get(&package_name) {
let check_result = if !attribute_info.is_derivation {
NixpkgsProblem::NonDerivation {
relative_package_file: relative_package_file.clone(),
package_name: package_name.clone(),
}
.into()
} else {
Success(())
};
use ratchet::RatchetState::*;
use AttributeInfo::*;
use ByNameAttribute::*;
use CallPackageVariant::*;
let check_result = check_result.and(match &attribute_info.variant {
AttributeVariant::AutoCalled => Success(ratchet::Package {
empty_non_auto_called: ratchet::EmptyNonAutoCalled::Valid,
}),
AttributeVariant::CallPackage { path, empty_arg } => {
let correct_file = if let Some(call_package_path) = path {
absolute_package_file == *call_package_path
} else {
false
};
if correct_file {
Success(ratchet::Package {
// Empty arguments for non-auto-called packages are not allowed anymore.
empty_non_auto_called: if *empty_arg {
ratchet::EmptyNonAutoCalled::Invalid
} else {
ratchet::EmptyNonAutoCalled::Valid
},
})
} else {
NixpkgsProblem::WrongCallPackage {
relative_package_file: relative_package_file.clone(),
package_name: package_name.clone(),
}
.into()
}
}
AttributeVariant::Other => NixpkgsProblem::WrongCallPackage {
relative_package_file: relative_package_file.clone(),
package_name: package_name.clone(),
}
.into(),
});
check_result.map(|value| (package_name.clone(), value))
} else {
NixpkgsProblem::UndefinedAttr {
let check_result = match attribute_value {
Missing => NixpkgsProblem::UndefinedAttr {
relative_package_file: relative_package_file.clone(),
package_name: package_name.clone(),
package_name: attribute_name.clone(),
}
.into()
}
}))
.map(|elems| ratchet::Nixpkgs {
packages: elems.into_iter().collect(),
}),
)
.into(),
Existing(NonAttributeSet) => NixpkgsProblem::NonDerivation {
relative_package_file: relative_package_file.clone(),
package_name: attribute_name.clone(),
}
.into(),
Existing(NonCallPackage) => NixpkgsProblem::WrongCallPackage {
relative_package_file: relative_package_file.clone(),
package_name: attribute_name.clone(),
}
.into(),
Existing(CallPackage(CallPackageInfo {
is_derivation,
call_package_variant,
})) => {
let check_result = if !is_derivation {
NixpkgsProblem::NonDerivation {
relative_package_file: relative_package_file.clone(),
package_name: attribute_name.clone(),
}
.into()
} else {
Success(())
};
check_result.and(match &call_package_variant {
Auto => Success(ratchet::Package {
empty_non_auto_called: Tight,
}),
Manual { path, empty_arg } => {
let correct_file = if let Some(call_package_path) = path {
relative_package_file == *call_package_path
} else {
false
};
if correct_file {
Success(ratchet::Package {
// Empty arguments for non-auto-called packages are not allowed anymore.
empty_non_auto_called: if *empty_arg {
Loose(ratchet::EmptyNonAutoCalled)
} else {
Tight
},
})
} else {
NixpkgsProblem::WrongCallPackage {
relative_package_file: relative_package_file.clone(),
package_name: attribute_name.clone(),
}
.into()
}
}
})
}
};
check_result.map(|value| (attribute_name.clone(), value))
},
));
Ok(check_result.map(|elems| ratchet::Nixpkgs {
package_names,
package_map: elems.into_iter().collect(),
}))
}

View File

@ -38,15 +38,13 @@ pub struct Args {
/// Path to the base Nixpkgs to run ratchet checks against.
/// For PRs, this should be set to a checkout of the PRs base branch.
/// If not specified, no ratchet checks will be performed.
/// However, this flag will become required once CI uses it.
#[arg(long)]
base: Option<PathBuf>,
base: PathBuf,
}
fn main() -> ExitCode {
let args = Args::parse();
match process(args.base.as_deref(), &args.nixpkgs, &[], &mut io::stderr()) {
match process(&args.base, &args.nixpkgs, &[], &mut io::stderr()) {
Ok(true) => {
eprintln!("{}", "Validated successfully".green());
ExitCode::SUCCESS
@ -77,7 +75,7 @@ fn main() -> ExitCode {
/// - `Ok(false)` if there are problems, all of which will be written to `error_writer`.
/// - `Ok(true)` if there are no problems
pub fn process<W: io::Write>(
base_nixpkgs: Option<&Path>,
base_nixpkgs: &Path,
main_nixpkgs: &Path,
eval_accessible_paths: &[&Path],
error_writer: &mut W,
@ -87,18 +85,14 @@ pub fn process<W: io::Write>(
let check_result = main_result.result_map(|nixpkgs_version| {
// If the main Nixpkgs doesn't have any problems, run the ratchet checks against the base
// Nixpkgs
if let Some(base) = base_nixpkgs {
check_nixpkgs(base, eval_accessible_paths, error_writer)?.result_map(
|base_nixpkgs_version| {
Ok(ratchet::Nixpkgs::compare(
Some(base_nixpkgs_version),
nixpkgs_version,
))
},
)
} else {
Ok(ratchet::Nixpkgs::compare(None, nixpkgs_version))
}
check_nixpkgs(base_nixpkgs, eval_accessible_paths, error_writer)?.result_map(
|base_nixpkgs_version| {
Ok(ratchet::Nixpkgs::compare(
base_nixpkgs_version,
nixpkgs_version,
))
},
)
})?;
match check_result {
@ -123,10 +117,12 @@ pub fn check_nixpkgs<W: io::Write>(
error_writer: &mut W,
) -> validation::Result<ratchet::Nixpkgs> {
Ok({
let nixpkgs_path = nixpkgs_path.canonicalize().context(format!(
"Nixpkgs path {} could not be resolved",
nixpkgs_path.display()
))?;
let nixpkgs_path = nixpkgs_path.canonicalize().with_context(|| {
format!(
"Nixpkgs path {} could not be resolved",
nixpkgs_path.display()
)
})?;
if !nixpkgs_path.join(utils::BASE_SUBPATH).exists() {
writeln!(
@ -234,16 +230,16 @@ mod tests {
let base_path = path.join("base");
let base_nixpkgs = if base_path.exists() {
Some(base_path.as_path())
base_path.as_path()
} else {
None
Path::new("tests/empty-base")
};
// We don't want coloring to mess up the tests
let writer = temp_env::with_var("NO_COLOR", Some("1"), || -> anyhow::Result<_> {
let mut writer = vec![];
process(base_nixpkgs, &path, &[&extra_nix_path], &mut writer)
.context(format!("Failed test case {name}"))?;
.with_context(|| format!("Failed test case {name}"))?;
Ok(writer)
})?;

View File

@ -10,31 +10,20 @@ use std::collections::HashMap;
/// The ratchet value for the entirety of Nixpkgs.
#[derive(Default)]
pub struct Nixpkgs {
/// The ratchet values for each package in `pkgs/by-name`
pub packages: HashMap<String, Package>,
/// Sorted list of attributes in package_map
pub package_names: Vec<String>,
/// The ratchet values for all packages
pub package_map: HashMap<String, Package>,
}
impl Nixpkgs {
/// Validates the ratchet checks for Nixpkgs
pub fn compare(optional_from: Option<Self>, to: Self) -> Validation<()> {
pub fn compare(from: Self, to: Self) -> Validation<()> {
validation::sequence_(
// We only loop over the current attributes,
// we don't need to check ones that were removed
to.packages.into_iter().map(|(name, attr_to)| {
let attr_from = if let Some(from) = &optional_from {
from.packages.get(&name)
} else {
// This pretends that if there's no base version to compare against, all
// attributes existed without conforming to the new strictness check for
// backwards compatibility.
// TODO: Remove this case. This is only needed because the `--base`
// argument is still optional, which doesn't need to be once CI is updated
// to pass it.
Some(&Package {
empty_non_auto_called: EmptyNonAutoCalled::Invalid,
})
};
Package::compare(&name, attr_from, &attr_to)
to.package_names.into_iter().map(|name| {
Package::compare(&name, from.package_map.get(&name), &to.package_map[&name])
}),
)
}
@ -43,13 +32,13 @@ impl Nixpkgs {
/// The ratchet value for a single package in `pkgs/by-name`
pub struct Package {
/// The ratchet value for the check for non-auto-called empty arguments
pub empty_non_auto_called: EmptyNonAutoCalled,
pub empty_non_auto_called: RatchetState<EmptyNonAutoCalled>,
}
impl Package {
/// Validates the ratchet checks for a single package defined in `pkgs/by-name`
pub fn compare(name: &str, optional_from: Option<&Self>, to: &Self) -> Validation<()> {
EmptyNonAutoCalled::compare(
RatchetState::<EmptyNonAutoCalled>::compare(
name,
optional_from.map(|x| &x.empty_non_auto_called),
&to.empty_non_auto_called,
@ -57,29 +46,59 @@ impl Package {
}
}
/// The ratchet value of a single package in `pkgs/by-name`
/// The ratchet state of a generic ratchet check.
pub enum RatchetState<Context> {
/// The ratchet is loose, it can be tightened more.
/// In other words, this is the legacy state we're trying to move away from.
/// Introducing new instances is not allowed but previous instances will continue to be allowed.
/// The `Context` is context for error messages in case a new instance of this state is
/// introduced
Loose(Context),
/// The ratchet is tight, it can't be tightened any further.
/// This is either because we already use the latest state, or because the ratchet isn't
/// relevant.
Tight,
}
/// A trait that can convert an attribute-specific error context into a NixpkgsProblem
pub trait ToNixpkgsProblem {
/// How to convert an attribute-specific error context into a NixpkgsProblem
fn to_nixpkgs_problem(name: &str, context: &Self, existed_before: bool) -> NixpkgsProblem;
}
impl<Context: ToNixpkgsProblem> RatchetState<Context> {
/// Compare the previous ratchet state of an attribute to the new state.
/// The previous state may be `None` in case the attribute is new.
fn compare(name: &str, optional_from: Option<&Self>, to: &Self) -> Validation<()> {
// If we don't have a previous state, enforce a tight ratchet
let from = optional_from.unwrap_or(&RatchetState::Tight);
match (from, to) {
// Always okay to keep it tight or tighten the ratchet
(_, RatchetState::Tight) => Success(()),
// Grandfathering policy for a loose ratchet
(RatchetState::Loose { .. }, RatchetState::Loose { .. }) => Success(()),
// Loosening a ratchet is now allowed
(RatchetState::Tight, RatchetState::Loose(context)) => {
Context::to_nixpkgs_problem(name, context, optional_from.is_some()).into()
}
}
}
}
/// The ratchet value of an attribute
/// for the non-auto-called empty argument check of a single.
///
/// This checks that packages defined in `pkgs/by-name` cannot be overridden
/// with an empty second argument like `callPackage ... { }`.
#[derive(PartialEq, PartialOrd)]
pub enum EmptyNonAutoCalled {
Invalid,
Valid,
}
pub struct EmptyNonAutoCalled;
impl EmptyNonAutoCalled {
/// Validates the non-auto-called empty argument ratchet check for a single package defined in `pkgs/by-name`
fn compare(name: &str, optional_from: Option<&Self>, to: &Self) -> Validation<()> {
let from = optional_from.unwrap_or(&Self::Valid);
if to >= from {
Success(())
} else {
NixpkgsProblem::WrongCallPackage {
relative_package_file: structure::relative_file_for_package(name),
package_name: name.to_owned(),
}
.into()
impl ToNixpkgsProblem for EmptyNonAutoCalled {
fn to_nixpkgs_problem(name: &str, _context: &Self, _existed_before: bool) -> NixpkgsProblem {
NixpkgsProblem::WrongCallPackage {
relative_package_file: structure::relative_file_for_package(name),
package_name: name.to_owned(),
}
}
}

View File

@ -17,10 +17,12 @@ pub fn check_references(
) -> validation::Result<()> {
// The empty argument here is the subpath under the package directory to check
// An empty one means the package directory itself
check_path(relative_package_dir, absolute_package_dir, Path::new("")).context(format!(
"While checking the references in package directory {}",
relative_package_dir.display()
))
check_path(relative_package_dir, absolute_package_dir, Path::new("")).with_context(|| {
format!(
"While checking the references in package directory {}",
relative_package_dir.display()
)
})
}
/// Checks for a specific path to not have references outside
@ -62,7 +64,9 @@ fn check_path(
.map(|entry| {
let entry_subpath = subpath.join(entry.file_name());
check_path(relative_package_dir, absolute_package_dir, &entry_subpath)
.context(format!("Error while recursing into {}", subpath.display()))
.with_context(|| {
format!("Error while recursing into {}", subpath.display())
})
})
.collect_vec()?,
)
@ -70,8 +74,8 @@ fn check_path(
// Only check Nix files
if let Some(ext) = path.extension() {
if ext == OsStr::new("nix") {
check_nix_file(relative_package_dir, absolute_package_dir, subpath).context(
format!("Error while checking Nix file {}", subpath.display()),
check_nix_file(relative_package_dir, absolute_package_dir, subpath).with_context(
|| format!("Error while checking Nix file {}", subpath.display()),
)?
} else {
Success(())
@ -93,13 +97,12 @@ fn check_nix_file(
subpath: &Path,
) -> validation::Result<()> {
let path = absolute_package_dir.join(subpath);
let parent_dir = path.parent().context(format!(
"Could not get parent of path {}",
subpath.display()
))?;
let parent_dir = path
.parent()
.with_context(|| format!("Could not get parent of path {}", subpath.display()))?;
let contents =
read_to_string(&path).context(format!("Could not read file {}", subpath.display()))?;
let contents = read_to_string(&path)
.with_context(|| format!("Could not read file {}", subpath.display()))?;
let root = Root::parse(&contents);
if let Some(error) = root.errors().first() {

View File

@ -10,10 +10,10 @@ pub const PACKAGE_NIX_FILENAME: &str = "package.nix";
pub fn read_dir_sorted(base_dir: &Path) -> anyhow::Result<Vec<fs::DirEntry>> {
let listing = base_dir
.read_dir()
.context(format!("Could not list directory {}", base_dir.display()))?;
.with_context(|| format!("Could not list directory {}", base_dir.display()))?;
let mut shard_entries = listing
.collect::<io::Result<Vec<_>>>()
.context(format!("Could not list directory {}", base_dir.display()))?;
.with_context(|| format!("Could not list directory {}", base_dir.display()))?;
shard_entries.sort_by_key(|entry| entry.file_name());
Ok(shard_entries)
}

View File

@ -0,0 +1 @@
import ../mock-nixpkgs.nix { root = ./.; }

View File

@ -19,6 +19,8 @@ It returns a Nixpkgs-like function that can be auto-called and evaluates to an a
overlays ? [],
# Passed by the checker to make sure a real Nixpkgs isn't influenced by impurities
config ? {},
# Passed by the checker to make sure a real Nixpkgs isn't influenced by impurities
system ? null,
}:
let