Skip to content

stdenv/check-meta: deep type checks#204840

Merged
ncfavier merged 3 commits intoNixOS:masterfrom
ncfavier:check-meta-deep
Jan 1, 2023
Merged

stdenv/check-meta: deep type checks#204840
ncfavier merged 3 commits intoNixOS:masterfrom
ncfavier:check-meta-deep

Conversation

@ncfavier
Copy link
Member

@ncfavier ncfavier commented Dec 6, 2022

Check meta attributes deeply: meta.maintainers = [ 42 ]; now results in an evaluation error if checkMeta is enabled. Adds a lib.typeCheck function that performs deep type checks.

Prior context (optional): #6794 (comment), #191171, NixOS/nixos-search#391

Note: this does NOT set checkMeta to true by default, it only changes what happens when it is.

I used the following command (extracted from OfBorg) to run the checks:

nix-env --json -f '<nixpkgs>' -I nixpkgs=. --arg config '{
  checkMeta = true;
  handleEvalIssue = reason: msg:
    if builtins.elem reason ["unknown-meta" "broken-outputs"] then abort msg else true;
}' -qa --out-path --meta > /dev/null

EDIT: this is not enough as it does not recurse into attrsets, I'd have to use OfBorg's outpaths.nix but my computer doesn't have enough memory...

It would be nice to abstract this into a script somewhere in nixpkgs, so that people can run it locally without needing OfBorg (and OfBorg can then just call it). Where should it go? maintainers/scripts? nixpkgs-basic-release-checks.nix?

I've left a few TODOs in the code, they don't have to be addressed in this PR (but they could).

Ping everyone remotely involved, feel free to ignore if you're not interested: @piegamesde @Profpatsch @sternenseemann @roberth @tazjin @SuperSandro2000 @7c6f434c @mweinelt @amjoseph-nixpkgs @K900

@github-actions github-actions bot added the 8.has: documentation This PR adds or changes documentation label Dec 7, 2022
@ofborg ofborg bot added 8.has: clean-up This PR removes packages or removes other cruft 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Dec 7, 2022
lib/modules.nix Outdated
Comment on lines 779 to 782
Copy link
Member

Choose a reason for hiding this comment

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

The names of types describe the output type, and it would be reasonable to expect this to check what the name of the type suggests.
However, as you know, this checks module system definition inputs, and not type outputs. This is not an acceptable implementation; at least not for lib.
If you don't want to implement an output checking method on the types, you may choose to move these functions to check-meta, with an explanatory comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not entirely sure what you mean by type inputs and outputs. Is this about my earlier comment about mkIf? I don't really see a solution to that that doesn't involve a deep refactoring of the module system.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, mkIf is one of the differences between type inputs and outputs.
They're not really types but functions, the way they're part of the module system. We try to write idempotent types, but that's not a guarantee.

deep refactoring

A simple change would be to add a new method to the types; to consider output checking as a completely new use case for types that's distinct from types' use in the module system.
I don't think the module system's merging and checking should be decoupled, because it's better to check the inputs, for better error messages, and it's better not to check the output and rely on a correct merge implementation, because otherwise, we're checking things twice (or perhaps even quadratically), which is slow for no real benefit.

Copy link
Member Author

Choose a reason for hiding this comment

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

So you're suggesting something like checkOutput = v: isList v && all elemType.checkOutput v, for listOf elemType? That seems like a lot of code duplication, but I guess we could at least make checkOutput default to check if not given.

In any case I'll probably just move the functions out of lib for this PR.

Copy link
Member

Choose a reason for hiding this comment

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

I guess you could call that duplication, yes. They're similar code paths to achieve similar goals, but there's no opportunity for code reuse a the type interface level, and any reuse achieved by a mkOptionType helper function would be disproportionally slow, considering how widely this code is going to be called.
So yeah, the implementation is not pretty, but what matters most is how it behaves, and the interface towards users of types and the module system.

In any case I'll probably just move the functions out of lib for this PR.

sgtm

something like checkOutput = v: isList v && all elemType.checkOutput v,

Maybe. This does not allow lazy checking, or nice error messages that can be listed, like the assertions and warnings in NixOS. Can be discussed later.

The new derivation should evaluate only if the old derivation does.

Sadly this means that the old derivation cannot depend on the new one
any more, which was used by xorgserver on Darwin. But this is not a
problem as `overrideAttrs` can (and should) usually be used instead.

This change allowed catching an invalid `meta.platforms` in the linux_rpi
kernels, which use `overrideDerivation`.
Use a wrapper around `mergeDefinitions` to type-check values deeply, so
that e.g. `maintainers = [ 42 ];` is an error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: kernel The Linux kernel 6.topic: stdenv Standard environment 8.has: clean-up This PR removes packages or removes other cruft 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants