`lib.fileset.union`, `lib.fileset.unions`: init #255025

infinisil · 2023-09-13T21:42:29Z

Description of changes

This is another split off from the file set combinators PR, based on top of the already-merged lib.fileset.toSource and fileset infrastructure.

This adds two new functions, allowing the creation of unions of file sets:

lib.fileset.toSource {
  root = ./.;
  # Includes ./Makefile and recursively all files in ./src
  fileset = lib.fileset.union
    ./Makefile
    ./src;
}

lib.fileset.toSource {
  # Needed to be able to include a file from the parent directory
  root = ./..;
  # unions takes a list instead, only the given paths are included, nothing else
  fileset = lib.fileset.unions [
    ./Makefile
    ./src
    # Also include a file from the parent directory
    ../LICENSE
  ];
}

Comparatively, doing this correctly using the existing lib.sources functions is notoriously difficult.

This work is sponsored by Antithesis ✨

Things done

We can now test returned paths being equal, no need to work around it anymore by making sure paths aren't returned (which would import them with the previous --json)

nixos-discourse · 2023-09-13T21:55:30Z

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

https://discourse.nixos.org/t/easy-source-filtering-with-file-sets/29117/11

fricklerhandwerk

Noice

lib/fileset/internal.nix

roberth

Minor things and a concern about stack overflows when thunks build up in a lazy part despite foldl' (might be due to incomplete understanding of filesets, but test would be appreciated)

lib/fileset/README.md

lib/fileset/default.nix

lib/fileset/internal.nix

infinisil · 2023-09-14T22:13:56Z

@roberth @fricklerhandwerk Thanks! I applied the suggestions

Good catch on the stack overflow potential @roberth, indeed it does stack overflow with many files! I fixed that now and included a test case with ulimit -s 100 that would break if unions was implemented recursively.

I also added a benchmark for unions and discovered that it can be sped up by a factor of 12! (The benchmark now also measures time)

I also opened two nixdoc issues:

infinisil · 2023-09-20T18:38:10Z

I'm fairly happy with this now, do you want to take a closer look @roberth?

roberth

Two suggestions, but lgtm now!

roberth · 2023-09-20T20:34:00Z

lib/fileset/default.nix

+      # Annotate the elements with context, used by _coerceMany for better errors
+      annotated = imap0 (i: el: {
+        context = "element ${toString i}";
+        value = el;
+      }) arg;
+
+      filesets = _coerceMany "lib.fileset.unions" annotated;


These bindings are only used once. Are you sure the earlier renaming is necessary?

Not necessary yeah, though just passing the argument between functions directly was a bit unsightly, so I'm using lib.pipe instead now: https://github.com/NixOS/nixpkgs/compare/622cdf4567bc0e98a0926df7ea8ffedf89d94298..1498af4bf1ab058053ff35a4f601e02859420828

I can never remember the order of pipe, probably because I dislike it.
Whatever works, I guess.

roberth · 2023-09-20T20:37:11Z

lib/fileset/default.nix

+    if ! isList arg then
+      throw "lib.fileset.unions: Expected argument to be a list, but got a ${typeOf arg}."
+    else if arg == [ ] then
+      # TODO: This could be supported, but requires an extra internal representation for the empty file set


Suggested change

# TODO: This could be supported, but requires an extra internal representation for the empty file set

# TODO: This could be supported, but requires an extra internal representation for the empty file set, which would be special for not having a root.

Did s/root/base path, since that's the term used throughout

Also some minor formatting improvements

Previously a lot of processes were used, slowing it down considerably the more files were tested

$ ./benchmark.sh HEAD [...] Mean CPU time 0.04006 (σ = 0.0040146) for 10 runs is 8.193619775953792% (σ = 0.9584251052704821%) of the old value 0.488917 (σ = 0.0294955) [...]

Co-authored-by: Robert Hensing <[email protected]> Co-authored-by: Valentin Gagarin <[email protected]>

infinisil · 2023-09-20T22:23:05Z

Very minor push to add a level of indentation to doc comments (diff)

Thanks for the review Robert, I'll merge this when I see it next time with CI passing!

infinisil added 4 commits September 13, 2023 18:43

lib/fileset: Test function improvement

48abfde

We can now test returned paths being equal, no need to work around it anymore by making sure paths aren't returned (which would import them with the previous --json)

lib.fileset: Internal representation v1

19b39dc

lib.fileset.toSource: Evaluate fileset even for empty directories

7d4eb3f

lib.fileset: Minor internal type doc fix

7c6b0b1

infinisil requested review from roberth and fricklerhandwerk September 13, 2023 21:42

github-actions bot added 8.has: documentation This PR adds or changes documentation 6.topic: lib The Nixpkgs function library labels Sep 13, 2023

infinisil mentioned this pull request Sep 13, 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 Sep 13, 2023

fricklerhandwerk approved these changes Sep 14, 2023

View reviewed changes

lib/fileset/internal.nix Outdated Show resolved Hide resolved

delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Sep 14, 2023

roberth reviewed Sep 14, 2023

View reviewed changes

delroth removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Sep 14, 2023

infinisil force-pushed the fileset.union branch from 4e9e8c3 to 622cdf4 Compare September 15, 2023 10:06

roberth approved these changes Sep 20, 2023

View reviewed changes

infinisil force-pushed the fileset.union branch from 622cdf4 to 1498af4 Compare September 20, 2023 20:59

infinisil added 9 commits September 21, 2023 00:19

lib.fileset.union: init

d866a0b

lib.fileset.unions: init

bd52895

lib.fileset: Create tests for union and unions

f78d650

lib.fileset: Various updates relating to union/unions

c5ae093

Also some minor formatting improvements

lib.fileset: Optimise tests

e04e40d

Previously a lot of processes were used, slowing it down considerably the more files were tested

lib.fileset.unions: Don't stack overflow for many files

7ab764e

lib.fileset: More reusable benchmark code

631ad21

lib.fileset: Add benchmark for unions

c8bac6e

lib.fileset: Have benchmark.sh measure the time

c9c9c09

infinisil and others added 4 commits September 21, 2023 00:21

lib.fileset: Use a nix-shell shebang for benchmark.sh

e05cf47

lib.fileset: Ignore irrelevant shellcheck warnings

45bf2c7

lib.fileset: Internal representation v2, ~12x faster unions!

fe6c153

$ ./benchmark.sh HEAD [...] Mean CPU time 0.04006 (σ = 0.0040146) for 10 runs is 8.193619775953792% (σ = 0.9584251052704821%) of the old value 0.488917 (σ = 0.0294955) [...]

lib.fileset: Minor changes from feedback

94e103e

Co-authored-by: Robert Hensing <[email protected]> Co-authored-by: Valentin Gagarin <[email protected]>

infinisil force-pushed the fileset.union branch from 1498af4 to 94e103e Compare September 20, 2023 22:21

roberth merged commit 5c97f01 into NixOS:master Sep 21, 2023
6 checks passed

infinisil deleted the fileset.union branch September 21, 2023 10:29

infinisil mentioned this pull request Sep 29, 2023

lib.fileset: Don't use non-reproducible ulimit for stack overflow testing #258080

Merged

1 task

infinisil mentioned this pull request Oct 16, 2023

lib.sourceSubdirs: init #234297

Closed

12 tasks

infinisil mentioned this pull request Nov 9, 2023

File set library tracking issue and feature requests #266356

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`lib.fileset.union`, `lib.fileset.unions`: init #255025

`lib.fileset.union`, `lib.fileset.unions`: init #255025

infinisil commented Sep 13, 2023 •

edited

Loading

nixos-discourse commented Sep 13, 2023

fricklerhandwerk left a comment

roberth left a comment

infinisil commented Sep 14, 2023 •

edited

Loading

infinisil commented Sep 20, 2023

roberth left a comment

roberth Sep 20, 2023

infinisil Sep 20, 2023 •

edited

Loading

roberth Sep 20, 2023

roberth Sep 20, 2023

infinisil Sep 20, 2023

infinisil commented Sep 20, 2023 •

edited

Loading

	# TODO: This could be supported, but requires an extra internal representation for the empty file set
	# TODO: This could be supported, but requires an extra internal representation for the empty file set, which would be special for not having a root.

lib.fileset.union, lib.fileset.unions: init #255025

lib.fileset.union, lib.fileset.unions: init #255025

Conversation

infinisil commented Sep 13, 2023 • edited Loading

Description of changes

Things done

nixos-discourse commented Sep 13, 2023

fricklerhandwerk left a comment

Choose a reason for hiding this comment

roberth left a comment

Choose a reason for hiding this comment

infinisil commented Sep 14, 2023 • edited Loading

infinisil commented Sep 20, 2023

roberth left a comment

Choose a reason for hiding this comment

roberth Sep 20, 2023

Choose a reason for hiding this comment

infinisil Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

roberth Sep 20, 2023

Choose a reason for hiding this comment

roberth Sep 20, 2023

Choose a reason for hiding this comment

infinisil Sep 20, 2023

Choose a reason for hiding this comment

infinisil commented Sep 20, 2023 • edited Loading

`lib.fileset.union`, `lib.fileset.unions`: init #255025

`lib.fileset.union`, `lib.fileset.unions`: init #255025

infinisil commented Sep 13, 2023 •

edited

Loading

infinisil commented Sep 14, 2023 •

edited

Loading

infinisil Sep 20, 2023 •

edited

Loading

infinisil commented Sep 20, 2023 •

edited

Loading