stdenv.mkDerivation: support output checks with and without structuredAttrs#357054
Merged
Ma27 merged 3 commits intoNixOS:stagingfrom Nov 30, 2024
Merged
Conversation
4 tasks
ofborg
bot
added
the
10.rebuild-darwin-stdenv
ofborg
bot
requested review from
Ma27,
globin,
ivan,
manveru,
rvolosatovs and
thoughtpolice
ofborg
bot
added
10.rebuild-darwin: 501+
nix-owners
bot
requested review from
Artturin,
Ericson2314,
RossComputerGuy,
emilazy,
philiptaron and
reckenrode
wegank
added
12.approvals: 1
This just moves the code around a little bit to be able to build on it in follow up commits without too much of a diff. Adding to removedOrReplacedAttrNames is just a cleanup right now and doesn't change anything functionally, yet. This will be required for later on to avoid having structured outputChecks and those directly on the derivation at the same time.
…hecks This was added for non-structuredAttrs output checks in NixOS#211783. Here we extend the same concept to structuredAttrs-enabled outputChecks, too. The postgresql package worked around this with some conditionals. Those can now be removed - without causing LLVM to be built or substituted.
Once __structuredAttrs are enabled, nix only supports disallowedReferences and friends inside the outputChecks attribute set, defined specifically for each output. Top-level disallowedReferences, as used throughout nixpkgs without structuredAttrs, throws a warning instead: warning: In a derivation named 'perl-5.40.0', 'structuredAttrs' disables the effect of the derivation attribute 'disallowedReferences'; use 'outputChecks.<output>.disallowedReferences' instead To support a seamless migration to enabling structuredAttrs by default, those derivation attributes are now mapped to each output separately when structuredAttrs are enabled. Since both top-level disallowedReferences and outputChecks can be given at the same time, those are now merged together. One package that can be simplified this way is neovim, because all checks should be applied to all outputs anyway.
wolfgangwalther
force-pushed
the
structured-attrs-output-checks
branch
from
c87e982 to
d37f90a
Compare
Contributor
Author
|
Rebased to target staging, because of the number of rebuilds. |
Contributor
Author
|
re rebuilds: IIUC, those are caused by adding empty |
wegank
added
12.approvals: 2
Artturin
reviewed
Dec 1, 2024
Comment on lines
+496
to
+501
| }) // lib.optionalAttrs (!__structuredAttrs) ( | ||
| makeOutputChecks attrs | ||
| ) // lib.optionalAttrs (__structuredAttrs) { | ||
| outputChecks = builtins.listToAttrs (map (name: { | ||
| inherit name; | ||
| value = lib.zipAttrsWith (_: builtins.concatLists) [ |
Member
There was a problem hiding this comment.
The functions should be inherited from lib at the top of the file for improved perf
Member
|
Not great performance-wise |
Contributor
Author
How can I run those performance tests myself?
What else should I be looking at, then? In general: Is there some documentation somewhere on writing performant nix code for such heavily used code paths as |
wolfgangwalther
added a commit
to wolfgangwalther/nixpkgs
that referenced
this pull request
Dec 1, 2024
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
With a recent enough nix, there will be an eval warning when building a derivation with
__structuredAttrs = trueand e.g.disallowedReferences = ... With structuredAttrs, nix expects those split up inoutputChecks.<output>.disallowedReferencesetc. instead.To allow derivations to support both structuredAttrs enabled and disabled during a transition period, this PR maps top-level
disallowedReferencesand friends to each output'soutputChecks. This has the nice side-effect that it is possible to define checks which should apply to all outputs at the top-level, and extend them with output-specific checks as well.While on it, we also apply the same fix that #211783 did to those structured
outputChecks, which had not been the case before.Part of the overall effort to enable structuredAttrs by default eventually, tracking: #205690
Marking as draft for now, because I intend to add some tests as well.It's really annoying to write tests for this, because we effectively need to test anix-buildfailure :/Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.