InstallableFlake::toDerivedPaths(): Support paths and store paths #7484

edolstra · 2022-12-20T14:10:32Z

This makes nix build work on paths (which will be copied to the store) and store paths (returned as is). E.g. the following flake output attributes can be built using nix build .#foo:

foo = ./src;
foo = self.outPath;
foo = builtins.fetchTarball { ... };
foo = (builtins.fetchTree { .. }).outPath;
foo = builtins.fetchTree { .. } + "/README.md";
foo = builtins.storePath /nix/store/...;

Note that this is potentially risky, e.g.

foo = /.;

will cause Nix to try to copy the entire file system to the store.

What doesn't work yet:

foo = self;
foo = builtins.fetchTree { .. };

because we don't handle attrsets with an outPath attribute in it yet, and

foo = builtins.storePath /nix/store/.../README.md;

since result symlinks have to point to a store path currently (rather than a file inside a store path).

Extracted from the lazy-trees branch. Fixes #7417. This PR also refactors Installables to merge InstallableFlake::toDerivations() into toDerivedPaths(), which simplifies the interface.

toDerivedPaths() now returns DerivedPathWithInfo, which is DerivedPath with some attributes needed by 'nix profile' etc. Preparation for NixOS#7417.

This makes 'nix build' work on paths (which will be copied to the store) and store paths (returned as is). E.g. the following flake output attributes can be built using 'nix build .#foo': foo = ./src; foo = self.outPath; foo = builtins.fetchTarball { ... }; foo = (builtins.fetchTree { .. }).outPath; foo = builtins.fetchTree { .. } + "/README.md"; foo = builtins.storePath /nix/store/...; Note that this is potentially risky, e.g. foo = /.; will cause Nix to try to copy the entire file system to the store. What doesn't work yet: foo = self; foo = builtins.fetchTree { .. }; because we don't handle attrsets with an outPath attribute in it yet, and foo = builtins.storePath /nix/store/.../README.md; since result symlinks have to point to a store path currently (rather than a file inside a store path). Fixes NixOS#7417.

nixos-discourse · 2023-01-03T10:23:06Z

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

https://discourse.nixos.org/t/2022-12-23-nix-team-meeting-minutes-19/24400/1

Ericson2314 · 2023-01-03T17:19:34Z

#7543 This should help us handle the remaining cases.

Ericson2314

I think this PR is an excellent first step, and we should merge it as is, but we should take away the "fixes #7543" because there is more to that issue remaining.

I am happy to do the rest myself :).

thufschmitt

That's a great addition!

src/libcmd/installables.hh

src/libcmd/installables.cc

thufschmitt · 2023-01-04T06:53:14Z

src/nix/build.cc

+            for (auto & i : installables)
+                for (auto & b : i->toDerivedPaths())
+                    pathsToBuild.push_back(b.path);
+


I find that removing the brackets is fairly dangerous, especially for nested loops like that where the body of the outer for already covers several lines

IMHO in C++ there isn't much risk here (unlike e.g. in a dynamically scoped language like Python).

thufschmitt · 2023-01-04T06:55:50Z

src/nix/profile.cc

+        /* Note that there could be conflicting info
+           (e.g. meta.priority fields) if the installable returned
+           multiple derivations. So pick one arbitrarily. */


Should we warn in that case?

I've added a FIXME for this.

thufschmitt · 2023-01-04T06:57:19Z

src/nix/profile.cc


                printInfo("upgrading '%s' from flake '%s' to '%s'",
-                    element.source->attrPath, element.source->resolvedRef, resolvedRef);
+                    element.source->attrPath, element.source->resolvedRef, *info.resolvedRef);

                element.source = ProfileElementSource {


Do we still need ProfileElementSource or could it just be an alias for ExtraInfo?

They largely contain the same info now, but that might not be the case in the future.

tomberek · 2023-01-04T08:10:32Z

Looking to include the more advanced features to support:

attrsets with outPath
derivations without drvPath
derivation without drvPath with multiple outputs with strings
derivation without drvPath with multiple outputs with paths

master...flox:nix:fix-7417 (WIP)

Edit: apologies for the push to the wrong remote

Co-authored-by: Théophane Hufschmitt <7226587+thufschmitt@users.noreply.github.com>

Applied most suggestions.

tomberek · 2023-01-10T16:39:53Z

attrsets with outPath

derivations without drvPath

derivation without drvPath with multiple outputs with strings

derivation without drvPath with multiple outputs with paths

@edolstra would you want the above implemented in a new PR?

Ericson2314 · 2023-01-10T20:12:27Z

tests/flakes/build-paths.sh

+    # FIXME
+    a5 = self;
+
+    a6 = flake2.outPath;


How does this work? This should have an !out!drvPath string context.

As discussed in NixOS#7417, it would be good to make more string values work as installables. That is to say, if an installable refers to a value, and the value is a string, it used to not work at all, since NixOS#7484, it works somewhat, and this PR make it work some more. The new cases that are added for `BuiltPath` contexts: - Fixed input- or content-addressed derivation: ``` nix-repl> hello.out.outPath "/nix/store/jppfl2bp1zhx8sgs2mgifmsx6dv16mv2-hello-2.12" nix-repl> :p builtins.getContext hello.out.outPath { "/nix/store/c7jrxqjhdda93lhbkanqfs07x2bzazbm-hello-2.12.drv" = { outputs = [ "out" ]; }; } The string matches the specified single output of that derivation, so it should also be valid. - Floating content-addressed derivation: ``` nix-repl> (hello.overrideAttrs (_: { __contentAddressed = true; })).out.outPath "/1a08j26xqc0zm8agps8anxpjji410yvsx4pcgyn4bfan1ddkx2g0" nix-repl> :p builtins.getContext (hello.overrideAttrs (_: { __contentAddressed = true; })).out.outPath { "/nix/store/qc645pyf9wl37c6qvqzaqkwsm1gp48al-hello-2.12.drv" = { outputs = [ "out" ]; }; } ``` The string is not a path but a placeholder, however it also matches the context, and because it is a CA derivation we have no better option. This should also be valid. We may also want to think about richer attrset based values (also discussed in that issue and NixOS#6507), but this change "completes" our string-based building blocks, from which the others can be desugared into or at least described/document/taught in terms of. Progress towards NixOS#7417

As discussed in NixOS#7417, it would be good to make more string values work as installables. That is to say, if an installable refers to a value, and the value is a string, it used to not work at all, since NixOS#7484, it works somewhat, and this PR make it work some more. The new cases that are added for `BuiltPath` contexts: - Fixed input- or content-addressed derivation: ``` nix-repl> hello.out.outPath "/nix/store/jppfl2bp1zhx8sgs2mgifmsx6dv16mv2-hello-2.12" nix-repl> :p builtins.getContext hello.out.outPath { "/nix/store/c7jrxqjhdda93lhbkanqfs07x2bzazbm-hello-2.12.drv" = { outputs = [ "out" ]; }; } The string matches the specified single output of that derivation, so it should also be valid. - Floating content-addressed derivation: ``` nix-repl> (hello.overrideAttrs (_: { __contentAddressed = true; })).out.outPath "/1a08j26xqc0zm8agps8anxpjji410yvsx4pcgyn4bfan1ddkx2g0" nix-repl> :p builtins.getContext (hello.overrideAttrs (_: { __contentAddressed = true; })).out.outPath { "/nix/store/qc645pyf9wl37c6qvqzaqkwsm1gp48al-hello-2.12.drv" = { outputs = [ "out" ]; }; } ``` The string is not a path but a placeholder, however it also matches the context, and because it is a CA derivation we have no better option. This should also be valid. We may also want to think about richer attrset based values (also discussed in that issue and NixOS#6507), but this change "completes" our string-based building blocks, from which the others can be desugared into or at least described/document/taught in terms of. Progress towards NixOS#7417 Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>

edolstra added 2 commits December 20, 2022 14:24

Merge toDerivations() into toDerivedPaths()

845fc3f

toDerivedPaths() now returns DerivedPathWithInfo, which is DerivedPath with some attributes needed by 'nix profile' etc. Preparation for NixOS#7417.

EvalState::copyPathToStore(): Return a StorePath

bda8791

edolstra requested a review from thufschmitt as a code owner December 20, 2022 14:10

edolstra force-pushed the fix-7417 branch from 313b52d to 5c97b5a Compare December 20, 2022 14:11

edolstra assigned roberth Dec 23, 2022

edolstra added the feature Feature request or proposal label Dec 23, 2022

Ericson2314 approved these changes Jan 3, 2023

View reviewed changes

thufschmitt previously requested changes Jan 4, 2023

View reviewed changes

tomberek force-pushed the fix-7417 branch from 7c27a0a to 5c97b5a Compare January 4, 2023 07:34

edolstra and others added 6 commits January 10, 2023 14:35

Merge remote-tracking branch 'origin/master' into fix-7417

8e923bf

ExtraInfo -> ExtraPathInfo

b80e4b5

Show string in error message

b4dc68f

Apply suggestions from code review

1123c42

Co-authored-by: Théophane Hufschmitt <7226587+thufschmitt@users.noreply.github.com>

Clean up toDerivedPaths() logic

7f1af27

Add a FIXME

59cc920

edolstra enabled auto-merge January 10, 2023 14:23

edolstra merged commit 1c98daf into NixOS:master Jan 10, 2023

Ericson2314 reviewed Jan 10, 2023

View reviewed changes

Ericson2314 mentioned this pull request Jan 20, 2023

Make more string values work as installables #7601

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

InstallableFlake::toDerivedPaths(): Support paths and store paths #7484

InstallableFlake::toDerivedPaths(): Support paths and store paths #7484

edolstra commented Dec 20, 2022

nixos-discourse commented Jan 3, 2023

Ericson2314 commented Jan 3, 2023

Ericson2314 left a comment •

edited

Loading

thufschmitt left a comment

thufschmitt Jan 4, 2023

edolstra Jan 10, 2023

thufschmitt Jan 4, 2023

edolstra Jan 10, 2023 •

edited

Loading

thufschmitt Jan 4, 2023

edolstra Jan 10, 2023

tomberek commented Jan 4, 2023 •

edited

Loading

tomberek commented Jan 10, 2023

Ericson2314 Jan 10, 2023

InstallableFlake::toDerivedPaths(): Support paths and store paths #7484

InstallableFlake::toDerivedPaths(): Support paths and store paths #7484

Conversation

edolstra commented Dec 20, 2022

nixos-discourse commented Jan 3, 2023

Ericson2314 commented Jan 3, 2023

Ericson2314 left a comment • edited Loading

Choose a reason for hiding this comment

thufschmitt left a comment

Choose a reason for hiding this comment

thufschmitt Jan 4, 2023

Choose a reason for hiding this comment

edolstra Jan 10, 2023

Choose a reason for hiding this comment

thufschmitt Jan 4, 2023

Choose a reason for hiding this comment

edolstra Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

thufschmitt Jan 4, 2023

Choose a reason for hiding this comment

edolstra Jan 10, 2023

Choose a reason for hiding this comment

tomberek commented Jan 4, 2023 • edited Loading

tomberek commented Jan 10, 2023

Ericson2314 Jan 10, 2023

Choose a reason for hiding this comment

Ericson2314 left a comment •

edited

Loading

edolstra Jan 10, 2023 •

edited

Loading

tomberek commented Jan 4, 2023 •

edited

Loading