`lib.fileset.fromSource`: init by infinisil · Pull Request #261732 · NixOS/nixpkgs

infinisil · 2023-10-17T22:22:56Z

Description of changes

Another split off from #222981, motivated by @alyssais's comment here. This introduces one new function:

lib.fileset.fromSource: Create a file set from a filtered local source as produced by the lib.sources functions.

lib.fileset.toSource {
  root = ./.;
  fileset = lib.fileset.intersection
    (lib.fileset.fromSource (lib.sources.cleanSource ./.))
    (lib.fileset.unions [
      ./Makefile
      ./src
    ]);
}

This is mainly useful for migration from lib.sources-based filtering. In the future we should have a convenience function in lib.fileset that could take over.

This work is sponsored by Antithesis ✨

Things done

Tests
Clean code
Documentation

infinisil · 2023-11-01T22:49:46Z

I'm wondering if this will still be necessary with #264891 🤔

alyssais · 2023-11-02T08:16:28Z

Definitely is IMO. Not everything is or should be a git repository.

alyssais · 2023-11-05T07:41:27Z

Another reason I can't switch from sources to filesets yet:

I want to be able to do, essentially: src = fileset.difference ./. ./generated.o, where generated.o is some generated file that may or may not exist in my development tree, but that shouldn't be input to the derivation. This is not currently possible, because filesets require all files to exist. So there needs to be a way to filter a fileset that doesn't require the files in the negative set to exist.

infinisil · 2023-11-07T00:07:38Z

It is possible to do this already with

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

But I think a convenient shorthand for this would be appropriate, I also had that in #222981, so I just created #265964 which adds a lib.fileset.optional!

fricklerhandwerk

That's a really valuable addition to the library, as you said because it would ease migration a lot. We may want to note that in the docstring, along the lines of

use this to gradually migrate builds relying on lib.sources to lib.fileset

but that's no reason to block this PR.

PS: Not sure why it's still in draft mode, looks ready to me.

fricklerhandwerk · 2023-11-08T00:42:42Z

lib/fileset/default.nix

What's a source-like value? What's a source?

"Very" good question unfortunately. Basically a path value or the "source attrset" returned by the lib.sources functions. Can we write introductions for sublibraries yet? It'd be great to have a sensible place to document the unique types of each sublibrary.

Can we write introductions for sublibraries yet?

Almost

fricklerhandwerk · 2023-11-08T00:52:10Z

lib/fileset/default.nix

The example in the PR description is also relevant, because it shows a bit of integration.

Added some more practical examples now

roberth

Remember to test that directories for which the predicate is false are not read. This is part of the (implicit) contract.
Other than that, I think the tests will "write themselves" if that makes sense.

lib/fileset/internal.nix

roberth · 2023-11-08T00:58:33Z

lib/fileset/default.nix

This doesn't seem to actually take SourceLike (although I didn't define that in words in lib.sources, you could perhaps tell from the code or behavior).

That type was meant to cover all source-coercible types, similar to how you allow both paths and filesets as arguments to fileset functions. In fact, it seems to be pretty much exactly that.

So what I meant to say is lib.sources users will probably expect to be able to use any source here, whether that's an _isLibCleanSourceWith, or a path value. Rejecting path values makes libraries (e.g. lang2nix) that use this fragile.

Not simplifying fromSource ./. to ./. is harmless, while { src, ... }: f (fromSource src) would be a bug waiting to happen.

infinisil · 2023-11-08T01:30:00Z

PS: Not sure why it's still in draft mode, looks ready to me.

See the checklist in the PR description 🙂. Generally I recommend not taking close looks at the code of draft PRs. If I request you for a review it's because I'd like feedback to the approach only. I'll mark PRs as ready to review when I think their implementation is ready to review.

The review comments you both left are very helpful though, will incorporate!

infinisil · 2023-11-08T04:34:59Z

Other than that, I think the tests will "write themselves" if that makes sense.

It didn't make sense to me, but I wrote tests anyways and pushed them now :)

Still need to clean up the code and docs a bit, including addressing the review comments. I'll mark it as ready when everything is addressed.

roberth

One question; otherwise lgtm.

roberth · 2023-11-10T18:54:02Z

lib/fileset/default.nix

+        throw ''
+          lib.fileset.toSource: `root` is a `lib.sources`-based value, but it should be a path instead.
+              To use a `lib.sources`-based value, convert it to a file set using `lib.fileset.fromSource` and pass it as `fileset`.
+              Note that this only works for sources created from paths.''


Any reason not to just call it for them?

I'm not 100% convinced it's a good idea, just minor ideas like

The operation this function does is somewhat expensive, so it being explicit makes people aware of that. This is different from the implicit path coercion, which is very cheap

Do we want to deprecate this at some point? Probably not

Source filters can be impure by changing behavior based on the project directory, does this impurity leak into file sets? Probably not a problem

Is it a problem to have a lack of symmetry regarding toSource/fromSource? Probably not, these aren't symmetric, fromSource discard's the root and toSource has to add it

Would it be a problem to have cases like toSource { root = ./.; fileset = toSource { root = ./.; fileset = toSource { ...? Probably also no

Furthermore this being an error for now means that we could still do this later.

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

This was referenced Oct 17, 2023

lib.sourceSubdirs: init #234297

Closed

[WIP] File set combinators #222981

Closed

infinisil requested a review from alyssais October 17, 2023 22:50

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Oct 18, 2023

This was referenced Oct 31, 2023

Add filterFiles to lib and lib.sources #188301

Closed

lib.fileset: Write down the rationale for not supporting store paths #264537

Closed

infinisil changed the title ~~lib.fileset.fromSource: init~~ lib.fileset.fromSource: init Nov 1, 2023

infinisil requested review from fricklerhandwerk and roberth November 1, 2023 22:50

infinisil mentioned this pull request Nov 7, 2023

lib.fileset.maybeMissing: init #265964

Merged

3 tasks

fricklerhandwerk approved these changes Nov 8, 2023

View reviewed changes

fricklerhandwerk reviewed Nov 8, 2023

View reviewed changes

roberth reviewed Nov 8, 2023

View reviewed changes

lib.fileset: Split out internal test helper

f82a168

infinisil force-pushed the fileset.fromSource branch 3 times, most recently from 4133f7a to 3dfaf15 Compare November 8, 2023 04:32

lib.fileset.fromSource: init

7349358

infinisil force-pushed the fileset.fromSource branch from 3dfaf15 to 7349358 Compare November 8, 2023 20:21

infinisil marked this pull request as ready for review November 8, 2023 20:24

infinisil requested review from fricklerhandwerk and roberth November 8, 2023 20:24

lib.fileset.toSource: Mention fromSource in errors

e07e80e

fricklerhandwerk approved these changes Nov 8, 2023

View reviewed changes

infinisil mentioned this pull request Nov 9, 2023

File set library tracking issue and feature requests #266356

Open

roberth reviewed Nov 10, 2023

View reviewed changes

roberth approved these changes Nov 10, 2023

View reviewed changes

roberth merged commit cfd83c9 into NixOS:master Nov 10, 2023

infinisil deleted the fileset.fromSource branch November 10, 2023 19:33

Uh oh!

Comments

Conversation

infinisil commented Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Things done

Uh oh!

infinisil commented Nov 1, 2023

Uh oh!

alyssais commented Nov 2, 2023

Uh oh!

alyssais commented Nov 5, 2023

Uh oh!

infinisil commented Nov 7, 2023

Uh oh!

fricklerhandwerk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roberth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

infinisil commented Nov 8, 2023

Uh oh!

infinisil commented Nov 8, 2023

Uh oh!

roberth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

infinisil commented Oct 17, 2023 •

edited

Loading

fricklerhandwerk left a comment •

edited

Loading