cafkafk/nixpkgs
cafkafk/nixpkgs
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

Merge pull request #285089 from tweag/by-name-syntactic-callPackage

Browse source 
tests.nixpkgs-check-by-name: Syntactic `callPackage` detection and allow new package variants
This commit is contained in:
Silvan Mosberger 2024-02-05 20:47:09 +01:00 committed by GitHub
commit 70f9d315a1
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
21 changed files with 913 additions and 255 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

8
pkgs/test/nixpkgs-check-by-name/Cargo.lock generated
View file

@ -213,6 +213,12 @@ version = "0.3.2"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "443144c8cdadd93ebf52ddb4056d257f5b52c04d3c804e657d19eb73fc33668b"
[[package]]
name = "indoc"
version = "2.0.4"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "1e186cfbae8084e513daff4240b4797e342f988cecda4fb6c939150f96315fd8"
[[package]]
name = "is-terminal"
version = "0.4.9"
@ -289,10 +295,12 @@ dependencies = [
"anyhow",
"clap",
"colored",
"indoc",
"itertools",
"lazy_static",
"regex",
"rnix",
"rowan",
"serde",
"serde_json",
"temp-env",

2
pkgs/test/nixpkgs-check-by-name/Cargo.toml
View file

@ -14,6 +14,8 @@ anyhow = "1.0"
lazy_static = "1.4.0"
colored = "2.0.4"
itertools = "0.11.0"
rowan = "0.15.11"
[dev-dependencies]
temp-env = "0.3.5"
indoc = "2.0.4"

1
pkgs/test/nixpkgs-check-by-name/src/eval.nix
View file

@ -76,6 +76,7 @@ let
CallPackage = {
call_package_variant = value._callPackageVariant;
is_derivation = pkgs.lib.isDerivation value;
location = builtins.unsafeGetAttrPos name pkgs;
};
};

368
pkgs/test/nixpkgs-check-by-name/src/eval.rs
View file

@ -1,7 +1,10 @@
use crate::nixpkgs_problem::NixpkgsProblem;
use crate::ratchet;
use crate::structure;
use crate::utils;
use crate::validation::ResultIteratorExt as _;
use crate::validation::{self, Validation::Success};
use crate::NixFileStore;
use std::path::Path;
use anyhow::Context;
@ -48,6 +51,15 @@ struct CallPackageInfo {
call_package_variant: CallPackageVariant,
/// Whether the attribute is a derivation (`lib.isDerivation`)
is_derivation: bool,
location: Option<Location>,
}
/// The structure returned by `builtins.unsafeGetAttrPos`
#[derive(Deserialize, Clone, Debug)]
struct Location {
pub file: PathBuf,
pub line: usize,
pub column: usize,
}
#[derive(Deserialize)]
@ -70,6 +82,7 @@ enum CallPackageVariant {
/// See the `eval.nix` file for how this is achieved on the Nix side
pub fn check_values(
nixpkgs_path: &Path,
nix_file_store: &mut NixFileStore,
package_names: Vec<String>,
keep_nix_path: bool,
) -> validation::Result<ratchet::Nixpkgs> {
@ -142,150 +155,223 @@ pub fn check_values(
)
})?;
let check_result = validation::sequence(attributes.into_iter().map(
|(attribute_name, attribute_value)| {
let relative_package_file = structure::relative_file_for_package(&attribute_name);
use ratchet::RatchetState::*;
use Attribute::*;
use AttributeInfo::*;
use ByNameAttribute::*;
use CallPackageVariant::*;
use NonByNameAttribute::*;
let check_result = match attribute_value {
// The attribute succeeds evaluation and is NOT defined in pkgs/by-name
NonByName(EvalSuccess(attribute_info)) => {
let uses_by_name = match attribute_info {
// In these cases the package doesn't qualify for being in pkgs/by-name,
// so the UsesByName ratchet is already as tight as it can be
NonAttributeSet => Success(NonApplicable),
NonCallPackage => Success(NonApplicable),
// This is the case when the `pkgs/by-name`-internal _internalCallByNamePackageFile
// is used for a package outside `pkgs/by-name`
CallPackage(CallPackageInfo {
call_package_variant: Auto,
..
}) => {
// With the current detection mechanism, this also triggers for aliases
// to pkgs/by-name packages, and there's no good method of
// distinguishing alias vs non-alias.
// Using `config.allowAliases = false` at least currently doesn't work
// because there's nothing preventing people from defining aliases that
// are present even with that disabled.
// In the future we could kind of abuse this behavior to have better
// enforcement of conditional aliases, but for now we just need to not
// give an error.
Success(NonApplicable)
}
// Only derivations can be in pkgs/by-name,
// so this attribute doesn't qualify
CallPackage(CallPackageInfo {
is_derivation: false,
..
}) => Success(NonApplicable),
// The case of an attribute that qualifies:
// - Uses callPackage
// - Is a derivation
CallPackage(CallPackageInfo {
is_derivation: true,
call_package_variant: Manual { path, empty_arg },
}) => Success(Loose(ratchet::CouldUseByName {
call_package_path: path,
empty_arg,
})),
};
uses_by_name.map(|x| ratchet::Package {
manual_definition: Tight,
uses_by_name: x,
})
}
NonByName(EvalFailure) => {
// We don't know anything about this attribute really
Success(ratchet::Package {
// We'll assume that we can't remove any manual definitions, which has the
// minimal drawback that if there was a manual definition that could've
// been removed, fixing the package requires removing the definition, no
// big deal, that's a minor edit.
manual_definition: Tight,
// Regarding whether this attribute could `pkgs/by-name`, we don't really
// know, so return NonApplicable, which has the effect that if a
// package evaluation gets broken temporarily, the fix can remove it from
// pkgs/by-name again. For now this isn't our problem, but in the future we
// might have another check to enforce that evaluation must not be broken.
// The alternative of assuming that it's using `pkgs/by-name` already
// has the problem that if a package evaluation gets broken temporarily,
// fixing it requires a move to pkgs/by-name, which could happen more
// often and isn't really justified.
uses_by_name: NonApplicable,
})
}
ByName(Missing) => NixpkgsProblem::UndefinedAttr {
relative_package_file: relative_package_file.clone(),
package_name: attribute_name.clone(),
}
.into(),
ByName(Existing(NonAttributeSet)) => NixpkgsProblem::NonDerivation {
relative_package_file: relative_package_file.clone(),
package_name: attribute_name.clone(),
}
.into(),
ByName(Existing(NonCallPackage)) => NixpkgsProblem::WrongCallPackage {
relative_package_file: relative_package_file.clone(),
package_name: attribute_name.clone(),
}
.into(),
ByName(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 {
manual_definition: Tight,
uses_by_name: 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.
manual_definition: if *empty_arg { Loose(()) } else { Tight },
uses_by_name: 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))
},
));
let check_result = validation::sequence(
attributes
.into_iter()
.map(|(attribute_name, attribute_value)| {
let check_result = match attribute_value {
Attribute::NonByName(non_by_name_attribute) => handle_non_by_name_attribute(
nixpkgs_path,
nix_file_store,
non_by_name_attribute,
)?,
Attribute::ByName(by_name_attribute) => {
by_name(&attribute_name, by_name_attribute)
}
};
Ok::<_, anyhow::Error>(check_result.map(|value| (attribute_name.clone(), value)))
})
.collect_vec()?,
);
Ok(check_result.map(|elems| ratchet::Nixpkgs {
package_names: elems.iter().map(|(name, _)| name.to_owned()).collect(),
package_map: elems.into_iter().collect(),
}))
}
/// Handles the evaluation result for an attribute in `pkgs/by-name`,
/// turning it into a validation result.
fn by_name(
attribute_name: &str,
by_name_attribute: ByNameAttribute,
) -> validation::Validation<ratchet::Package> {
use ratchet::RatchetState::*;
use AttributeInfo::*;
use ByNameAttribute::*;
use CallPackageVariant::*;
let relative_package_file = structure::relative_file_for_package(attribute_name);
match by_name_attribute {
Missing => NixpkgsProblem::UndefinedAttr {
relative_package_file: relative_package_file.to_owned(),
package_name: attribute_name.to_owned(),
}
.into(),
Existing(NonAttributeSet) => NixpkgsProblem::NonDerivation {
relative_package_file: relative_package_file.to_owned(),
package_name: attribute_name.to_owned(),
}
.into(),
Existing(NonCallPackage) => NixpkgsProblem::WrongCallPackage {
relative_package_file: relative_package_file.to_owned(),
package_name: attribute_name.to_owned(),
}
.into(),
Existing(CallPackage(CallPackageInfo {
is_derivation,
call_package_variant,
..
})) => {
let check_result = if !is_derivation {
NixpkgsProblem::NonDerivation {
relative_package_file: relative_package_file.to_owned(),
package_name: attribute_name.to_owned(),
}
.into()
} else {
Success(())
};
check_result.and(match &call_package_variant {
Auto => Success(ratchet::Package {
manual_definition: Tight,
uses_by_name: Tight,
}),
// TODO: Use the call_package_argument_info_at instead/additionally and
// simplify the eval.nix code
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.
manual_definition: if *empty_arg { Loose(()) } else { Tight },
uses_by_name: Tight,
})
} else {
NixpkgsProblem::WrongCallPackage {
relative_package_file: relative_package_file.to_owned(),
package_name: attribute_name.to_owned(),
}
.into()
}
}
})
}
}
}
/// Handles the evaluation result for an attribute _not_ in `pkgs/by-name`,
/// turning it into a validation result.
fn handle_non_by_name_attribute(
nixpkgs_path: &Path,
nix_file_store: &mut NixFileStore,
non_by_name_attribute: NonByNameAttribute,
) -> validation::Result<ratchet::Package> {
use ratchet::RatchetState::*;
use AttributeInfo::*;
use CallPackageVariant::*;
use NonByNameAttribute::*;
Ok(match non_by_name_attribute {
// The attribute succeeds evaluation and is NOT defined in pkgs/by-name
EvalSuccess(attribute_info) => {
let uses_by_name = match attribute_info {
// In these cases the package doesn't qualify for being in pkgs/by-name,
// so the UsesByName ratchet is already as tight as it can be
NonAttributeSet => Success(NonApplicable),
NonCallPackage => Success(NonApplicable),
// This is the case when the `pkgs/by-name`-internal _internalCallByNamePackageFile
// is used for a package outside `pkgs/by-name`
CallPackage(CallPackageInfo {
call_package_variant: Auto,
..
}) => {
// With the current detection mechanism, this also triggers for aliases
// to pkgs/by-name packages, and there's no good method of
// distinguishing alias vs non-alias.
// Using `config.allowAliases = false` at least currently doesn't work
// because there's nothing preventing people from defining aliases that
// are present even with that disabled.
// In the future we could kind of abuse this behavior to have better
// enforcement of conditional aliases, but for now we just need to not
// give an error.
Success(NonApplicable)
}
// Only derivations can be in pkgs/by-name,
// so this attribute doesn't qualify
CallPackage(CallPackageInfo {
is_derivation: false,
..
}) => Success(NonApplicable),
// A location of None indicates something weird, we can't really know where
// this attribute is defined, probably an alias
CallPackage(CallPackageInfo { location: None, .. }) => Success(Tight),
// The case of an attribute that qualifies:
// - Uses callPackage
// - Is a derivation
CallPackage(CallPackageInfo {
is_derivation: true,
call_package_variant: Manual { .. },
location: Some(location),
}) =>
// We'll use the attribute's location to parse the file that defines it
{
match nix_file_store
.get(&location.file)?
.call_package_argument_info_at(
location.line,
location.column,
nixpkgs_path,
)? {
// If the definition is not of the form `<attr> = callPackage <arg1> <arg2>;`,
// it's generally not possible to migrate to `pkgs/by-name`
None => Success(NonApplicable),
Some(call_package_argument_info) => {
if let Some(ref rel_path) = call_package_argument_info.relative_path {
if rel_path.starts_with(utils::BASE_SUBPATH) {
// Package variants of by-name packages are explicitly allowed according to RFC 140
// https://github.com/NixOS/rfcs/blob/master/rfcs/0140-simple-package-paths.md#package-variants:
//
// foo-variant = callPackage ../by-name/fo/foo/package.nix {
// someFlag = true;
// }
//
// While such definitions could be moved to `pkgs/by-name` by using
// `.override { someFlag = true; }` instead, this changes the semantics in
// relation with overlays.
Success(NonApplicable)
} else {
Success(Loose(call_package_argument_info))
}
} else {
Success(Loose(call_package_argument_info))
}
}
}
}
};
uses_by_name.map(|x| ratchet::Package {
manual_definition: Tight,
uses_by_name: x,
})
}
EvalFailure => {
// We don't know anything about this attribute really
Success(ratchet::Package {
// We'll assume that we can't remove any manual definitions, which has the
// minimal drawback that if there was a manual definition that could've
// been removed, fixing the package requires removing the definition, no
// big deal, that's a minor edit.
manual_definition: Tight,
// Regarding whether this attribute could `pkgs/by-name`, we don't really
// know, so return NonApplicable, which has the effect that if a
// package evaluation gets broken temporarily, the fix can remove it from
// pkgs/by-name again. For now this isn't our problem, but in the future we
// might have another check to enforce that evaluation must not be broken.
// The alternative of assuming that it's using `pkgs/by-name` already
// has the problem that if a package evaluation gets broken temporarily,
// fixing it requires a move to pkgs/by-name, which could happen more
// often and isn't really justified.
uses_by_name: NonApplicable,
})
}
})
}

10
pkgs/test/nixpkgs-check-by-name/src/main.rs
View file

@ -1,4 +1,6 @@
use crate::nix_file::NixFileStore;
mod eval;
mod nix_file;
mod nixpkgs_problem;
mod ratchet;
mod references;
@ -116,6 +118,8 @@ pub fn check_nixpkgs<W: io::Write>(
keep_nix_path: bool,
error_writer: &mut W,
) -> validation::Result<ratchet::Nixpkgs> {
let mut nix_file_store = NixFileStore::default();
Ok({
let nixpkgs_path = nixpkgs_path.canonicalize().with_context(|| {
format!(
@ -132,9 +136,9 @@ pub fn check_nixpkgs<W: io::Write>(
)?;
Success(ratchet::Nixpkgs::default())
} else {
check_structure(&nixpkgs_path)?.result_map(|package_names|
check_structure(&nixpkgs_path, &mut nix_file_store)?.result_map(|package_names|
// Only if we could successfully parse the structure, we do the evaluation checks
eval::check_values(&nixpkgs_path, package_names, keep_nix_path))?
eval::check_values(&nixpkgs_path, &mut nix_file_store, package_names, keep_nix_path))?
}
})
}
@ -169,7 +173,7 @@ mod tests {
// tempfile::tempdir needs to be wrapped in temp_env lock
// because it accesses TMPDIR environment variable.
fn tempdir() -> anyhow::Result<TempDir> {
pub fn tempdir() -> anyhow::Result<TempDir> {
let empty_list: [(&str, Option<&str>); 0] = [];
Ok(temp_env::with_vars(empty_list, tempfile::tempdir)?)
}

510
pkgs/test/nixpkgs-check-by-name/src/nix_file.rs Normal file
View file

@ -0,0 +1,510 @@
//! This is a utility module for interacting with the syntax of Nix files
use crate::utils::LineIndex;
use anyhow::Context;
use rnix::ast;
use rnix::ast::Expr;
use rnix::ast::HasEntry;
use rnix::SyntaxKind;
use rowan::ast::AstNode;
use rowan::TextSize;
use rowan::TokenAtOffset;
use std::collections::hash_map::Entry;
use std::collections::HashMap;
use std::fs::read_to_string;
use std::path::Path;
use std::path::PathBuf;
/// A structure to store parse results of Nix files in memory,
/// making sure that the same file never has to be parsed twice
#[derive(Default)]
pub struct NixFileStore {
entries: HashMap<PathBuf, NixFile>,
}
impl NixFileStore {
/// Get the store entry for a Nix file if it exists, otherwise parse the file, insert it into
/// the store, and return the value
///
/// Note that this function only gives an anyhow::Result::Err for I/O errors.
/// A parse error is anyhow::Result::Ok(Result::Err(error))
pub fn get(&mut self, path: &Path) -> anyhow::Result<&NixFile> {
match self.entries.entry(path.to_owned()) {
Entry::Occupied(entry) => Ok(entry.into_mut()),
Entry::Vacant(entry) => Ok(entry.insert(NixFile::new(path)?)),
}
}
}
/// A structure for storing a successfully parsed Nix file
pub struct NixFile {
/// The parent directory of the Nix file, for more convenient error handling
pub parent_dir: PathBuf,
/// The path to the file itself, for errors
pub path: PathBuf,
pub syntax_root: rnix::Root,
pub line_index: LineIndex,
}
impl NixFile {
/// Creates a new NixFile, failing for I/O or parse errors
fn new(path: impl AsRef<Path>) -> anyhow::Result<NixFile> {
let Some(parent_dir) = path.as_ref().parent() else {
anyhow::bail!("Could not get parent of path {}", path.as_ref().display())
};
let contents = read_to_string(&path)
.with_context(|| format!("Could not read file {}", path.as_ref().display()))?;
let line_index = LineIndex::new(&contents);
// NOTE: There's now another Nixpkgs CI check to make sure all changed Nix files parse
// correctly, though that uses mainline Nix instead of rnix, so it doesn't give the same
// errors. In the future we should unify these two checks, ideally moving the other CI
// check into this tool as well and checking for both mainline Nix and rnix.
rnix::Root::parse(&contents)
// rnix's ::ok returns Result<_, _> , so no error is thrown away like it would be with
// std::result's ::ok
.ok()
.map(|syntax_root| NixFile {
parent_dir: parent_dir.to_path_buf(),
path: path.as_ref().to_owned(),
syntax_root,
line_index,
})
.with_context(|| format!("Could not parse file {} with rnix", path.as_ref().display()))
}
}
/// Information about callPackage arguments
#[derive(Debug, PartialEq)]
pub struct CallPackageArgumentInfo {
/// The relative path of the first argument, or `None` if it's not a path.
pub relative_path: Option<PathBuf>,
/// Whether the second argument is an empty attribute set
pub empty_arg: bool,
}
impl NixFile {
/// Returns information about callPackage arguments for an attribute at a specific line/column
/// index.
/// If the location is not of the form `<attr> = callPackage <arg1> <arg2>;`, `None` is
/// returned.
/// This function only returns `Err` for problems that can't be caused by the Nix contents,
/// but rather problems in this programs code itself.
///
/// This is meant to be used with the location returned from `builtins.unsafeGetAttrPos`, e.g.:
/// - Create file `default.nix` with contents
/// ```nix
/// self: {
/// foo = self.callPackage ./default.nix { };
/// }
/// ```
/// - Evaluate
/// ```nix
/// builtins.unsafeGetAttrPos "foo" (import ./default.nix { })
/// ```
/// results in `{ file = ./default.nix; line = 2; column = 3; }`
/// - Get the NixFile for `.file` from a `NixFileStore`
/// - Call this function with `.line`, `.column` and `relative_to` as the (absolute) current directory
///
/// You'll get back
/// ```rust
/// Some(CallPackageArgumentInfo { path = Some("default.nix"), empty_arg: true })
/// ```
///
/// Note that this also returns the same for `pythonPackages.callPackage`. It doesn't make an
/// attempt at distinguishing this.
pub fn call_package_argument_info_at(
&self,
line: usize,
column: usize,
relative_to: &Path,
) -> anyhow::Result<Option<CallPackageArgumentInfo>> {
let Some(attrpath_value) = self.attrpath_value_at(line, column)? else {
return Ok(None);
};
self.attrpath_value_call_package_argument_info(attrpath_value, relative_to)
}
// Internal function mainly to make it independently testable
fn attrpath_value_at(
&self,
line: usize,
column: usize,
) -> anyhow::Result<Option<ast::AttrpathValue>> {
let index = self.line_index.fromlinecolumn(line, column);
let token_at_offset = self
.syntax_root
.syntax()
.token_at_offset(TextSize::from(index as u32));
// The token_at_offset function takes indices to mean a location _between_ characters,
// which in this case is some spacing followed by the attribute name:
//
// foo = 10;
// /\
// This is the token offset, we get both the (newline + indentation) on the left side,
// and the attribute name on the right side.
let TokenAtOffset::Between(_space, token) = token_at_offset else {
anyhow::bail!("Line {line} column {column} in {} is not the start of a token, but rather {token_at_offset:?}", self.path.display())
};
// token looks like "foo"
let Some(node) = token.parent() else {
anyhow::bail!(
"Token on line {line} column {column} in {} does not have a parent node: {token:?}",
self.path.display()
)
};
// node looks like "foo"
let Some(attrpath_node) = node.parent() else {
anyhow::bail!(
"Node in {} does not have a parent node: {node:?}",
self.path.display()
)
};
if attrpath_node.kind() != SyntaxKind::NODE_ATTRPATH {
// This can happen for e.g. `inherit foo`, so definitely not a syntactic `callPackage`
return Ok(None);
}
// attrpath_node looks like "foo.bar"
let Some(attrpath_value_node) = attrpath_node.parent() else {
anyhow::bail!(
"Attribute path node in {} does not have a parent node: {attrpath_node:?}",
self.path.display()
)
};
if !ast::AttrpathValue::can_cast(attrpath_value_node.kind()) {
anyhow::bail!(
"Node in {} is not an attribute path value node: {attrpath_value_node:?}",
self.path.display()
)
}
// attrpath_value_node looks like "foo.bar = 10;"
// unwrap is fine because we confirmed that we can cast with the above check.
// We could avoid this `unwrap` for a `clone`, since `cast` consumes the argument,
// but we still need it for the error message when the cast fails.
Ok(Some(ast::AttrpathValue::cast(attrpath_value_node).unwrap()))
}
// Internal function mainly to make attrpath_value_at independently testable
fn attrpath_value_call_package_argument_info(
&self,
attrpath_value: ast::AttrpathValue,
relative_to: &Path,
) -> anyhow::Result<Option<CallPackageArgumentInfo>> {
let Some(attrpath) = attrpath_value.attrpath() else {
anyhow::bail!("attrpath value node doesn't have an attrpath: {attrpath_value:?}")
};
// At this point we know it's something like `foo...bar = ...`
if attrpath.attrs().count() > 1 {
// If the attribute path has multiple entries, the left-most entry is an attribute and
// can't be a `callPackage`.
//
// FIXME: `builtins.unsafeGetAttrPos` will return the same position for all attribute
// paths and we can't really know which one it is. We could have a case like
// `foo.bar = callPackage ... { }` and trying to determine if `bar` is a `callPackage`,
// where this is not correct.
// However, this case typically doesn't occur anyways,
// because top-level packages wouldn't be nested under an attribute set.
return Ok(None);
}
let Some(value) = attrpath_value.value() else {
anyhow::bail!("attrpath value node doesn't have a value: {attrpath_value:?}")
};
// At this point we know it's something like `foo = ...`
let Expr::Apply(apply1) = value else {
// Not even a function call, instead something like `foo = null`
return Ok(None);
};
let Some(function1) = apply1.lambda() else {
anyhow::bail!("apply node doesn't have a lambda: {apply1:?}")
};
let Some(arg1) = apply1.argument() else {
anyhow::bail!("apply node doesn't have an argument: {apply1:?}")
};
// At this point we know it's something like `foo = <fun> <arg>`.
// For a callPackage, `<fun>` would be `callPackage ./file` and `<arg>` would be `{ }`
let empty_arg = if let Expr::AttrSet(attrset) = arg1 {
// We can only statically determine whether the argument is empty if it's an attribute
// set _expression_, even though other kind of expressions could evaluate to an attribute
// set _value_. But this is what we want anyways
attrset.entries().next().is_none()
} else {
false
};
// Because callPackage takes two curried arguments, the first function needs to be a
// function call itself
let Expr::Apply(apply2) = function1 else {
// Not a callPackage, instead something like `foo = import ./foo`
return Ok(None);
};
let Some(function2) = apply2.lambda() else {
anyhow::bail!("apply node doesn't have a lambda: {apply2:?}")
};
let Some(arg2) = apply2.argument() else {
anyhow::bail!("apply node doesn't have an argument: {apply2:?}")
};
// At this point we know it's something like `foo = <fun2> <arg2> <arg1>`.
// For a callPackage, `<fun2>` would be `callPackage`, `<arg2>` would be `./file`
// Check that <arg2> is a path expression
let path = if let Expr::Path(actual_path) = arg2 {
// Try to statically resolve the path and turn it into a nixpkgs-relative path
if let ResolvedPath::Within(p) = self.static_resolve_path(actual_path, relative_to) {
Some(p)
} else {
// We can't statically know an existing path inside Nixpkgs used as <arg2>
None
}
} else {
// <arg2> is not a path, but rather e.g. an inline expression
None
};
// Check that <fun2> is an identifier, or an attribute path with an identifier at the end
let ident = match function2 {
Expr::Ident(ident) => {
// This means it's something like `foo = callPackage <arg2> <arg1>`
ident
}
Expr::Select(select) => {
// This means it's something like `foo = self.callPackage <arg2> <arg1>`.
// We also end up here for e.g. `pythonPackages.callPackage`, but the
// callPackage-mocking method will take care of not triggering for this case.
if select.default_expr().is_some() {
// Very odd case, but this would be `foo = self.callPackage or true ./test.nix {}
// (yes this is valid Nix code)
return Ok(None);
}
let Some(attrpath) = select.attrpath() else {
anyhow::bail!("select node doesn't have an attrpath: {select:?}")
};
let Some(last) = attrpath.attrs().last() else {
// This case shouldn't be possible, it would be `foo = self. ./test.nix {}`,
// which shouldn't parse
anyhow::bail!("select node has an empty attrpath: {select:?}")
};
if let ast::Attr::Ident(ident) = last {
ident
} else {
// Here it's something like `foo = self."callPackage" /test.nix {}`
// which we're not gonna bother with
return Ok(None);
}
}
// Any other expression we're not gonna treat as callPackage
_ => return Ok(None),
};
let Some(token) = ident.ident_token() else {
anyhow::bail!("ident node doesn't have a token: {ident:?}")
};
if token.text() == "callPackage" {
Ok(Some(CallPackageArgumentInfo {
relative_path: path,
empty_arg,
}))
} else {
Ok(None)
}
}
}
/// The result of trying to statically resolve a Nix path expression
pub enum ResolvedPath {
/// Something like `./foo/${bar}/baz`, can't be known statically
Interpolated,
/// Something like `<nixpkgs>`, can't be known statically
SearchPath,
/// Path couldn't be resolved due to an IO error,
/// e.g. if the path doesn't exist or you don't have the right permissions
Unresolvable(std::io::Error),
/// The path is outside the given absolute path
Outside,
/// The path is within the given absolute path.
/// The `PathBuf` is the relative path under the given absolute path.
Within(PathBuf),
}
impl NixFile {
/// Statically resolves a Nix path expression and checks that it's within an absolute path
///
/// E.g. for the path expression `./bar.nix` in `./foo.nix` and an absolute path of the
/// current directory, the function returns `ResolvedPath::Within(./bar.nix)`
pub fn static_resolve_path(&self, node: ast::Path, relative_to: &Path) -> ResolvedPath {
if node.parts().count() != 1 {
// If there's more than 1 interpolated part, it's of the form `./foo/${bar}/baz`.
return ResolvedPath::Interpolated;
}
let text = node.to_string();
if text.starts_with('<') {
// A search path like `<nixpkgs>`. There doesn't appear to be better way to detect
// these in rnix
return ResolvedPath::SearchPath;
}
// Join the file's parent directory and the path expression, then resolve it
// FIXME: Expressions like `../../../../foo/bar/baz/qux` or absolute paths
// may resolve close to the original file, but may have left the relative_to.
// That should be checked more strictly
match self.parent_dir.join(Path::new(&text)).canonicalize() {
Err(resolution_error) => ResolvedPath::Unresolvable(resolution_error),
Ok(resolved) => {
// Check if it's within relative_to
match resolved.strip_prefix(relative_to) {
Err(_prefix_error) => ResolvedPath::Outside,
Ok(suffix) => ResolvedPath::Within(suffix.to_path_buf()),
}
}
}
}
}
#[cfg(test)]
mod tests {
use super::*;
use crate::tests;
use indoc::indoc;
#[test]
fn detects_attributes() -> anyhow::Result<()> {
let temp_dir = tests::tempdir()?;
let file = temp_dir.path().join("file.nix");
let contents = indoc! {r#"
toInherit: {
foo = 1;
"bar" = 2;
${"baz"} = 3;
"${"qux"}" = 4;
# A
quux
# B
=
# C
5
# D
;
# E
/**/quuux/**/=/**/5/**/;/*E*/
inherit toInherit;
}
"#};
std::fs::write(&file, contents)?;
let nix_file = NixFile::new(&file)?;
// These are builtins.unsafeGetAttrPos locations for the attributes
let cases = [
(2, 3, Some("foo = 1;")),
(3, 3, Some(r#""bar" = 2;"#)),
(4, 3, Some(r#"${"baz"} = 3;"#)),
(5, 3, Some(r#""${"qux"}" = 4;"#)),
(8, 3, Some("quux\n # B\n =\n # C\n 5\n # D\n ;")),
(17, 7, Some("quuux/**/=/**/5/**/;")),
(19, 10, None),
];
for (line, column, expected_result) in cases {
let actual_result = nix_file
.attrpath_value_at(line, column)?
.map(|node| node.to_string());
assert_eq!(actual_result.as_deref(), expected_result);
}
Ok(())
}
#[test]
fn detects_call_package() -> anyhow::Result<()> {
let temp_dir = tests::tempdir()?;
let file = temp_dir.path().join("file.nix");
let contents = indoc! {r#"
self: with self; {
a.sub = null;
b = null;
c = import ./file.nix;
d = import ./file.nix { };
e = pythonPackages.callPackage ./file.nix { };
f = callPackage ./file.nix { };
g = callPackage ({ }: { }) { };
h = callPackage ./file.nix { x = 0; };
i = callPackage ({ }: { }) (let in { });
}
"#};
std::fs::write(&file, contents)?;
let nix_file = NixFile::new(&file)?;
let cases = [
(2, None),
(3, None),
(4, None),
(5, None),
(
6,
Some(CallPackageArgumentInfo {
relative_path: Some(PathBuf::from("file.nix")),
empty_arg: true,
}),
),
(
7,
Some(CallPackageArgumentInfo {
relative_path: Some(PathBuf::from("file.nix")),
empty_arg: true,
}),
),
(
8,
Some(CallPackageArgumentInfo {
relative_path: None,
empty_arg: true,
}),
),
(
9,
Some(CallPackageArgumentInfo {
relative_path: Some(PathBuf::from("file.nix")),
empty_arg: false,
}),
),
(
10,
Some(CallPackageArgumentInfo {
relative_path: None,
empty_arg: false,
}),
),
];
for (line, expected_result) in cases {
let actual_result = nix_file.call_package_argument_info_at(line, 3, temp_dir.path())?;
assert_eq!(actual_result, expected_result);
}
Ok(())
}
}

14
pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
View file

@ -1,6 +1,5 @@
use crate::structure;
use crate::utils::PACKAGE_NIX_FILENAME;
use rnix::parser::ParseError;
use std::ffi::OsString;
use std::fmt;
use std::io;
@ -58,11 +57,6 @@ pub enum NixpkgsProblem {
subpath: PathBuf,
io_error: io::Error,
},
CouldNotParseNix {
relative_package_dir: PathBuf,
subpath: PathBuf,
error: ParseError,
},
PathInterpolation {
relative_package_dir: PathBuf,
subpath: PathBuf,
@ -184,14 +178,6 @@ impl fmt::Display for NixpkgsProblem {
relative_package_dir.display(),
subpath.display(),
),
NixpkgsProblem::CouldNotParseNix { relative_package_dir, subpath, error } =>
write!(
f,
"{}: File {} could not be parsed by rnix: {}",
relative_package_dir.display(),
subpath.display(),
error,
),
NixpkgsProblem::PathInterpolation { relative_package_dir, subpath, line, text } =>
write!(
f,

16
pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
View file

@ -2,11 +2,11 @@
//!
//! Each type has a `compare` method that validates the ratchet checks for that item.
use crate::nix_file::CallPackageArgumentInfo;
use crate::nixpkgs_problem::NixpkgsProblem;
use crate::structure;
use crate::validation::{self, Validation, Validation::Success};
use std::collections::HashMap;
use std::path::PathBuf;
/// The ratchet value for the entirety of Nixpkgs.
#[derive(Default)]
@ -148,16 +148,8 @@ impl ToNixpkgsProblem for ManualDefinition {
/// It also checks that once a package uses pkgs/by-name, it can't switch back to all-packages.nix
pub enum UsesByName {}
#[derive(Clone)]
pub struct CouldUseByName {
/// The first callPackage argument, used for better errors
pub call_package_path: Option<PathBuf>,
/// Whether the second callPackage argument is empty, used for better errors
pub empty_arg: bool,
}
impl ToNixpkgsProblem for UsesByName {
type ToContext = CouldUseByName;
type ToContext = CallPackageArgumentInfo;
fn to_nixpkgs_problem(
name: &str,
@ -167,13 +159,13 @@ impl ToNixpkgsProblem for UsesByName {
if let Some(()) = optional_from {
NixpkgsProblem::MovedOutOfByName {
package_name: name.to_owned(),
call_package_path: to.call_package_path.clone(),
call_package_path: to.relative_path.clone(),
empty_arg: to.empty_arg,
}
} else {
NixpkgsProblem::NewPackageNotUsingByName {
package_name: name.to_owned(),
call_package_path: to.call_package_path.clone(),
call_package_path: to.relative_path.clone(),
empty_arg: to.empty_arg,
}
}

154
pkgs/test/nixpkgs-check-by-name/src/references.rs
View file

@ -1,23 +1,32 @@
use crate::nixpkgs_problem::NixpkgsProblem;
use crate::utils;
use crate::utils::LineIndex;
use crate::validation::{self, ResultIteratorExt, Validation::Success};
use crate::NixFileStore;
use anyhow::Context;
use rnix::{Root, SyntaxKind::NODE_PATH};
use rowan::ast::AstNode;
use std::ffi::OsStr;
use std::fs::read_to_string;
use std::path::Path;
/// Check that every package directory in pkgs/by-name doesn't link to outside that directory.
/// Both symlinks and Nix path expressions are checked.
pub fn check_references(
nix_file_store: &mut NixFileStore,
relative_package_dir: &Path,
absolute_package_dir: &Path,
) -> 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("")).with_context(|| {
// The first subpath to check is the package directory itself, which we can represent as an
// empty path, since the absolute package directory gets prepended to this.
// We don't use `./.` to keep the error messages cleaner
// (there's no canonicalisation going on underneath)
let subpath = Path::new("");
check_path(
nix_file_store,
relative_package_dir,
absolute_package_dir,
subpath,
)
.with_context(|| {
format!(
"While checking the references in package directory {}",
relative_package_dir.display()
@ -26,7 +35,12 @@ pub fn check_references(
}
/// Checks for a specific path to not have references outside
///
/// The subpath is the relative path within the package directory we're currently checking.
/// A relative path so that the error messages don't get absolute paths (which are messy in CI).
/// The absolute package directory gets prepended before doing anything with it though.
fn check_path(
nix_file_store: &mut NixFileStore,
relative_package_dir: &Path,
absolute_package_dir: &Path,
subpath: &Path,
@ -62,21 +76,27 @@ fn check_path(
utils::read_dir_sorted(&path)?
.into_iter()
.map(|entry| {
let entry_subpath = subpath.join(entry.file_name());
check_path(relative_package_dir, absolute_package_dir, &entry_subpath)
.with_context(|| {
format!("Error while recursing into {}", subpath.display())
})
check_path(
nix_file_store,
relative_package_dir,
absolute_package_dir,
&subpath.join(entry.file_name()),
)
})
.collect_vec()?,
.collect_vec()
.with_context(|| format!("Error while recursing into {}", subpath.display()))?,
)
} else if path.is_file() {
// 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).with_context(
|| format!("Error while checking Nix file {}", subpath.display()),
)?
check_nix_file(
nix_file_store,
relative_package_dir,
absolute_package_dir,
subpath,
)
.with_context(|| format!("Error while checking Nix file {}", subpath.display()))?
} else {
Success(())
}
@ -92,91 +112,63 @@ fn check_path(
/// Check whether a nix file contains path expression references pointing outside the package
/// directory
fn check_nix_file(
nix_file_store: &mut NixFileStore,
relative_package_dir: &Path,
absolute_package_dir: &Path,
subpath: &Path,
) -> validation::Result<()> {
let path = absolute_package_dir.join(subpath);
let parent_dir = path
.parent()
.with_context(|| format!("Could not get parent of path {}", subpath.display()))?;
let contents = read_to_string(&path)
.with_context(|| format!("Could not read file {}", subpath.display()))?;
let nix_file = nix_file_store.get(&path)?;
let root = Root::parse(&contents);
if let Some(error) = root.errors().first() {
// NOTE: There's now another Nixpkgs CI check to make sure all changed Nix files parse
// correctly, though that uses mainline Nix instead of rnix, so it doesn't give the same
// errors. In the future we should unify these two checks, ideally moving the other CI
// check into this tool as well and checking for both mainline Nix and rnix.
return Ok(NixpkgsProblem::CouldNotParseNix {
relative_package_dir: relative_package_dir.to_path_buf(),
subpath: subpath.to_path_buf(),
error: error.clone(),
}
.into());
}
let line_index = LineIndex::new(&contents);
Ok(validation::sequence_(root.syntax().descendants().map(
|node| {
Ok(validation::sequence_(
nix_file.syntax_root.syntax().descendants().map(|node| {
let text = node.text().to_string();
let line = line_index.line(node.text_range().start().into());
let line = nix_file.line_index.line(node.text_range().start().into());
if node.kind() != NODE_PATH {
// We're only interested in Path expressions
Success(())
} else if node.children().count() != 0 {
// Filters out ./foo/${bar}/baz
// TODO: We can just check ./foo
NixpkgsProblem::PathInterpolation {
// We're only interested in Path expressions
let Some(path) = rnix::ast::Path::cast(node) else {
return Success(());
};
use crate::nix_file::ResolvedPath;
match nix_file.static_resolve_path(path, absolute_package_dir) {
ResolvedPath::Interpolated => NixpkgsProblem::PathInterpolation {
relative_package_dir: relative_package_dir.to_path_buf(),
subpath: subpath.to_path_buf(),
line,
text,
}
.into()
} else if text.starts_with('<') {
// Filters out search paths like <nixpkgs>
NixpkgsProblem::SearchPath {
.into(),
ResolvedPath::SearchPath => NixpkgsProblem::SearchPath {
relative_package_dir: relative_package_dir.to_path_buf(),
subpath: subpath.to_path_buf(),
line,
text,
}
.into()
} else {
// Resolves the reference of the Nix path
// turning `../baz` inside `/foo/bar/default.nix` to `/foo/baz`
match parent_dir.join(Path::new(&text)).canonicalize() {
Ok(target) => {
// Then checking if it's still in the package directory
// No need to handle the case of it being inside the directory, since we scan through the
// entire directory recursively anyways
if let Err(_prefix_error) = target.strip_prefix(absolute_package_dir) {
NixpkgsProblem::OutsidePathReference {
relative_package_dir: relative_package_dir.to_path_buf(),
subpath: subpath.to_path_buf(),
line,
text,
}
.into()
} else {
Success(())
}
}
Err(e) => NixpkgsProblem::UnresolvablePathReference {
relative_package_dir: relative_package_dir.to_path_buf(),
subpath: subpath.to_path_buf(),
line,
text,
io_error: e,
}
.into(),
.into(),
ResolvedPath::Outside => NixpkgsProblem::OutsidePathReference {
relative_package_dir: relative_package_dir.to_path_buf(),
subpath: subpath.to_path_buf(),
line,
text,
}
.into(),
ResolvedPath::Unresolvable(e) => NixpkgsProblem::UnresolvablePathReference {
relative_package_dir: relative_package_dir.to_path_buf(),
subpath: subpath.to_path_buf(),
line,
text,
io_error: e,
}
.into(),
ResolvedPath::Within(..) => {
// No need to handle the case of it being inside the directory, since we scan through the
// entire directory recursively anyways
Success(())
}
}
},
)))
}),
))
}

16
pkgs/test/nixpkgs-check-by-name/src/structure.rs
View file

@ -3,6 +3,7 @@ use crate::references;
use crate::utils;
use crate::utils::{BASE_SUBPATH, PACKAGE_NIX_FILENAME};
use crate::validation::{self, ResultIteratorExt, Validation::Success};
use crate::NixFileStore;
use itertools::concat;
use lazy_static::lazy_static;
use regex::Regex;
@ -34,7 +35,10 @@ pub fn relative_file_for_package(package_name: &str) -> PathBuf {
/// Check the structure of Nixpkgs, returning the attribute names that are defined in
/// `pkgs/by-name`
pub fn check_structure(path: &Path) -> validation::Result<Vec<String>> {
pub fn check_structure(
path: &Path,
nix_file_store: &mut NixFileStore,
) -> validation::Result<Vec<String>> {
let base_dir = path.join(BASE_SUBPATH);
let shard_results = utils::read_dir_sorted(&base_dir)?
@ -88,7 +92,13 @@ pub fn check_structure(path: &Path) -> validation::Result<Vec<String>> {
let package_results = entries
.into_iter()
.map(|package_entry| {
check_package(path, &shard_name, shard_name_valid, package_entry)
check_package(
nix_file_store,
path,
&shard_name,
shard_name_valid,
package_entry,
)
})
.collect_vec()?;
@ -102,6 +112,7 @@ pub fn check_structure(path: &Path) -> validation::Result<Vec<String>> {
}
fn check_package(
nix_file_store: &mut NixFileStore,
path: &Path,
shard_name: &str,
shard_name_valid: bool,
@ -161,6 +172,7 @@ fn check_package(
});
let result = result.and(references::check_references(
nix_file_store,
&relative_package_dir,
&path.join(&relative_package_dir),
)?);

48
pkgs/test/nixpkgs-check-by-name/src/utils.rs
View file

@ -35,12 +35,13 @@ impl LineIndex {
// the vec
for split in s.split_inclusive('\n') {
index += split.len();
newlines.push(index);
newlines.push(index - 1);
}
LineIndex { newlines }
}
/// Returns the line number for a string index
/// Returns the line number for a string index.
/// If the index points to a newline, returns the line number before the newline
pub fn line(&self, index: usize) -> usize {
match self.newlines.binary_search(&index) {
// +1 because lines are 1-indexed
@ -48,4 +49,47 @@ impl LineIndex {
Err(x) => x + 1,
}
}
/// Returns the string index for a line and column.
pub fn fromlinecolumn(&self, line: usize, column: usize) -> usize {
// If it's the 1th line, the column is the index
if line == 1 {
// But columns are 1-indexed
column - 1
} else {
// For the nth line, we add the index of the (n-1)st newline to the column,
// and remove one more from the index since arrays are 0-indexed.
// Then add the 1-indexed column to get not the newline index itself,
// but rather the index of the position on the next line
self.newlines[line - 2] + column
}
}
}
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn line_index() {
let line_index = LineIndex::new("a\nbc\n\ndef\n");
let pairs = [
(0, 1, 1),
(1, 1, 2),
(2, 2, 1),
(3, 2, 2),
(4, 2, 3),
(5, 3, 1),
(6, 4, 1),
(7, 4, 2),
(8, 4, 3),
(9, 4, 4),
];
for (index, line, column) in pairs {
assert_eq!(line_index.line(index), line);
assert_eq!(line_index.fromlinecolumn(line, column), index);
}
}
}

7
pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/all-packages.nix Normal file
View file

@ -0,0 +1,7 @@
self: super: {
set = self.callPackages ({ callPackage }: {
foo = callPackage ({ someDrv }: someDrv) { };
}) { };
inherit (self.set) foo;
}

1
pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/default.nix Normal file
View file

@ -0,0 +1 @@
import <test-nixpkgs> { root = ./.; }

0
pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/pkgs/by-name/README.md Normal file
View file

5
pkgs/test/nixpkgs-check-by-name/tests/package-variants/all-packages.nix Normal file
View file

@ -0,0 +1,5 @@
self: super: {
foo-variant-unvarianted = self.callPackage ./package.nix { };
foo-variant-new = self.callPackage ./pkgs/by-name/fo/foo/package.nix { };
}

3
pkgs/test/nixpkgs-check-by-name/tests/package-variants/base/all-packages.nix Normal file
View file

@ -0,0 +1,3 @@
self: super: {
foo-variant-unvarianted = self.callPackage ./pkgs/by-name/fo/foo/package.nix { };
}

1
pkgs/test/nixpkgs-check-by-name/tests/package-variants/base/default.nix Normal file
View file

@ -0,0 +1 @@
import <test-nixpkgs> { root = ./.; }

1
pkgs/test/nixpkgs-check-by-name/tests/package-variants/base/pkgs/by-name/fo/foo/package.nix Normal file
View file

@ -0,0 +1 @@
{ someDrv }: someDrv

1
pkgs/test/nixpkgs-check-by-name/tests/package-variants/default.nix Normal file
View file

@ -0,0 +1 @@
import <test-nixpkgs> { root = ./.; }

1
pkgs/test/nixpkgs-check-by-name/tests/package-variants/package.nix Normal file
View file

@ -0,0 +1 @@
{ someDrv }: someDrv

1
pkgs/test/nixpkgs-check-by-name/tests/package-variants/pkgs/by-name/fo/foo/package.nix Normal file
View file

@ -0,0 +1 @@
{ someDrv }: someDrv