by-name-overlay: don't include empty directories #340778
Conversation
ofborg
bot
added
10.rebuild-darwin: 0
| (readDir (baseDirectory + "/${shard}")); | ||
| lib.filterAttrs (_: path: builtins.pathExists path) ( | ||
| mapAttrs (name: _: baseDirectory + "/${shard}/${name}/package.nix") ( | ||
| readDir (baseDirectory + "/${shard}") |
There was a problem hiding this comment.
Perhaps this is a use case for lib.pipe?
|
Hmm, this is a tricky one, because it increases evaluation time for every Nixpkgs evaluation. Not by much, but measurably:
This is by running The fix for the issue is nice, but it's a tricky call as to whether it's worth it for the evaluation time increase. I'm on the edge! A future alternative would be to have a |
On one hand, I think (but haven't tested) that this shouldn't be an issue with nix3 commands, on the other, I feel like nixpkgs already has made compromises to support pre nix3 commands... Like personally, I lean not making nixpkgs slower here, or perhaps just refactoring this? Alternatively, maybe it's possible to make a fix that instead of doing file IO on all packages in by-name runs some EDIT: Alternatively, if there is a way to only check folders that aren't empty (i.e. are visible to git) maybe there is a more elegant solution in that directon? |
|
We probably at least should find a way to warn people affected by #338227, because an empty folder is invisible to e.g. |
fixes: NixOS#338227 Signed-off-by: Christina Sørensen <christina@cafkafk.com>
cafkafk
force-pushed
the
by-name-empty-dir-bug
branch
from
77c69d4
to
c6aa1f7
Compare
Yah, expected that a fix would hit performance...
Agree, maybe a warning would be enough, especially as more and more people are moving towards nix3, where this isn't an issue. |
|
I propose a more targeted fix for |
|
I assume this is superseded by #341041? |
Essentially, although it would likely make sense to create a more generic fix if possible (without performance regressions) |
Agreed. This is one of the places where Nix builtins could be highly leveraged, since they're able to make these tests much cheaper in aggregate than is possible in pure Nix. It would take a high degree of coordination across deployed Nix runtimes (Lix, Nix, maybe Tvix) to do that. |
Description of changes
Currently, by-name-overlay does not check if the directories are empty. This is
specially problematic because it doesn't matter that git isn't tracking them
when not using nix3 commands.
This adds a check to ensure that any package in a shard that gets included is
actually a package.
I'd love some review on this. I'm not super familiar with nixpkgs internals.
Fixes: #338227
Signed-off-by: Christina Sørensen christina@cafkafk.com
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.