Commit 1c3eb9ef authored by Silvan Mosberger's avatar Silvan Mosberger Committed by Silvan Mosberger
Browse files

lib.fileset.fileFilter: Restrict second argument to paths 

While this change is backwards-incompatible, I think it's okay because:
- The `fileFilter` function is not yet in a stable NixOS release, it was only merged about [a month ago](https://github.com/NixOS/nixpkgs/pull/257356).
  - All public uses of the function on GitHub only pass a path
- Any `fileFilter pred fileset` can also be expressed as `intersection fileset (fileFilter pred path)` without loss of functionality.
  - This is furthermore pointed out in the new error message when a file set is passed
parent b5953ae4

lib/fileset/README.md

+15 −0
Original line number Diff line number Diff line
@@ -238,6 +238,21 @@ Arguments: 
  And it would be unclear how the library should behave if the one file wouldn't be added to the store:

  `toSource { root = ./file.nix; fileset = <empty>; }` has no reasonable result because returing an empty store path wouldn't match the file type, and there's no way to have an empty file store path, whatever that would mean.



### `fileFilter` takes a path



The `fileFilter` function takes a path, and not a file set, as its second argument.



- (-) Makes it harder to compose functions, since the file set type, the return value, can't be passed to the function itself like `fileFilter predicate fileset`

  - (+) It's still possible to use `intersection` to filter on file sets: `intersection fileset (fileFilter predicate ./.)`

    - (-) This does need an extra `./.` argument that's not obvious

      - (+) This could always be `/.` or the project directory, `intersection` will make it lazy

- (+) In the future this will allow `fileFilter` to support a predicate property like `subpath` and/or `components` in a reproducible way.

  This wouldn't be possible if it took a file set, because file sets don't have a predictable absolute path.

  - (-) What about the base path?

    - (+) That can change depending on which files are included, so if it's used for `fileFilter`

      it would change the `subpath`/`components` value depending on which files are included.

- (+) If necessary, this restriction can be relaxed later, the opposite wouldn't be possible



## To update in the future



Here's a list of places in the library that need to be updated in the future:

lib/fileset/default.nix

+15 −5
Original line number Diff line number Diff line
@@ -366,7 +366,7 @@ in { 
          type :: String,

          ...

        } -> Bool)

        -> FileSet

        -> Path

        -> FileSet



    Example:
@@ -397,14 +397,24 @@ in { 
      Other attributes may be added in the future.

    */

    predicate:

    # The file set to filter based on the predicate function

    fileset:

    # The path whose files to filter

    path:

    if ! isFunction predicate then

      throw ''

        lib.fileset.fileFilter: First argument is of type ${typeOf predicate}, but it should be a function instead.''

    else if ! isPath path then

      if path._type or "" == "fileset" then

        throw ''

          lib.fileset.fileFilter: Second argument is a file set, but it should be a path instead.

              If you need to filter files in a file set, use `intersection fileset (fileFilter pred ./.)` instead.''

      else

        throw ''

          lib.fileset.fileFilter: Second argument is of type ${typeOf path}, but it should be a path instead.''

    else if ! pathExists path then

      throw ''

        lib.fileset.fileFilter: Second argument (${toString path}) is a path that does not exist.''

    else

      _fileFilter predicate

        (_coerce "lib.fileset.fileFilter: Second argument" fileset);

      _fileFilter predicate path;



  /*

    The file set containing all files that are in both of two given file sets.

lib/fileset/internal.nix

+18 −15
Original line number Diff line number Diff line
@@ -786,9 +786,9 @@ rec { 
        _differenceTree (path + "/${name}") lhsValue (rhs.${name} or null)

      ) (_directoryEntries path lhs);



  # Filters all files in a file set based on a predicate

  # Type: ({ name, type, ... } -> Bool) -> FileSet -> FileSet

  _fileFilter = predicate: fileset:

  # Filters all files in a path based on a predicate

  # Type: ({ name, type, ... } -> Bool) -> Path -> FileSet

  _fileFilter = predicate: root:

    let

      # Check the predicate for a single file

      # Type: String -> String -> filesetTree
@@ -807,19 +807,22 @@ rec { 


      # Check the predicate for all files in a directory

      # Type: Path -> filesetTree

      fromDir = path: tree:

        mapAttrs (name: subtree:

          if isAttrs subtree || subtree == "directory" then

            fromDir (path + "/${name}") subtree

          else if subtree == null then

            null

      fromDir = path:

        mapAttrs (name: type:

          if type == "directory" then

            fromDir (path + "/${name}")

          else

            fromFile name subtree

        ) (_directoryEntries path tree);

            fromFile name type

        ) (readDir path);



      rootType = pathType root;

    in

    if fileset._internalIsEmptyWithoutBase then

      _emptyWithoutBase

    if rootType == "directory" then

      _create root (fromDir root)

    else

      _create fileset._internalBase

        (fromDir fileset._internalBase fileset._internalTree);

      # Single files are turned into a directory containing that file or nothing.

      _create (dirOf root) {

        ${baseNameOf root} =

          fromFile (baseNameOf root) rootType;

      };

}

lib/fileset/tests.sh

+4 −11
Original line number Diff line number Diff line
@@ -813,14 +813,15 @@ checkFileset 'difference ./. ./b' 
# The first argument needs to be a function

expectFailure 'fileFilter null (abort "this is not needed")' 'lib.fileset.fileFilter: First argument is of type null, but it should be a function instead.'



# The second argument can be a file set or an existing path

expectFailure 'fileFilter (file: abort "this is not needed") null' 'lib.fileset.fileFilter: Second argument is of type null, but it should be a file set or a path instead.'

# The second argument needs to be an existing path

expectFailure 'fileFilter (file: abort "this is not needed") _emptyWithoutBase' 'lib.fileset.fileFilter: Second argument is a file set, but it should be a path instead.

\s*If you need to filter files in a file set, use `intersection fileset \(fileFilter pred \./\.\)` instead.'

expectFailure 'fileFilter (file: abort "this is not needed") null' 'lib.fileset.fileFilter: Second argument is of type null, but it should be a path instead.'

expectFailure 'fileFilter (file: abort "this is not needed") ./a' 'lib.fileset.fileFilter: Second argument \('"$work"'/a\) is a path that does not exist.'



# The predicate is not called when there's no files

tree=()

checkFileset 'fileFilter (file: abort "this is not needed") ./.'

checkFileset 'fileFilter (file: abort "this is not needed") _emptyWithoutBase'



# The predicate must be able to handle extra attributes

touch a
@@ -882,14 +883,6 @@ checkFileset 'union ./c/a (fileFilter (file: assert file.name != "a"; true) ./.) 
# but here we need to use ./c

checkFileset 'union (fileFilter (file: assert file.name != "a"; true) ./.) ./c'



# Also lazy, the filter isn't called on a filtered out path

tree=(

    [a]=1

    [b]=0

    [c]=0

)

checkFileset 'fileFilter (file: assert file.name != "c"; file.name == "a") (difference ./. ./c)'



# Make sure single files are filtered correctly

tree=(

    [a]=1