lib.fileset.gitTracked: Support out-of-tree builds#273893
lib.fileset.gitTracked: Support out-of-tree builds#273893infinisil merged 3 commits intoNixOS:masterfrom
lib.fileset.gitTracked: Support out-of-tree builds#273893Conversation
ofborg
bot
added
10.rebuild-darwin: 0
infinisil
force-pushed
the
fileset.gitTracked-storeDir
branch
from
19dc2b3 to
1f5b475
Compare
|
This is now ready! As always, comes with docs, tests and comments. Best reviewed commit-by-commit :) |
lib/fileset/internal.nix
Outdated
There was a problem hiding this comment.
Even then, it'd be inefficient to copy at least a whole .git into the store every time. Those can be quite big.
| See https://github.com/NixOS/nix/issues/9292.'' | |
| You could make this work by using a fetcher such as `fetchGit` instead of copying the whole repository. | |
| If you can't avoid copying the repo to the store, see https://github.com/NixOS/nix/issues/9292.'' |
It would be great to link a more useful resource that explains what to do instead, because I don't think anyone should rely on that issue to be fixed, because copying a repo into the store is just not good for performance.
Perhaps sometimes even security, if it actually happened to be local and it has unencrypted un-added secrets, although that's a vaguely recognizable condition, that does not originate from fileset, and isn't fixable by fileset.
There was a problem hiding this comment.
Yeah, for now I think your suggestion is good. This is such a rare case that I don't think it's worthwhile writing more about it for the time being.
lib/fileset/default.nix
Outdated
There was a problem hiding this comment.
This paragraph doesn't feel relatable to someone who's writing a source filter for their project, because they're not using fetchGit. I think what we're trying to explain here is that gitTracked tries to do the right thing if what's a repo for them is filtered away by someone else. gitTracked can only assume that this someone else fetched it the right way, and that's relevant, because gitTracked would seem involved in potential problems that are beyond its power.
(Sorry, no time for a suggestion rn)
There was a problem hiding this comment.
| Git repositories can also be fetched as a [Nix store path](https://nixos.org/manual/nix/stable/store/store-path.html#store-path) | |
| using a Git tree fetcher like `builtins.fetchGit` or `pkgs.fetchgit` (without `leaveDotGit`), | |
| for the purposes of so-called out-of-tree building. | |
| If such a fetched Git repository uses `gitTrackedWith` on a local path and is evaluated using `import`, | |
| the function returns the Git-tracked files even in such a case. | |
| So this function doesn't interfere with out-of-tree building. | |
| `gitTrackedWith` does not perform any filtering when the path is a [Nix store path](https://nixos.org/manual/nix/stable/store/store-path.html#store-path) and not a repository. | |
| In this way, it accommodates the use case where the expression that makes the `gitTracked` call does not reside in an actual git repository anymore, and has presumably already been fetched in a way that excludes untracked files. | |
| Fetchers that are expected to implement such equivalent behavior include `builtins.fetchGit`, `builtins.fetchTree` (experimental), and `pkgs.fetchgit` when used without `leaveDotGit`. | |
so-called
Can do without the jargon.
There was a problem hiding this comment.
The last sentence feels a bit like fetchers need to implement support for this explicitly, how about just:
Fetchers with such behavior include
builtins.fetchGit,builtins.fetchTree(experimental), andpkgs.fetchgitwhen used withoutleaveDotGit.
Otherwise this sounds good to me
There was a problem hiding this comment.
Yeah sgtm.
I wish we could just claim equivalence, but for example fetchGit implements export-ignore by default for historic reasons.
There was a problem hiding this comment.
Oh the "default" part is WIP actually...
infinisil
force-pushed
the
fileset.gitTracked-storeDir
branch
from
1f5b475 to
201be69
Compare
|
@roberth Applied the suggestions with my suggestion-suggestion :) |
delroth
added
the
12.approvals: 1
infinisil
force-pushed
the
fileset.gitTracked-storeDir
branch
from
201be69 to
38cf6ff
Compare
|
Ugh CI failed because I forgot to update the tests after updating the I won't bother waiting another 3h just for CI to finish.. I tested it locally, it works now |
|
Successfully created backport PR for |
Now that NixOS/nixpkgs#273893 is merged, the previous note isn't accurate anymore.
Now that NixOS/nixpkgs#273893 is merged, the previous note isn't accurate anymore.
Now that NixOS/nixpkgs#273893 is merged, the previous note isn't accurate anymore.
Now that NixOS/nixpkgs#273893 is merged, the previous note isn't accurate anymore.
3 participants
Note
This PR depended on #273883, now merged.
Originally it also depended on #273884, but not anymore.
Description of changes
This adds out-of-tree support for
lib.fileset.gitTracked, where the repository is fetched using a method that already removes files not tracked by Git, such asbuiltins.fetchGit,pkgs.fetchgitor GitHub tarballs.It works by falling back to importing the entire path if:
.gitdirectoryThis fixes #269283.
This work is sponsored by Antithesis ✨
Things done
Add a 👍 reaction to pull requests you find important.