From 1c3eb9eff1b864bf49c3661558b495235fc3b3b4 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Tue, 14 Nov 2023 07:42:59 +0100 Subject: [PATCH] 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 --- lib/fileset/README.md | 15 +++++++++++++++ lib/fileset/default.nix | 20 +++++++++++++++----- lib/fileset/internal.nix | 33 ++++++++++++++++++--------------- lib/fileset/tests.sh | 15 ++++----------- 4 files changed, 52 insertions(+), 31 deletions(-) diff --git a/lib/fileset/README.md b/lib/fileset/README.md index 91f892a1be95..14b6877a9065 100644 --- a/lib/fileset/README.md +++ b/lib/fileset/README.md @@ -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 = ; }` 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: diff --git a/lib/fileset/default.nix b/lib/fileset/default.nix index d90c770633d8..81be1af9d8a1 100644 --- a/lib/fileset/default.nix +++ b/lib/fileset/default.nix @@ -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. diff --git a/lib/fileset/internal.nix b/lib/fileset/internal.nix index b245caade1f5..6b5cea066afd 100644 --- a/lib/fileset/internal.nix +++ b/lib/fileset/internal.nix @@ -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; + }; } diff --git a/lib/fileset/tests.sh b/lib/fileset/tests.sh index 5ef155d25a88..ebf9b6c37bf2 100755 --- a/lib/fileset/tests.sh +++ b/lib/fileset/tests.sh @@ -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