Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

File sets tutorial #802

Merged
merged 51 commits into from
Nov 23, 2023
Merged

File sets tutorial #802

merged 51 commits into from
Nov 23, 2023

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Nov 19, 2023

This is a tutorial for the lib.fileset library I've been working on recently.

Initially this also included a tutorial on builtin features for adding files to the store, but this has now been moved to a gist, since it's not really best practice.

This work is sponsored by Antithesis

Copy link
Contributor

github-actions bot commented Nov 19, 2023

@github-actions github-actions bot temporarily deployed to pull request November 19, 2023 03:29 Inactive
@infinisil infinisil force-pushed the source-file-selection branch from 05405cb to 197782d Compare November 19, 2023 23:00
@github-actions github-actions bot temporarily deployed to commit November 19, 2023 23:01 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 19, 2023 23:01 Inactive
@github-actions github-actions bot temporarily deployed to commit November 19, 2023 23:39 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 19, 2023 23:39 Inactive
Copy link

@bzm3r bzm3r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this! I love reading new tutorials. I have a couple of suggestions, but I'm not really sure what the nix.dev policy is around flakes/"experimental commands", so I might be off-base.

source/tutorials/source-file-selection.md Outdated Show resolved Hide resolved
source/tutorials/source-file-selection.md Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to commit November 20, 2023 04:48 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 20, 2023 04:48 Inactive
@infinisil infinisil force-pushed the source-file-selection branch 2 times, most recently from f8c6e16 to 87a30e1 Compare November 20, 2023 04:58
@github-actions github-actions bot temporarily deployed to commit November 20, 2023 04:59 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 20, 2023 04:59 Inactive
@infinisil infinisil force-pushed the source-file-selection branch from 87a30e1 to 0120fa2 Compare November 20, 2023 05:01
@github-actions github-actions bot temporarily deployed to commit November 20, 2023 05:01 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 20, 2023 05:01 Inactive
@infinisil infinisil force-pushed the source-file-selection branch from 0120fa2 to 239f2d9 Compare November 20, 2023 05:02
@github-actions github-actions bot temporarily deployed to commit November 20, 2023 05:03 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 20, 2023 05:03 Inactive
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-11-20-documentation-team-meeting-notes-95/35769/1

@github-actions github-actions bot temporarily deployed to commit November 20, 2023 20:44 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 20, 2023 20:44 Inactive
@github-actions github-actions bot temporarily deployed to commit November 22, 2023 19:16 Inactive
@github-actions github-actions bot temporarily deployed to commit November 22, 2023 19:19 Inactive
@github-actions github-actions bot temporarily deployed to commit November 22, 2023 19:21 Inactive
@github-actions github-actions bot temporarily deployed to commit November 22, 2023 19:30 Inactive
Use master branch (nixos-unstable doesn't have maybeMissing yet)
@github-actions github-actions bot temporarily deployed to commit November 22, 2023 19:46 Inactive
@github-actions github-actions bot temporarily deployed to commit November 22, 2023 20:33 Inactive
@github-actions github-actions bot temporarily deployed to commit November 22, 2023 20:48 Inactive
@infinisil
Copy link
Member Author

infinisil commented Nov 22, 2023

@fricklerhandwerk @roberth In a full read-through I made some adjustments in 69e6dd6.

And now i also removed the initial part using src = ./.. I don't think that added much, it's not good practice. And src = ./hello.txt failing was only incidental, it's no reason for wanting the file set library. 9ecfd01

I also switched to use the master branch now, such that maybeMissing from NixOS/nixpkgs#265964 is there now, I verified that it works as is. I'll follow up at the latest after the release.

This is mergeable now. (Edit: @fricklerhandwerk wants to do another review)

source/tutorials/file-sets.md Outdated Show resolved Hide resolved
Comment on lines 27 to 28
In this tutorial you'll learn how to use Nixpkgs' [`lib.fileset` library](https://nixos.org/manual/nixpkgs/unstable/#sec-fileset) to work with local files in derivations.
It abstracts over built-in functionality with a safer and more convenient interface.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd revert both sentences. Apostrophising Nixpkgs looks really weird, and abstracting over X with Y, while grammatically correct can be misleading to be read as "built in functionality having a safer interface".


## File sets

The file set library is based on the concept of _file sets_,
The basic concept is that of a _file set_,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert too (or if you want file set as singular, say "based on the concept of a file set". "The basic concept is" - of what? What's the subject? And starting with "the basic concept" even if you say "of the file set library" is probably not even important, and the original sentence said what we want: that the file set revolves around this basic concept.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the docs team meeting right now, changing it to

A file set is a data type representing a collection of local files.
File sets can be created, composed, and manipulated with the various functions of the library.

@@ -48,7 +53,7 @@ trace: /home/user (all files in directory)
null
```

All functions that expect a file set for an argument also accept a [path](https://nixos.org/manual/nix/stable/language/values#type-path).
All functions that expect a file set for an argument also accept a [path](https://nixos.org/manual/nix/stable/language/values#type-path) instead.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert, because "instead" can be misinterpreted as "mutually exclusive".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing it to

All functions that expect a file set for an argument can also accept a path.

@@ -64,14 +69,14 @@ trace: /home/user (all files in directory)
:::

Even though file sets conceptually contain local files, these files are *never* added to the Nix store unless explicitly requested.
You don't have to worry as much about accidentally copying secrets into the world-readable store.
So You don't have to worry as much about accidentally copying secrets into the world-readable store.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Capitalisation. Not sure if the connective is really needed here. I would (and did) skip it.

Copy link
Member Author

@infinisil infinisil Nov 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing to

Therefore you don't have to worry as much about accidentally copying secrets into the world-readable store.

which copies the whole directory to the Nix store on evaluation!

:::{warning}
When using [experimental Flakes](https://nixos.org/manual/nix/stable/command-ref/new-cli/nix3-flake), local directories with a `flake.nix` are always copied into the Nix store *completely* unless it is a Git repository!
Copy link
Collaborator

@fricklerhandwerk fricklerhandwerk Nov 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are "experimental Flakes"? Are there non-experimental flakes?

I still think that should be "With the flakes experimental feature enabled, ...", and if you insist it also needs nix-command, then I suggest writing that out.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using the flakes and nix-command experimental features,
a local directory within a Flake is always copied into the Nix store completely unless it is a Git repository!

In the meeting

Comment on lines 136 to 137
To add files in a given file set to the Nix store,
we use [`toSource`](https://nixos.org/manual/nixpkgs/unstable/#function-library-lib.fileset.toSource).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who is "we" here? It makes sense to say "we" when we're referring to this group writing the tutorial, e.g. when explaining why we're telling people to fetch from master rather than nixpkgs-unstable. But here here we'd run the danger of saying "we use this, you do you", while we really want to either state a fact "X does Y" or tell people what to do.

Suggested change
To add files in a given file set to the Nix store,
we use [`toSource`](https://nixos.org/manual/nixpkgs/unstable/#function-library-lib.fileset.toSource).
Files in a given file set can be added to the Nix store with [`toSource`](https://nixos.org/manual/nixpkgs/unstable/#function-library-lib.fileset.toSource).

I don't super like the phrasing, but it should be good enough.

Comment on lines 138 to 139
This function needs a `root` attribute to know which path to use as the root of the resulting store path,
but only exactly the files in the `fileset` argument are included in the result.
Copy link
Collaborator

@fricklerhandwerk fricklerhandwerk Nov 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"root of store path" is potentially confusing. Isn't that /nix/store?

There is no "but" here, because one part doesn't contradict the other.

Suggested change
This function needs a `root` attribute to know which path to use as the root of the resulting store path,
but only exactly the files in the `fileset` argument are included in the result.
The argument to this function requires a `root` attribute to determine which source directory to copy to the store.
Exactly the files in the `fileset` attribute are included in the result.
Suggested change
This function needs a `root` attribute to know which path to use as the root of the resulting store path,
but only exactly the files in the `fileset` argument are included in the result.
The argument to this function requires a `root` attribute to determine which source directory to copy to the store, and exactly the files in the `fileset` attribute are included in the result.

I'm not sure about the exact phrasing, I find it a bit hard to package it nicely, but something like this.

PS: I really don't like breaking sentences up into multiple lines, because edits are usually based on sentence, not phrase. Don't know about others, but dealing with multiple lines in the GitHub UI almost always make it more tedious for me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used

The argument to this function requires a root attribute to determine which source directory to copy to the store.
Only the files in the fileset attribute are included in the result.


The call to `fs.trace` prints the file set that will be used as a derivation input.

The build will succeed (note that it will take some time to fetch the first time around):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should take some time already when doing the first trace, so either note it there or skip.

Suggested change
The build will succeed (note that it will take some time to fetch the first time around):
Try building it:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happens here due to niv not being able to re-use the download.

Changed it to an admonition though (see commit)

@@ -312,7 +316,7 @@ result -> /nix/store/xknflcvjaa8dj6a6vkg629zmcrgz10rh-fileset
Since `src` refers to the whole directory, and its contents change when `nix-build` succeeds, Nix will have to start over every time.

:::{note}
This will also happen without the file set library, e.g. when setting `src = ./.;` directly.
This will also happen with `src = ./.`, without using the file set library.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence structure is potentially confusing. What you probably mean here is "... i.e., without using the file set library", but that makes it a lot more clunky. I'd revert that too.

Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
@github-actions github-actions bot temporarily deployed to commit November 23, 2023 14:59 Inactive
Copy link
Collaborator

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few kinks found their way in among a lot of good corrections. Ready to merge from my side after resolving the icky ones.

Great stuff, looking forward to getting this out there.

Comment on lines 415 to 416
With the [`fileFilter`](https://nixos.org/manual/nixpkgs/unstable/#function-library-lib.fileset.fileFilter) function,
you can find sets of files satisfying a criteria.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But filter doesn't find anything. Criteria is plural, the singular is criterion.

Suggested change
With the [`fileFilter`](https://nixos.org/manual/nixpkgs/unstable/#function-library-lib.fileset.fileFilter) function,
you can find sets of files satisfying a criteria.
The [`fileFilter`](https://nixos.org/manual/nixpkgs/unstable/#function-library-lib.fileset.fileFilter) function allows filtering file sets such that each file satisfies the given criteria.

I think it sounds needlessly fancy and doesn't really hint at what to do with it, but okay.

Here's the last version for reference, which is buried under a lot of commits:

Suggested change
With the [`fileFilter`](https://nixos.org/manual/nixpkgs/unstable/#function-library-lib.fileset.fileFilter) function,
you can find sets of files satisfying a criteria.
Dealing with a large number of files, independent of their location, can be done programmatically with the [`fileFilter`](https://nixos.org/manual/nixpkgs/unstable/#function-library-lib.fileset.fileFilter) function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fileFilter function allows filtering file sets such that each included file satisfies the given criteria.


Then create a file set from only the files to be included explicitly:

```{code-block} diff
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was intentionally a full file, because the diff is so large, and I think that makes it a lot harder to read. Also it's a good point for readers to make sure that they have the right thing in front of them. You have to follow the diffs manually, so indefinite accumulation of changes is really error prone. I suggest reverting that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it back to a partial file, no diff.

Only the specified files are used, even when a new one is added:

```shell-session
$ touch src/select.o README.md
Copy link
Collaborator

@fricklerhandwerk fricklerhandwerk Nov 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$ touch src/select.o README.md
$ touch src/select.o

Why do we need another one? It's not used anywhere else. Not important though...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discussed in meeting, it's fine to have it


Now we can re-use this selection of files using `gitTracked`:

```{code-block} diff
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, there's no reason to do the diff here just for the sake of consistency. We have enough repetition that we can expect readers to have understood that all we care about here is the sourceFiles attribute. I'd revert that.

$ git reset src/select.o result
```

Now we can re-use this selection of files using `gitTracked`:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Now we can re-use this selection of files using `gitTracked`:
Re-use this selection of files with `gitTracked`:

in
```

Building we get
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Building we get
Build it again:

I forgot touching that in my first review pass.

This includes too much though, as not all of these files are needed to build the derivation as originally intended.

:::{note}
When using [experimental Flakes](https://nixos.org/manual/nix/stable/command-ref/new-cli/nix3-flake), it's [not really possible](https://github.com/NixOS/nix/issues/9292) to use this function, even with
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above for "experimental Flakes"

## Intersection

This is where `intersection` comes in.
It allows creating a file set that consists only of files that are in _both_ of two given file sets.
Copy link
Collaborator

@fricklerhandwerk fricklerhandwerk Nov 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
It allows creating a file set that consists only of files that are in _both_ of two given file sets.
It allows creating a file set that consists only of files that are in _both_ of the two given file sets.

This is more idiomatic, "both of the two x" seems to be a fixed expression.

Also, if you think about it, union takes exactly two file sets, they're definite; it's not arbitrary that it's two. "Both of the two it takes"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed, it's fine without :)


Select all files that are both tracked by Git *and* relevant for the build:

```{code-block} diff
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above on the diff.

@github-actions github-actions bot temporarily deployed to commit November 23, 2023 16:07 Inactive
@infinisil
Copy link
Member Author

Reviewed together in the docs team meeting today with @fricklerhandwerk, @proofconstruction, @henrik-ch, @DanielSidhion and @wamirez, let's merge this!

@fricklerhandwerk fricklerhandwerk merged commit 614b6fe into master Nov 23, 2023
10 checks passed
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-11-23-documentation-team-meeting-notes-96/35918/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants