Fixes for overrideAttrs (split from #201734) #211685
Merged
Conversation
This file contains 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
(cherry picked from commit 43c8b43)
roberth
changed the title
|
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
| @@ -16,17 +16,22 @@ let | |||
| # This function introduces `overridePythonAttrs` and it overrides the call to `buildPythonPackage`. | |||
| makeOverridablePythonPackage = f: origArgs: | |||
| let | |||
| ff = f origArgs; | |||
| overrideWith = newArgs: origArgs // (if pkgs.lib.isFunction newArgs then newArgs origArgs else newArgs); | |||
| args = lib.fix (lib.extends | |||
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.
Artturin
force-pushed
the
splicingstuff1-split
branch
3 times, most recently
from
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
Artturin
force-pushed
the
splicingstuff1-split
branch
from
d3db1cc
to
9c0ac56
Compare
Artturin
requested review from
FRidh,
Ericson2314,
matthewbauer,
edolstra,
nbp and
infinisil
as code owners
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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