top-level/impure.nix: fix overlay directory check by arcnmx · Pull Request #249165 · NixOS/nixpkgs

arcnmx · 2023-08-14T17:38:07Z

Description of changes

nix 2.16 and newer return true for pathExists (path + "/.") regardless of whether path is a file or directory, resulting in a new error after updating:

error: /home/arc/.config/nixpkgs/overlays.nix should be a file

This issue likely also affects stable nixpkgs (when evaluated with a newer nix) and may be worth backporting.

This implementation of isDir was stolen from filesystem.nix

Things done

solson

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.

infinisil · 2023-09-01T14:51:58Z

We can just use lib.filesystem.isDirectory instead. It won't be a problem to import lib in this file

solson · 2023-09-01T14:58:13Z

Ah, I assumed that would be circular (probably biased by thinking the original author had a good reason to redefine isDir inline), but I guess it will work just like this one in default.nix?

nixpkgs/pkgs/top-level/default.nix

Line 50 in 1c1df7f

lib = import ../../lib;

Edit: I've confirmed locally that using isDir = (import ../../lib).filesystem.pathIsDirectory; (one-line diff) works and fixes my issues with overlays.

infinisil · 2023-09-01T15:35:00Z

Actually no we can't do that, because lib.filesystem.pathIsDirectory always treats symlinks as non-directories, even if they link to a directory.

That's the main reason this pathExists (toString ./path + "/") hack is necessary.

infinisil · 2023-09-01T15:36:04Z

pkgs/top-level/impure.nix

Suggested change

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.

arcnmx · 2023-09-01T16:14:34Z

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 lib.filesystem.pathType path anyway and only fall back to using the hack when it == "symlink".

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

error: ~/.config/nixpkgs/overlays.nix should be a file

vs

error: opening directory '~/.config/nixpkgs/overlays.nix': Not a directory

(we may still need a form of isDir to handle <nixpkgs-overlays> properly but if we can avoid using it for homeOverlaysFile and homeOverlaysDir then that eliminates most of our problems)

infinisil · 2023-09-01T16:18:45Z

It would be great to have a lib.filesystem.isDirectory as described in #224834 (comment)

nix 2.16 and newer return true for `pathExists (path + "/.")` regardless of whether `path` is a file or directory.

infinisil · 2023-10-16T18:10:21Z

pkgs/top-level/impure.nix

+      isDir = path: {
+        symlink = builtins.pathExists (toString path + "/");
+        directory = true;
+      }.${pathType path} or false;


Suggested change

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;

infinisil · 2023-10-16T18:12:19Z

pkgs/top-level/impure.nix

-        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


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.

mjhoy · 2024-09-18T10:32:30Z

Wondering if this has since been fixed? I just installed nix 2.18.5, and have a symlink at ~/.config/nixpkgs/overlays.nix, and everything appears to work.

arcnmx requested a review from Ericson2314 as a code owner August 14, 2023 17:38

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Aug 14, 2023

This was referenced Sep 1, 2023

Fix #8838, pathExists: isDir when ends with / NixOS/nix#8869

Merged

Regression in builtins.pathExists NixOS/nix#8838

Closed

solson approved these changes Sep 1, 2023

View reviewed changes

infinisil requested changes Sep 1, 2023

View reviewed changes

top-level/impure.nix: fix overlay directory check

eace1c0

nix 2.16 and newer return true for `pathExists (path + "/.")` regardless of whether `path` is a file or directory.

arcnmx force-pushed the nix216-isdir branch from b80c0f8 to eace1c0 Compare September 1, 2023 16:46

solson mentioned this pull request Sep 3, 2023

2.16 regression: invoking nix-shell -p: overlays.nix should be a file NixOS/nix#8443

Open

infinisil requested changes Oct 16, 2023

View reviewed changes

singron mentioned this pull request Dec 10, 2023

builtins.pathExists returns false for symlink to directory suffixed with "/." NixOS/nix#9585

Open

wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 19, 2024

FliegendeWurst added the awaiting_changes label Dec 4, 2024

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 4, 2024

wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 2, 2025

SigmaSquadron removed the awaiting_changes label Jan 5, 2025

wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 2, 2025

nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Jun 25, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

top-level/impure.nix: fix overlay directory check#249165

top-level/impure.nix: fix overlay directory check#249165
arcnmx wants to merge 1 commit intoNixOS:masterfrom
arcnmx:nix216-isdir

arcnmx commented Aug 14, 2023 •

edited

Loading

Uh oh!

solson left a comment •

edited

Loading

Uh oh!

infinisil commented Sep 1, 2023

Uh oh!

solson commented Sep 1, 2023 •

edited

Loading

Uh oh!

infinisil commented Sep 1, 2023

Uh oh!

infinisil Sep 1, 2023 •

edited

Loading

Uh oh!

arcnmx commented Sep 1, 2023 •

edited

Loading

Uh oh!

infinisil commented Sep 1, 2023

Uh oh!

infinisil Oct 16, 2023

Uh oh!

infinisil Oct 16, 2023

Uh oh!

mjhoy commented Sep 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

-      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 + "/");

Uh oh!

Comments

Conversation

arcnmx commented Aug 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Things done

Uh oh!

solson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

infinisil commented Sep 1, 2023

Uh oh!

solson commented Sep 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

infinisil commented Sep 1, 2023

Uh oh!

infinisil Sep 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arcnmx commented Sep 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

infinisil commented Sep 1, 2023

Uh oh!

infinisil Oct 16, 2023

Choose a reason for hiding this comment

Uh oh!

infinisil Oct 16, 2023

Choose a reason for hiding this comment

Uh oh!

mjhoy commented Sep 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

arcnmx commented Aug 14, 2023 •

edited

Loading

solson left a comment •

edited

Loading

solson commented Sep 1, 2023 •

edited

Loading

infinisil Sep 1, 2023 •

edited

Loading

arcnmx commented Sep 1, 2023 •

edited

Loading