`lib.fileset.maybeMissing`: init #265964

infinisil · 2023-11-07T00:06:45Z

Description of changes

Another split-off from #222981, motivated by @alyssais comment describing a use case. This PR adds a new function to lib.fileset:

lib.fileset.maybeMissing :: Path -> FileSet: Create a file set from a path that may or may not exist.
```
lib.fileset.maybeMissing ./generated.o
```

An alternative is #267091, but we decided against that

This work is sponsored by Antithesis ✨

Things done

Tests
Documentation
Update related documentation

roberth · 2023-11-07T16:26:18Z

The name suggests a parallel with lib.optional : Bool -> a -> List a, but there isn't one.
How about unchecked instead?

OT: I would quite like to have fileset.optional : Bool -> FileSet -> FileSet such that optional false x == fileset.empty. Example:

mkDerivation(finalAttrs: {
  src = fileset.unions [
    ./src
    (fileset.optional finalAttrs.doCheck ./tests)
  ];
})

infinisil · 2023-11-07T16:51:18Z

The name suggests a parallel with lib.optional : Bool -> a -> List a, but there isn't one.
How about unchecked instead?

Hmm good point, not a fan of unchecked though, makes me ask the question what property is not being checked, and whether this is an unsafe operation.

Some other ideas:

lib.fileset.optionalIfExists ./generated.o -> Meh, it's not optional if it does exist, then it's definitely included
lib.fileset.ifExists ./generated.o -> If it exists then what? Does this return a bool?
lib.fileset.try ./generated.o -> Doesn't say what is being tried
lib.fileset.tryCoerce ./generated.o -> Does make sense, but maybe a bit too odd of a word
lib.fileset.tryFrom ./generated.o -> Not bad imo, nicer word than coerce
lib.fileset.maybe ./generated.o -> Not bad imo, though it doesn't match Haskell's maybe
lib.fileset.maybeMissing ./generated.o -> A bit more explicit, but a bit long

I think I like lib.fileset.tryFrom the most, which could also lead to a lib.fileset.from that does coercion but fails for missing files in case that's ever needed.

OT: I would quite like to have fileset.optional : Bool -> FileSet -> FileSet such that optional false x == fileset.empty.

Oh that sounds nice, should be fairly easy to add too.

alyssais · 2023-11-07T17:23:48Z

OT: I would quite like to have fileset.optional : Bool -> FileSet -> FileSet such that optional false x == fileset.empty

Is this actually required? You can already use lib.optional(s) and have the result coerced to a fileset.

infinisil · 2023-11-07T17:34:19Z

@roberth @alyssais I migrated (and replied to) our discussion on lib.fileset.optional :: Bool -> FileSet -> FileSet to a new issue: #266106

roberth · 2023-11-07T19:48:09Z

lib.fileset.maybe ./generated.o -> Not bad imo, though it doesn't match Haskell's maybe

We don't have Haskell's Maybe in the first place. nullOr might feel similar but is very very different (in the context of a standard library).
maybe is kinda bad, for reasons that point-free is bad.

So I wouldn't be too concerned about this name in general, but I don't think it's specific enough.

lib.fileset.tryFrom ./generated.o

Still doesn't say what's tried.

My favorite from your list is maybeMissing.

What about whenExists?

infinisil · 2023-11-07T23:14:45Z

What about whenExists?

Also doesn't indicate what happens when it exists.

I'll just go for maybeMissing (though should it be mayBeMissing instead?), seems like the least evil.

roberth · 2023-11-07T23:52:43Z

What about whenExists?

Also doesn't indicate what happens when it exists.

I'm used to Haskell's when which "does" nothing when false. It could be read as "only when exists", with the implied alternative of nothing (empty).

mayBeMissing

Correct, but I predict 50% will mistype.

fricklerhandwerk · 2023-11-08T00:40:45Z

I'm worried the interface is growing faster than adoption of the library warrants. This issue could be stop-gapped with an example in the documentation, because the use case can be implemented with what's already there.

If it turns out to be a heavily used pattern one can still add a convenience layer.

Right now I'd prefer efforts being directed to making good use of existing facilities, evolving some usage patterns, and adding more practical examples to the docs.

infinisil · 2023-11-08T21:08:08Z

This particular use case I also encountered while working on the original WIP, and @alyssais being one of the first users and immediately running into it too gives me the feeling we should support this. The alternative without this function is too verbose:

unions (lib.optional (builtins.pathExists ./generated.o) ./generated.o))

# With this PR:
maybeMissing ./generated.o

Even just the fact that the path needs to be repeated will make anybody needing this want to write a function for it! I think lib.fileset should be a batteries-included library that just lets you do what you need to do, without having to create extra functions or write boilerplate.

alyssais · 2023-11-10T19:36:24Z

FWIW: all I'm trying to do is replace this very simple use of cleanSource(With), and then add some additional filtering on top for individual components. I'd consider this a fairly basic use case.

alyssais · 2023-11-10T19:40:16Z

@infinisil how would you feel about having lib.fileset.difference implicitly handle missing files in its second argument? It's used for filtering out files you don't want, so I'm not sure when you'd ever care that they didn't exist, and otherwise IIUC you need to lib.fileset.difference set (lib.unions (map lib.fileset.maybeMissing [ … ])), which is pretty boilerplate-y. I'd understand if it felt like too much magic to you, though.

infinisil · 2023-11-11T22:14:49Z

Hmm that doesn't always make sense though, because you can also do difference ./. (difference ./foo ./foo/bar), where ./foo/bar would be included in the end and so has to exist.

Maybe the check for files existing could be pushed further down generally though, I'll look into that.

roberth · 2023-11-11T22:51:39Z

Right! That reminds me of the function type operator, which also have a binary property that flips in one of the operands, and flips back when the operator is nested: covariance/contravariance aka positive/negative.

Seems to reinforce the idea that you can negate a contextual boolean (ie argument to some internal function) whenever you process a difference rhs.

infinisil · 2023-11-12T19:33:17Z

Unfortunately the difference ./. (difference ./foo ./foo/bar) case doesn't work out with the current internal representation, but otherwise it's totally doable, check out #267091!

infinisil · 2023-11-16T00:02:38Z

Since we decided against doing #267091, we should have maybeMissing after all.

I rebased on top of master, this would be ready from my side.

infinisil · 2023-11-19T00:38:01Z

Rebased again, still good from my side :)

roberth · 2023-11-20T00:05:24Z

lib/fileset/default.nix

+    if ! isPath path then
+      if isStringLike path then
+        throw ''
+          lib.fileset.maybeMissing: Argument ("${toString path}") is a string-like value, but it should be a path instead.''


Suggested change

lib.fileset.maybeMissing: Argument ("${toString path}") is a string-like value, but it should be a path instead.''

lib.fileset.maybeMissing: Argument ("${toString path}") is a string-like value, but it should be a path instead. See https://nixos.org/manual/nix/stable/language/values#type-path''

I don't think the word "path" is enough for novices.

Done in 701cff3!

roberth · 2023-11-20T00:06:17Z

lib/fileset/default.nix

+          lib.fileset.maybeMissing: Argument ("${toString path}") is a string-like value, but it should be a path instead.''
+      else
+        throw ''
+          lib.fileset.maybeMissing: Argument is of type ${typeOf path}, but it should be a path instead.''


Same here.
Might be helpful to have a function that generates an explanation or something. Or just a let binding with the url?

Just for the sake of consistency I'd prefer just copying it to all the error messages for now. We could consider having a dedicated lib/fileset/errors.nix in the future to collect and abstract over all error messages together.

roberth · 2023-11-20T00:07:19Z

lib/fileset/README.md

Could you document the rationale for maybeMissing here, as opposed to non-strict by default?

Done in 4594aa0!

infinisil · 2023-11-21T03:07:00Z

Ready to be merged again from my side

fricklerhandwerk

The implementation is good, but I suggest cutting down on the other diff noise.

fricklerhandwerk · 2023-11-22T07:47:24Z

lib/fileset/default.nix

+          lib.fileset.maybeMissing: Argument ("${toString path}") is a string-like value, but it should be a path instead, see https://nixos.org/manual/nix/stable/language/values#type-path.''
+      else
+        throw ''
+          lib.fileset.maybeMissing: Argument is of type ${typeOf path}, but it should be a path instead, see https://nixos.org/manual/nix/stable/language/values#type-path.''


I don't think we need links to the manual. It's spammy and doesn't exactly make things easier with maintaining that web stuff in the long run.

Fair enough, removed them again!

lib/fileset/README.md

infinisil · 2023-11-22T13:43:23Z

I'll soon merge this myself so we can unblock NixOS/nix.dev#802 :)

roberth

One suggestion, then lgtm.

lib/fileset/README.md

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>

github-actions · 2023-11-22T18:47:00Z

Successfully created backport PR for release-23.11:

[Backport release-23.11] lib.fileset.maybeMissing: init #269259

github-actions bot added the 6.topic: lib The Nixpkgs function library label Nov 7, 2023

infinisil mentioned this pull request Nov 7, 2023

lib.fileset.fromSource: init #261732

Merged

3 tasks

infinisil changed the title ~~lib.fileset.optional: init~~ lib.fileset.optional: init Nov 7, 2023

infinisil mentioned this pull request Nov 7, 2023

[WIP] File set combinators #222981

Closed

7 tasks

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

infinisil requested review from roberth and fricklerhandwerk November 7, 2023 15:27

infinisil mentioned this pull request Nov 7, 2023

Add lib.fileset.optional :: Bool -> FileSet -> FileSet #266106

Open

infinisil force-pushed the fileset.optional branch from 80eef12 to 54f292f Compare November 7, 2023 23:16

infinisil changed the title ~~lib.fileset.optional: init~~ lib.fileset.maybeMissing: init Nov 7, 2023

infinisil mentioned this pull request Nov 9, 2023

File set library tracking issue and feature requests #266356

Open

infinisil force-pushed the fileset.optional branch from 54f292f to 4e31015 Compare November 10, 2023 18:41

ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 10, 2023

infinisil mentioned this pull request Nov 12, 2023

lib.fileset: Lazier path existence check #267091

Closed

3 tasks

infinisil force-pushed the fileset.optional branch from 4e31015 to 23a68fc Compare November 16, 2023 00:01

ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 16, 2023

infinisil force-pushed the fileset.optional branch from 23a68fc to bf9a068 Compare November 19, 2023 00:37

roberth reviewed Nov 20, 2023

View reviewed changes

lib.fileset.maybeMissing: init

1cc2c2f

ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 20, 2023

infinisil force-pushed the fileset.optional branch from 4594aa0 to 9748413 Compare November 20, 2023 19:05

ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 20, 2023

infinisil added the backport release-23.11 label Nov 21, 2023

infinisil mentioned this pull request Nov 22, 2023

File sets tutorial NixOS/nix.dev#802

Merged

fricklerhandwerk approved these changes Nov 22, 2023

View reviewed changes

infinisil force-pushed the fileset.optional branch from 9748413 to cbaa880 Compare November 22, 2023 12:34

fricklerhandwerk approved these changes Nov 22, 2023

View reviewed changes

roberth approved these changes Nov 22, 2023

View reviewed changes

lib/fileset/README.md Outdated Show resolved Hide resolved

lib.fileset: Document decision for strict existence checks

827232d

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>

infinisil force-pushed the fileset.optional branch from a775c6c to 827232d Compare November 22, 2023 17:51

infinisil merged commit f37dd68 into NixOS:master Nov 22, 2023
6 checks passed

infinisil deleted the fileset.optional branch November 22, 2023 18:46

github-actions bot mentioned this pull request Nov 22, 2023

[Backport release-23.11] lib.fileset.maybeMissing: init #269259

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`lib.fileset.maybeMissing`: init #265964

`lib.fileset.maybeMissing`: init #265964

infinisil commented Nov 7, 2023 •

edited

Loading

roberth commented Nov 7, 2023

infinisil commented Nov 7, 2023

alyssais commented Nov 7, 2023

infinisil commented Nov 7, 2023 •

edited

Loading

roberth commented Nov 7, 2023

infinisil commented Nov 7, 2023

roberth commented Nov 7, 2023

fricklerhandwerk commented Nov 8, 2023

infinisil commented Nov 8, 2023

alyssais commented Nov 10, 2023

alyssais commented Nov 10, 2023

infinisil commented Nov 11, 2023

roberth commented Nov 11, 2023

infinisil commented Nov 12, 2023

infinisil commented Nov 16, 2023

infinisil commented Nov 19, 2023

roberth Nov 20, 2023

infinisil Nov 20, 2023

roberth Nov 20, 2023

infinisil Nov 20, 2023

roberth Nov 20, 2023

infinisil Nov 20, 2023 •

edited

Loading

infinisil commented Nov 21, 2023

fricklerhandwerk left a comment

fricklerhandwerk Nov 22, 2023

infinisil Nov 22, 2023

infinisil commented Nov 22, 2023

roberth left a comment

github-actions bot commented Nov 22, 2023

	lib.fileset.maybeMissing: Argument ("${toString path}") is a string-like value, but it should be a path instead.''
	lib.fileset.maybeMissing: Argument ("${toString path}") is a string-like value, but it should be a path instead. See https://nixos.org/manual/nix/stable/language/values#type-path''

lib.fileset.maybeMissing: init #265964

lib.fileset.maybeMissing: init #265964

Conversation

infinisil commented Nov 7, 2023 • edited Loading

Description of changes

Things done

roberth commented Nov 7, 2023

infinisil commented Nov 7, 2023

alyssais commented Nov 7, 2023

infinisil commented Nov 7, 2023 • edited Loading

roberth commented Nov 7, 2023

infinisil commented Nov 7, 2023

roberth commented Nov 7, 2023

fricklerhandwerk commented Nov 8, 2023

infinisil commented Nov 8, 2023

alyssais commented Nov 10, 2023

alyssais commented Nov 10, 2023

infinisil commented Nov 11, 2023

roberth commented Nov 11, 2023

infinisil commented Nov 12, 2023

infinisil commented Nov 16, 2023

infinisil commented Nov 19, 2023

roberth Nov 20, 2023

Choose a reason for hiding this comment

infinisil Nov 20, 2023

Choose a reason for hiding this comment

roberth Nov 20, 2023

Choose a reason for hiding this comment

infinisil Nov 20, 2023

Choose a reason for hiding this comment

roberth Nov 20, 2023

Choose a reason for hiding this comment

infinisil Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

infinisil commented Nov 21, 2023

fricklerhandwerk left a comment

Choose a reason for hiding this comment

fricklerhandwerk Nov 22, 2023

Choose a reason for hiding this comment

infinisil Nov 22, 2023

Choose a reason for hiding this comment

infinisil commented Nov 22, 2023

roberth left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 22, 2023

`lib.fileset.maybeMissing`: init #265964

`lib.fileset.maybeMissing`: init #265964

infinisil commented Nov 7, 2023 •

edited

Loading

infinisil commented Nov 7, 2023 •

edited

Loading

infinisil Nov 20, 2023 •

edited

Loading