Fixes for overrideAttrs (split from #201734)#211685
Merged
Artturin merged 5 commits intoNixOS:stagingfrom Feb 3, 2023
Merged
Conversation
(cherry picked from commit 43c8b43)
Member
|
The report looks great! The relevant numbers are all <1%. (And ignore cpuTime as always, although I wish -10% was true) If you could add some tests in |
roberth
reviewed
Jan 20, 2023
Member
There was a problem hiding this comment.
By inlining fix and extends, and then simplifying the expression, you'll improve performance, but the gain will be marginal, as the performance stats indicate that this addition is in fact quite cheap. Readability is more important for now, as you're still working on this code.
5eb8237 to
d3db1cc
Compare
// shouldn't be used when overrideAttrs is available here we can use extends instead of overrideAttrs for performance
only outputs the first failing test atm
d3db1cc to
9c0ac56
Compare
12 tasks
roberth
added a commit
to hercules-ci/nixpkgs
that referenced
this pull request
Feb 27, 2023
The effect of `.out // { outputSpecified = false; }` in these cases
is to select the default output explicitly, but then make the
selection implicit until `overrideAttrs` is called. Previously
`overrideAttrs` would not preserve output selection, masking the
apparently unnecessary behavior of this workaround.
For `libllvm-polly`, this logic does not apply, as it does not
select the default output.
The `outputSpecified` workaround was introduced in
NixOS#122554
and was perhaps rushed because of a release deadline, and expected
delays from mass rebuilds.
The change in `overrideAttrs` behavior was added in
5b2f597 / NixOS#211685
and the problem was discovered in NixOS#218537,
which may contain further information.
12 tasks
alyssais
pushed a commit
that referenced
this pull request
Feb 28, 2023
The effect of `.out // { outputSpecified = false; }` in these cases
is to select the default output explicitly, but then make the
selection implicit until `overrideAttrs` is called. Previously
`overrideAttrs` would not preserve output selection, masking the
apparently unnecessary behavior of this workaround.
For `libllvm-polly`, this logic does not apply, as it does not
select the default output.
The `outputSpecified` workaround was introduced in
#122554
and was perhaps rushed because of a release deadline, and expected
delays from mass rebuilds.
The change in `overrideAttrs` behavior was added in
5b2f597 / #211685
and the problem was discovered in #218537,
which may contain further information.
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
2 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.
Description of changes
Fixes from #201734 that don't come with a significant performance impact.
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes