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

Make builtins.fetchTree return a path as its outPath element #10115

Open
thufschmitt opened this issue Feb 29, 2024 · 4 comments · May be fixed by #10225
Open

Make builtins.fetchTree return a path as its outPath element #10115

thufschmitt opened this issue Feb 29, 2024 · 4 comments · May be fixed by #10225
Assignees
Labels
feature Feature request or proposal fetching Networking with the outside (non-Nix) world, input locking

Comments

@thufschmitt
Copy link
Member

thufschmitt commented Feb 29, 2024

Is your feature request related to a problem? Please describe.

(builtins.fetchTree foo).outPath is currently a string (pointing to a path in the Nix store).
This means that something like (builtins.fetchTree foo).outPath + "/foo" will copy the whole tree into the store (to get the outPath string), and then return that store path + /foo.

Describe the solution you'd like

Make it a path value instead, and benefit from the extra laziness of paths (thus getting part of the lazy-trees benefits).

This would mean that (builtins.fetchTree foo).outPath + "/foo" would be a path in its own right, and accessing it would only mean copyting the /foo subdirectory into the store.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context

This is already implemented in the lazy trees branch, would have to be backported to master.

Priorities

Add 👍 to issues you find important.

@thufschmitt thufschmitt added fetching Networking with the outside (non-Nix) world, input locking feature Feature request or proposal labels Feb 29, 2024
@thufschmitt thufschmitt added this to the fetchTree stabilisation milestone Feb 29, 2024
@thufschmitt thufschmitt moved this from Accepted work to Implementation in Nix implementation board Feb 29, 2024
@edolstra
Copy link
Member

edolstra commented Mar 5, 2024

It turns out that this is not really possible without lazy trees, since it causes an undesirable double copying of source trees.

For instance, on master we currently have this behaviour:

$ nix eval --impure --expr '(builtins.fetchTree "github:NixOS/patchelf").outPath'
"/nix/store/wjb45dlgycjw8759q43js031yjn5l0g5-source"

$ nix eval --impure --expr '"${(builtins.fetchTree "github:NixOS/patchelf")}"'
"/nix/store/wjb45dlgycjw8759q43js031yjn5l0g5-source"

But if fetchTree returns a path value (instead of string with context), we get this behaviour:

$ nix eval --impure --expr '(builtins.fetchTree "github:NixOS/patchelf").outPath'
/nix/store/wjb45dlgycjw8759q43js031yjn5l0g5-source

$ nix eval --impure --expr '"${(builtins.fetchTree "github:NixOS/patchelf")}"'
"/nix/store/03g2lyjysgl3j60gqxdsiqsv7yqlvk0s-wjb45dlgycjw8759q43js031yjn5l0g5-source"

This is because EvalState::copyPathToStore() sees a path with basename wjb45dlgycjw8759q43js031yjn5l0g5-source instead of source.

On the lazy-trees branch, we don't get this double copy:

$ nix eval --impure --expr '(builtins.fetchTree "github:NixOS/patchelf").outPath'
«github:NixOS/patchelf/7c2f768bf9601268a4e71c2ebe91e2011918a70f»/

$ nix eval --impure --expr '"${(builtins.fetchTree "github:NixOS/patchelf")}"'
"/nix/store/wjb45dlgycjw8759q43js031yjn5l0g5-source"

since fetchTree returns a root directory, which is treated equivalent to having a basename of source.

@roberth
Copy link
Member

roberth commented Mar 6, 2024

I see. I've underestimated the amount of lazy-trees behavior that is needed.

I think it's worth exploring a less lazy lazy-trees that doesn't have impurities and is more compatible with existing code, at the cost of doing fetchToStore when misused. For instance toString on such a path value should perform fetchToStore in order to return something deterministic and usable. It's still a really good optimization that way.

@edolstra
Copy link
Member

edolstra commented Mar 8, 2024

I think the easiest solution is to keep the current behaviour (i.e. fetchTree returns a store path), and in the future, let the user specify lazy behaviour by passing a lazy = true argument to fetchTree.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2024-03-11-nix-team-meeting-132/42960/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature request or proposal fetching Networking with the outside (non-Nix) world, input locking
Projects
Status: Implementation
4 participants