dev/nixpkgs
1
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

tests.nixpkgs-check-by-name: Improve code

Browse Source 
- Detect manual use of _internalCallByNamePackageFile for packages in
  `pkgs/by-name` (can't be done for others though)
- Separate error message for when attribute locations can't be
  determined for `pkgs/by-name` attributes
- Much better structure of the code in eval.rs, representing more
  closely what is being checked
- Much more extensive comments
This commit is contained in:
Silvan Mosberger 2024-02-08 00:03:55 +01:00
parent ebbe86306f
commit 89a7afabf8
5 changed files with 335 additions and 221 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

76
pkgs/test/nixpkgs-check-by-name/src/eval.nix
View File

@ -1,5 +1,5 @@
# Takes a path to nixpkgs and a path to the json-encoded list of attributes to check.
# Returns an value containing information on each requested attribute,
# Takes a path to nixpkgs and a path to the json-encoded list of `pkgs/by-name` attributes.
# Returns a value containing information on all Nixpkgs attributes
# which is decoded on the Rust side.
# See ./eval.rs for the meaning of the returned values
{
@ -9,33 +9,28 @@
let
attrs = builtins.fromJSON (builtins.readFile attrsPath);
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.
# We need to check whether attributes are defined manually e.g. in
# `all-packages.nix`, automatically by the `pkgs/by-name` overlay, or
# neither. The only way to do so is to override `callPackage` and
# `_internalCallByNamePackageFile` with our own version that adds this
# information to the result, and then try to access it.
overlay = final: prev: {
# Information for attributes defined using `callPackage`
# Adds information to each attribute about whether it's manually defined using `callPackage`
callPackage = fn: args:
addVariantInfo (prev.callPackage fn args) {
Manual = {
path =
if builtins.isPath fn then
removeNixpkgsPrefix (toString fn)
else
null;
empty_arg =
args == { };
};
# This is a manual definition of the attribute, and it's a callPackage, specifically a semantic callPackage
ManualDefinition.is_semantic_call_package = true;
};
# Information for attributes that are auto-called from pkgs/by-name.
# This internal attribute is only used by pkgs/by-name
# Adds information to each attribute about whether it's automatically
# defined by the `pkgs/by-name` overlay. This internal attribute is only
# used by that overlay.
# This overrides the above `callPackage` information (we don't need that
# one, since `pkgs/by-name` always uses `callPackage` underneath.
_internalCallByNamePackageFile = file:
addVariantInfo (prev._internalCallByNamePackageFile file) {
Auto = null;
AutoDefinition = null;
};
};
@ -50,7 +45,7 @@ let
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.
# so just don't return the value directly and treat it as if it wasn't a callPackage.
value;
pkgs = import nixpkgsPath {
@ -62,30 +57,33 @@ let
system = "x86_64-linux";
};
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;
location = builtins.unsafeGetAttrPos name pkgs;
# See AttributeInfo in ./eval.rs for the meaning of this
attrInfo = name: value: {
location = builtins.unsafeGetAttrPos name pkgs;
attribute_variant =
if ! builtins.isAttrs value then
{ NonAttributeSet = null; }
else
{
AttributeSet = {
is_derivation = pkgs.lib.isDerivation value;
definition_variant =
if ! value ? _callPackageVariant then
{ ManualDefinition.is_semantic_call_package = false; }
else
value._callPackageVariant;
};
};
};
};
# Information on all attributes that are in pkgs/by-name.
byNameAttrs = builtins.listToAttrs (map (name: {
inherit name;
value.ByName =
if ! pkgs ? ${name} then
{ Missing = null; }
else
# Evaluation failures are not allowed, so don't try to catch them
{ Existing = attrInfo name pkgs.${name}; };
}) attrs);
@ -93,6 +91,8 @@ let
# We need this to enforce pkgs/by-name for new packages
nonByNameAttrs = builtins.mapAttrs (name: value:
let
# Packages outside `pkgs/by-name` often fail evaluation,
# so we need to handle that
output = attrInfo name value;
result = builtins.tryEval (builtins.deepSeq output null);
in

459
pkgs/test/nixpkgs-check-by-name/src/eval.rs
View File

@ -37,24 +37,13 @@ enum ByNameAttribute {
}
#[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,
struct AttributeInfo {
/// The location of the attribute as returned by `builtins.unsafeGetAttrPos`
location: Option<Location>,
attribute_variant: AttributeVariant,
}
/// The structure returned by `builtins.unsafeGetAttrPos`
/// The structure returned by a successful `builtins.unsafeGetAttrPos`
#[derive(Deserialize, Clone, Debug)]
struct Location {
pub file: PathBuf,
@ -63,17 +52,28 @@ struct Location {
}
#[derive(Deserialize)]
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
Auto,
/// The attribute is defined as a pkgs.callPackage <path> <args>,
/// and overridden by all-packages.nix
Manual {
/// The <path> argument or None if it's not a path
path: Option<PathBuf>,
/// true if <args> is { }
empty_arg: bool,
pub enum AttributeVariant {
/// The attribute is not an attribute set, we're limited in the amount of information we can get
/// from it (though it's obviously not a derivation)
NonAttributeSet,
AttributeSet {
/// Whether the attribute is a derivation (`lib.isDerivation`)
is_derivation: bool,
/// The type of callPackage
definition_variant: DefinitionVariant,
},
}
#[derive(Deserialize)]
pub enum DefinitionVariant {
/// An automatic definition by the `pkgs/by-name` overlay
/// Though it's detected using the internal _internalCallByNamePackageFile attribute,
/// which can in theory also be used by other code
AutoDefinition,
/// A manual definition of the attribute, typically in `all-packages.nix`
ManualDefinition {
/// Whether the attribute is defined as `pkgs.callPackage ...` or something else.
is_semantic_call_package: bool,
},
}
@ -165,9 +165,12 @@ pub fn check_values(
nix_file_store,
non_by_name_attribute,
)?,
Attribute::ByName(by_name_attribute) => {
by_name(&attribute_name, by_name_attribute)
}
Attribute::ByName(by_name_attribute) => by_name(
nix_file_store,
nixpkgs_path,
&attribute_name,
by_name_attribute,
)?,
};
Ok::<_, anyhow::Error>(check_result.map(|value| (attribute_name.clone(), value)))
})
@ -183,78 +186,168 @@ pub fn check_values(
/// Handles the evaluation result for an attribute in `pkgs/by-name`,
/// turning it into a validation result.
fn by_name(
nix_file_store: &mut NixFileStore,
nixpkgs_path: &Path,
attribute_name: &str,
by_name_attribute: ByNameAttribute,
) -> validation::Validation<ratchet::Package> {
) -> validation::Result<ratchet::Package> {
use ratchet::RatchetState::*;
use AttributeInfo::*;
use ByNameAttribute::*;
use CallPackageVariant::*;
let relative_package_file = structure::relative_file_for_package(attribute_name);
let absolute_package_file = nixpkgs_path.join(&relative_package_file);
match by_name_attribute {
Missing => NixpkgsProblem::UndefinedAttr {
relative_package_file: relative_package_file.to_owned(),
package_name: attribute_name.to_owned(),
// At this point we know that `pkgs/by-name/fo/foo/package.nix` has to exists.
// This match decides whether the attribute `foo` is defined accordingly
// and whether a legacy manual definition could be removed
let manual_definition_result = match by_name_attribute {
// The attribute is missing
Missing => {
// This indicates a bug in the `pkgs/by-name` overlay, because it's supposed to
// automatically defined attributes in `pkgs/by-name`
NixpkgsProblem::UndefinedAttr {
relative_package_file: relative_package_file.to_owned(),
package_name: attribute_name.to_owned(),
}
.into()
}
.into(),
Existing(NonAttributeSet) => NixpkgsProblem::NonDerivation {
relative_package_file: relative_package_file.to_owned(),
package_name: attribute_name.to_owned(),
// The attribute exists
Existing(AttributeInfo {
// But it's not an attribute set, which limits the amount of information we can get
// about this attribute (see ./eval.nix)
attribute_variant: AttributeVariant::NonAttributeSet,
location: _location,
}) => {
// The only thing we know is that it's definitely not a derivation, since those are
// always attribute sets.
//
// We can't know whether the attribute is automatically or manually defined for sure,
// and while we could check the location, the error seems clear enough as is.
NixpkgsProblem::NonDerivation {
relative_package_file: relative_package_file.to_owned(),
package_name: attribute_name.to_owned(),
}
.into()
}
.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 {
// The attribute exists
Existing(AttributeInfo {
// And it's an attribute set, which allows us to get more information about it
attribute_variant:
AttributeVariant::AttributeSet {
is_derivation,
definition_variant,
},
location,
}) => {
// Only derivations are allowed in `pkgs/by-name`
let is_derivation_result = if is_derivation {
Success(())
} else {
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
// If the definition looks correct
let variant_result = match definition_variant {
// An automatic `callPackage` by the `pkgs/by-name` overlay.
// Though this gets detected by checking whether the internal
// `_internalCallByNamePackageFile` was used
DefinitionVariant::AutoDefinition => {
if let Some(_location) = location {
// Such an automatic definition should definitely not have a location
// Having one indicates that somebody is using `_internalCallByNamePackageFile`,
NixpkgsProblem::InternalCallPackageUsed {
attr_name: attribute_name.to_owned(),
}
.into()
} else {
false
};
Success(Tight)
}
}
// The attribute is manually defined, e.g. in `all-packages.nix`.
// This means we need to enforce it to look like this:
// callPackage ../pkgs/by-name/fo/foo/package.nix { ... }
DefinitionVariant::ManualDefinition {
is_semantic_call_package,
} => {
// We should expect manual definitions to have a location, otherwise we can't
// enforce the expected format
if let Some(location) = location {
// Parse the Nix file in the location and figure out whether it's an
// attribute definition of the form `= callPackage <arg1> <arg2>`,
// returning the arguments if so.
let optional_syntactic_call_package = nix_file_store
.get(&location.file)?
.call_package_argument_info_at(
location.line,
location.column,
// We're passing `pkgs/by-name/fo/foo/package.nix` here, which causes
// the function to verify that `<arg1>` is the same path,
// making `syntactic_call_package.relative_path` end up as `""`
// TODO: This is confusing and should be improved
&absolute_package_file,
)?;
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,
})
// At this point, we completed two different checks for whether it's a
// `callPackage`
match (is_semantic_call_package, optional_syntactic_call_package) {
// Something like `<attr> = { ... }`
// or a `pkgs.callPackage` but with the wrong file
(false, None)
// Something like `<attr> = pythonPackages.callPackage ./pkgs/by-name/...`
| (false, Some(_))
// Something like `<attr> = bar` where `bar = pkgs.callPackage ...`
// or a `callPackage` but with the wrong file
| (true, None) => {
// All of these are not of the expected form, so error out
// TODO: Make error more specific, don't lump everything together
NixpkgsProblem::WrongCallPackage {
relative_package_file: relative_package_file.to_owned(),
package_name: attribute_name.to_owned(),
}.into()
}
// Something like `<attr> = pkgs.callPackage ./pkgs/by-name/...`,
// with the correct file
(true, Some(syntactic_call_package)) => {
Success(
// Manual definitions with empty arguments are not allowed
// anymore
if syntactic_call_package.empty_arg {
Loose(())
} else {
Tight
}
)
}
}
} else {
NixpkgsProblem::WrongCallPackage {
relative_package_file: relative_package_file.to_owned(),
package_name: attribute_name.to_owned(),
// If manual definitions don't have a location, it's likely `mapAttrs`'d
// over, e.g. if it's defined in aliases.nix.
// We can't verify whether its of the expected `callPackage`, so error out
NixpkgsProblem::CannotDetermineAttributeLocation {
attr_name: attribute_name.to_owned(),
}
.into()
}
}
})
};
// Independently report problems about whether it's a derivation and the callPackage variant
is_derivation_result.and(variant_result)
}
}
};
Ok(
// Packages being checked in this function are _always_ already defined in `pkgs/by-name`,
// so instead of repeating ourselves all the time to define `uses_by_name`, just set it
// once at the end with a map
manual_definition_result.map(|manual_definition| ratchet::Package {
manual_definition,
uses_by_name: Tight,
}),
)
}
/// Handles the evaluation result for an attribute _not_ in `pkgs/by-name`,
@ -265,113 +358,117 @@ fn handle_non_by_name_attribute(
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))
}
}
// The ratchet state whether this attribute uses `pkgs/by-name`.
// This is never `Tight`, because we only either:
// - Know that the attribute _could_ be migrated to `pkgs/by-name`, which is `Loose`
// - Or we're unsure, in which case we use NonApplicable
let uses_by_name =
// This is a big ol' match on various properties of the attribute
// First, it needs to succeed evaluation. We can't know whether an attribute could be
// migrated to `pkgs/by-name` if it doesn't evaluate, since we need to check that it's a
// derivation.
//
// This only has the minor negative effect that if a PR that breaks evaluation
// gets merged, fixing those failures won't force anything into `pkgs/by-name`.
//
// 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 failing attributes would have been fit for `pkgs/by-name`
// 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.
if let EvalSuccess(AttributeInfo {
// We're only interested in attributes that are attribute sets (which includes
// derivations). Anything else can't be in `pkgs/by-name`.
attribute_variant: AttributeVariant::AttributeSet {
// Indeed, we only care about derivations, non-derivation attribute sets can't be
// in `pkgs/by-name`
is_derivation: true,
// Of the two definition variants, really only the manual one makes sense here.
// Special cases are:
// - Manual aliases to auto-called packages are not treated as manual definitions,
// due to limitations in the semantic callPackage detection. So those should be
// ignored.
// - Manual definitions using the internal _internalCallByNamePackageFile are
// not treated as manual definitions, since _internalCallByNamePackageFile is
// used to detect automatic ones. We can't distinguish from the above case, so we
// just need to ignore this one too, even if that internal attribute should never
// be called manually.
definition_variant: DefinitionVariant::ManualDefinition { is_semantic_call_package }
},
// We need the location of the manual definition, because otherwise
// we can't figure out whether it's a syntactic callPackage
location: Some(location),
}) = non_by_name_attribute {
// Parse the Nix file in the location and figure out whether it's an
// attribute definition of the form `= callPackage <arg1> <arg2>`,
// returning the arguments if so.
let optional_syntactic_call_package = nix_file_store
.get(&location.file)?
.call_package_argument_info_at(
location.line,
location.column,
// Passing the Nixpkgs path here both checks that the <arg1> is within Nixpkgs, and
// strips the absolute Nixpkgs path from it, such that
// syntactic_call_package.relative_path is relative to Nixpkgs
nixpkgs_path
)?;
// At this point, we completed two different checks for whether it's a
// `callPackage`
match (is_semantic_call_package, optional_syntactic_call_package) {
// Something like `<attr> = { }`
(false, None)
// Something like `<attr> = pythonPackages.callPackage ...`
| (false, Some(_))
// Something like `<attr> = bar` where `bar = pkgs.callPackage ...`
| (true, None) => {
// In all of these cases, it's not possible to migrate the package to `pkgs/by-name`
NonApplicable
}
// Something like `<attr> = pkgs.callPackage ...`
(true, Some(syntactic_call_package)) => {
// It's only possible to migrate such a definitions if..
match syntactic_call_package.relative_path {
Some(ref rel_path) if rel_path.starts_with(utils::BASE_SUBPATH) => {
// ..the path is not already within `pkgs/by-name` like
//
// 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, so migration is generally not possible.
//
// See also "package variants" in RFC 140:
// https://github.com/NixOS/rfcs/blob/master/rfcs/0140-simple-package-paths.md#package-variants
NonApplicable
}
_ => {
// Otherwise, the path is outside `pkgs/by-name`, which means it can be
// migrated
Loose(syntactic_call_package)
}
}
};
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,
})
}
})
} else {
// This catches all the cases not matched by the above `if let`, falling back to not being
// able to migrate such attributes
NonApplicable
};
Ok(Success(ratchet::Package {
// Packages being checked in this function _always_ need a manual definition, because
// they're not using `pkgs/by-name` which would allow avoiding it.
// so instead of repeating ourselves all the time to define `manual_definition`,
// just set it once at the end here
manual_definition: Tight,
uses_by_name,
}))
}

18
pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
View File

@ -92,6 +92,12 @@ pub enum NixpkgsProblem {
call_package_path: Option<PathBuf>,
empty_arg: bool,
},
InternalCallPackageUsed {
attr_name: String,
},
CannotDetermineAttributeLocation {
attr_name: String,
},
}
impl fmt::Display for NixpkgsProblem {
@ -252,6 +258,16 @@ impl fmt::Display for NixpkgsProblem {
structure::relative_file_for_package(package_name).display(),
)
},
}
NixpkgsProblem::InternalCallPackageUsed { attr_name } =>
write!(
f,
"pkgs.{attr_name}: This attribute is defined using `_internalCallByNamePackageFile`, which is an internal function not intended for manual use.",
),
NixpkgsProblem::CannotDetermineAttributeLocation { attr_name } =>
write!(
f,
"pkgs.{attr_name}: Cannot determine the location of this attribute using `builtins.unsafeGetAttrPos`.",
),
}
}
}

1
pkgs/test/nixpkgs-check-by-name/tests/internalCallPackage/expected Normal file
View File

@ -0,0 +1 @@
pkgs.foo: This attribute is defined using `_internalCallByNamePackageFile`, which is an internal function not intended for manual use.

2
pkgs/test/nixpkgs-check-by-name/tests/unknown-location/expected
View File

@ -1 +1 @@
pkgs.foo: This attribute is manually defined (most likely in pkgs/top-level/all-packages.nix), which is only allowed if the definition is of the form `pkgs.callPackage pkgs/by-name/fo/foo/package.nix { ... }` with a non-empty second argument.
pkgs.foo: Cannot determine the location of this attribute using `builtins.unsafeGetAttrPos`.