Fix #8838, pathExists: isDir when ends with /

[roberth]

Motivation

Fix a regression.

Context

• Appears to have been caused by Backport SourcePath from the lazy-trees branch #8172
• Fixes Regression in builtins.pathExists #8838

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

• agreed on idea
• agreed on implementation strategy
• tests, as appropriate
  • functional tests - tests/**.sh
  • unit tests - src/*/tests
  • integration tests - tests/nixos/*
• documentation in the manual
• documentation in the internal API docs
• code and comments are self-explanatory
• commit message explains why the change was made
• new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

[edolstra]

This seems very hacky since it only works for strings.

My preference would be to not "fix" this unless it causes a lot of issues in real code, since this is really something that only happened to work by accident (i.e. because we didn't canonicalize paths early enough). IMHO path values should be canonical, and /foo/ canonicalizes to /foo, so pathExists /foo/ == pathExist /foo. And using pathExists to determine file type seems iffy.

Alternatively, path values could carry an extra bit to store that they have a trailing slash.

[roberth]

very hacky

Just UNIX heritage in my view.

path values could carry an extra bit

I like the simple and to the point data model of path values. By adding the extra bit, this form of needless entropy will propagate through our expressions; an extra thing we have to keep in mind when reading code.

Either suggestion would be a breaking change to the language, so I propose to just fix the regression and move on to something more useful.

[infinisil]

@edolstra It's an accidental regression, so it should be fixed, it's as simple as that. If Nix starts randomly changing behaviour any time it seems convenient then we cannot trust it's purity anymore. If the test suite was better this should've never happened in the first place.

[yorickvP]

Hitting this on master when I try to use nixpkgs (because my overlays.nix is a symlink)

❯ nix repl --show-trace
Welcome to Nix 2.17.0. Type :? for help.

nix-repl> import /home/yorick/nixpkgs {}
error:
       … from call site

         at «string»:1:1:

            1| import /home/yorick/nixpkgs {}
             | ^

       … while calling anonymous lambda

         at /home/yorick/nixpkgs/pkgs/top-level/impure.nix:14:1:

           13|
           14| { # We put legacy `system` into `localSystem`, if `localSystem` was not passed.
             | ^
           15|   # If neither is passed, assume we are building packages on the current

       … from call site

         at /home/yorick/nixpkgs/pkgs/top-level/impure.nix:87:1:

           86|
           87| import ./. (builtins.removeAttrs args [ "system" ] // {
             | ^
           88|   inherit config overlays localSystem;

       … while calling anonymous lambda

         at /home/yorick/nixpkgs/pkgs/top-level/default.nix:19:1:

           18|
           19| { # The system packages will be built on. See the manual for the
             | ^
           20|   # subtle division of labor between these two `*System`s and the three

       … from call site

         at /home/yorick/nixpkgs/pkgs/top-level/default.nix:55:5:

           54|   checked =
           55|     throwIfNot (lib.isList overlays) "The overlays argument to nixpkgs must be a list."
             |     ^
           56|     lib.foldr (x: throwIfNot (lib.isFunction x) "All overlays passed to nixpkgs must be functions.") (r: r) overlays

       … while calling 'throwIfNot'

         at /home/yorick/nixpkgs/lib/trivial.nix:386:22:

          385|   */
          386|   throwIfNot = cond: msg: if cond then x: x else throw msg;
             |                      ^
          387|

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

nix-repl>

[edolstra]

A problem with this fix is that it's kind of ad hoc in that it changes the behaviour of paths for pathExists only. (E.g. should builtins.readFile "/etc/passwd/" succeed?) That may be fine as a backward compatibility hack, but then we should probably add a comment to the source noting that this is special behaviour to unbreak Nixpkgs overlays.

[Ericson2314]

I agree the first order of business is fixing unintentional regressions. We may also decide we want to change the semantics, but then there should be a graceful deprecation, like #8738 proposes (for something else). Bugs shouldn't allow us to fast-track deprecations.

[roberth]

pathExists only. (E.g. should builtins.readFile "/etc/passwd/" succeed?)

That's a good point. We may have more regressions, although the change in readFile behavior is less troublesome. However, we should restore the old behavior so that users don't start to rely on such a quirk.

noting that this is special behaviour to unbreak Nixpkgs overlays.

We don't know the scope of the effect of the regression, and I don't like the suggestion that Nix exists for the purpose of Nixpkgs. Nixpkgs is important, but Nix should be a reliable tool on its own.

[edolstra]

We don't know the scope of the effect of the regression, and I don't like the suggestion that Nix exists for the purpose of Nixpkgs. Nixpkgs is important, but Nix should be a reliable tool on its own.

Absolutely, but with enough users, there will always be somebody depending on buggy or undocumented behaviour. (Obligatory xkcd.) 100% backwards compatibility is not a realistic goal. So the main consideration is how many people are affected by a change in behaviour.

[github-actions]

Successfully created backport PR for 2.16-maintenance:

• [Backport 2.16-maintenance] Fix #8838, pathExists: isDir when ends with /  #8896

[github-actions]

Successfully created backport PR for 2.17-maintenance:

• [Backport 2.17-maintenance] Fix #8838, pathExists: isDir when ends with /  #8897

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-09-01-nix-team-meeting-minutes-84/32466/1

[solson]

Despite discussion above about making this change to unbreak Nixpkgs overlays, they are still broken after this PR because of this line: https://github.com/NixOS/nixpkgs/blob/9ace7896297b51fee5ad34c1977bec1e6541c089/pkgs/top-level/impure.nix#L40

Note that it uses "/." rather than a trailing slash.

At this point it's probably better to focus on NixOS/nixpkgs#249165, but I thought it worth commenting here because Nix is the cause of the regression, not Nixpkgs.