Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements to pathType, pathIsDirectory and pathIsRegularFile #224834

Merged
merged 8 commits into from
May 23, 2023

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Apr 5, 2023

Description of changes

This is related to the path library effort.

Improves the lib.sources.pathType, lib.sources.pathIsDirectory and lib.sources.pathIsRegularFile functions, each of which is a single commit:

  • Moves them to lib.filesystem, since they only work with the filesystem, they don't import any sources. The functions are still available in lib.sources, but will be deprecated after the next release.
  • Add test cases
  • Minor refactor
  • Fix them for when the filesystem root / is passed. Previously they threw an unintuitive error:
    error: attribute '' missing
      
         at nixpkgs/lib/filesystem.nix:24:20:
      
             23|   */
             24|   pathType = path: (readDir (dirOf path)).${baseNameOf path};
               |                    ^
             25|
    
  • Improve the pathType error message for invalid paths. Previously this was the error:
    error: attribute 'nonexistent' missing
      
         at nixpkgs/lib/filesystem.nix:29:10:
      
             28|     if dirOf path == path then "directory"
             29|     else (readDir (dirOf path)).${baseNameOf path};
               |          ^
             30|
    
  • Improve documentation, adding a better description, a type and examples.
  • Indicate that pathType can get replaced with the new builtins.readFileType once Nix 2.14 is released

This work is sponsored by Antithesis

Things done
  • Wrote docs, built and checked the manual (nix-build doc)
  • Wrote and ran the tests
    • The new script lib/tests/filesystem.sh on its own
    • And the integration into nix-build lib/tests/release.nix

These functions only work with the filesystem, they don't import
anything as sources
@infinisil infinisil requested a review from edolstra as a code owner April 5, 2023 15:33
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Apr 5, 2023
@infinisil
Copy link
Member Author

infinisil commented Apr 5, 2023

Something I don't want to go into for this PR, but maybe in the future: The semantics of pathIsDirectory and pathIsRegularFile are not obvious:

  • They return false when the path doesn't exist, which is often not what you want.
  • They always return false for symlinks without checking where the symlink points (it could point to a directory or a regular file)

We should consider having alternate functions with cleaner semantics in the future:

  • isDirectory :: Path -> Bool: Returns true iff the path is a directory, following symlinks. An error is thrown if the path doesn't exist.
  • isFile :: Path -> Bool: Returns true iff the path is a file, following symlinks. An error is thrown if the path doesn't exist.

These laws would then hold:

  • If isDirectory path succeeds and is true, then builtins.readDir path succeeds
  • If isFile path succeeds and is true, then builtins.readFile path succeeds
  • isDirectory path != isFile path

Unfortunately the implementation of these functions would have to be a bit hacky (throwing for non-existent paths not implemented here):

{
  isDirectory = path: builtins.pathExists "${toString path}/";
  isFile = path: ! builtins.pathExists "${toString path}/";
}

This uses the fact that:

  • toString can turn paths to strings, which would not be the case with lazy trees, and would violate the minimal assumptions of the path library
  • builtins.pathExists works not only on paths, but also on strings, with the important difference that strings aren't normalised, therefore keeping the trailing /

To implement these I'd propose adding more builtins, either:

  • builtins.readSymlink: return the target of a symlink
  • builtins.isDirectory: A builtin version of the above isDirectory

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/tweag-nix-dev-update-47/27387/1

@roberth
Copy link
Member

roberth commented Apr 19, 2023

We should consider having alternate functions with cleaner semantics in the future:

  • isDirectory :: Path -> Bool: Returns true iff the path is a directory, following symlinks. An error is thrown if the path doesn't exist.
  • isFile :: Path -> Bool: Returns true iff the path is a file, following symlinks. An error is thrown if the path doesn't exist.

That's good. How about this?

  • isDirectory :: Path -> Bool: Returns true iff the path is a directory, following symlinks. An error is thrown if the path doesn't exist.
  • existsDirectory :: Path -> Bool: Returns pathExists f && isDirectory f
  • same for *File

existsDirectory is just a rename of pathIsDirectory, but less ambiguous.

=> "regular"
*/
pathType = path:
# TODO: Once Nix 2.14 is released, switch to `builtins.readFileType`:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2.14 was not a success, but 2.15 is out now.
This can be upgraded to a polyfill now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Also take a look at NixOS/nix#7447 (comment)

@infinisil
Copy link
Member Author

@roberth I'd probably go for directoryExists instead of existsDirectory, this then also mimics pathExists

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor suggestions and a less minor idea to improve the developer experience. I'd add the comments myself, but it's better to share the idea.

lib/filesystem.nix Outdated Show resolved Hide resolved
lib/filesystem.nix Outdated Show resolved Hide resolved
lib/filesystem.nix Outdated Show resolved Hide resolved
lib/tests/filesystem.sh Show resolved Hide resolved
lib/filesystem.nix Show resolved Hide resolved
infinisil and others added 7 commits May 22, 2023 14:13
Co-Authored-By: Robert Hensing <robert@roberthensing.nl>
Co-Authored-By: Robert Hensing <robert@roberthensing.nl>
Previously this function couldn't handle / being passed, it would throw
an error:

error: attribute '' missing

       at nixpkgs/lib/filesystem.nix:24:20:

           23|   */
           24|   pathType = path: (readDir (dirOf path)).${baseNameOf path};
             |                    ^
           25|

Consequently this also fixes the
lib.filesystem.{pathIsDirectory,pathIsRegularFile} functions.
Previously it would fail with

  error: attribute 'nonexistent' missing

         at nixpkgs/lib/filesystem.nix:29:10:

             28|     if dirOf path == path then "directory"
             29|     else (readDir (dirOf path)).${baseNameOf path};
               |          ^
             30|
Co-Authored-By: Robert Hensing <robert@roberthensing.nl>
Co-Authored-By: Robert Hensing <robert@roberthensing.nl>
@github-actions
Copy link
Contributor

Successfully created backport PR for release-23.05:

@github-actions
Copy link
Contributor

Git push to origin failed for release-23.05 with exitcode 1

@infinisil infinisil deleted the pathType-and-co branch May 23, 2023 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants