top-level/impure.nix: fix overlay directory check #249165
Conversation
There was a problem hiding this comment.
I would love to get this merged soon. Nix caused the regression but the merged fix from today wouldn't even fix this issue (because it only checks for trailing /): NixOS/nix#8869
And I'd rather not wait around for Nix to unbreak this regression if we can fix it more quickly in Nixpkgs.
|
We can just use |
|
Ah, I assumed that would be circular (probably biased by thinking the original author had a good reason to redefine nixpkgs/pkgs/top-level/default.nix Line 50 in 1c1df7f Edit: I've confirmed locally that using |
|
Actually no we can't do that, because That's the main reason this |
pkgs/top-level/impure.nix
Outdated
| isDir = path: let | ||
| parent = dirOf path; | ||
| base = baseNameOf path; | ||
| type = (builtins.readDir parent).${base} or null; | ||
| in (/. + path) == /. || type == "directory"; |
There was a problem hiding this comment.
| isDir = path: let | |
| parent = dirOf path; | |
| base = baseNameOf path; | |
| type = (builtins.readDir parent).${base} or null; | |
| in (/. + path) == /. || type == "directory"; | |
| isDir = path: builtins.pathExists (path + "/"); |
We can just remove the . for now, it was never necessary in the first place I'm pretty sure.
That seems like a reasonable partial mitigation for now... Though I think we should prefer using Then I'd argue that we should remove most of the checking here. Given that the check is an unreliable hack, there's no real benefit of displaying a message like
vs
(we may still need a form of |
|
It would be great to have a |
nix 2.16 and newer return true for `pathExists (path + "/.")` regardless of whether `path` is a file or directory.
| isDir = path: { | ||
| symlink = builtins.pathExists (toString path + "/"); | ||
| directory = true; | ||
| }.${pathType path} or false; |
There was a problem hiding this comment.
| isDir = path: { | |
| symlink = builtins.pathExists (toString path + "/"); | |
| directory = true; | |
| }.${pathType path} or false; | |
| isDir = path: { | |
| # This is hacky but the only way to check the path type of a symlink target | |
| symlink = builtins.pathExists (toString path + "/"); | |
| directory = true; | |
| }.${pathType path} or false; |
| if isDir homeOverlaysFile then | ||
| throw (homeOverlaysFile + " should be a file") | ||
| else overlays homeOverlaysFile | ||
| overlaysFile homeOverlaysFile | ||
| else if builtins.pathExists homeOverlaysDir then | ||
| if !(isDir homeOverlaysDir) then | ||
| throw (homeOverlaysDir + " should be a directory") | ||
| else overlays homeOverlaysDir | ||
| overlaysDir homeOverlaysDir |
There was a problem hiding this comment.
This leads to a worse error message, the isDir check should be kept because of that.
Nix 2.17.x appears to fail when encountering an overlays.nix file through a symbolic link. This PR may address although I haven't tested: NixOS/nixpkgs#249165 For now, stick to Nix 2.16.x which seems to work.
wegank
added
the
2.status: stale
Description of changes
nix 2.16 and newer return true for
pathExists (path + "/.")regardless of whetherpathis a file or directory, resulting in a new error after updating:This issue likely also affects stable nixpkgs (when evaluated with a newer nix) and may be worth backporting.
This implementation of
isDirwas stolen from filesystem.nixThings 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/)