Restore input substitution by edolstra · Pull Request #11701 · NixOS/nix

edolstra · 2024-10-15T19:06:22Z

Motivation

The ability to substitute inputs was removed in #10612 because it was broken: with user-specified inputs containing a narHash attribute, substitution resulted in an input that lacked the attributes returned by the real fetcher (such as lastModified). This could result in a different evaluation result, depending on whether the input was substitutable.

To fix this, we introduce a new input attribute __final. This is for internal use only (specifically, by call-flake.nix) and may be removed in the future. If __final = true, fetching the input cannot change any attributes and we ignore any new attributes returned by the fetcher. For example:

$ nix eval --impure --expr 'builtins.fetchTree { type = "github"; owner = "NixOS"; repo = "patchelf"; rev = "a0f54334df36770b335c051e540ba40afcbf8378"; narHash = "sha256-FSoxTcRZMGHNJh8dNtKOkcUtjhmhU6yQXcZZfUPLhQM="; __final = true; }'
{ narHash = "sha256-FSoxTcRZMGHNJh8dNtKOkcUtjhmhU6yQXcZZfUPLhQM="; outPath = "/nix/store/z1lyf7s6klqvd97027b56lmckm5p9hik-source"; rev = "a0f54334df36770b335c051e540ba40afcbf8378"; shortRev = "a0f5433"; }

Note the absence of the lastModified attribute usually returned by the github fetcher.

We only attempt to substitute inputs that have __final = true. This is implied by lock file entries; we only write a lock file if all its entries are "final".

Context

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

The ability to substitute inputs was removed in NixOS#10612 because it was broken: with user-specified inputs containing a `narHash` attribute, substitution resulted in an input that lacked the attributes returned by the real fetcher (such as `lastModified`). To fix this, we introduce a new input attribute `final`. If `final = true`, fetching the input cannot add or change any attributes. We only attempt to substitute inputs that have `final = true`. This is implied by lock file entries; we only write a lock file if all its entries are "final". The user can specified `final = true` in `fetchTree`, in which case it is their responsibility to ensure that all attributes returned by the fetcher are included in the `fetchTree` call. For example, nix eval --impure --expr 'builtins.fetchTree { type = "github"; owner = "NixOS"; repo = "patchelf"; final = true; narHash = "sha256-FSoxTcRZMGHNJh8dNtKOkcUtjhmhU6yQXcZZfUPLhQM="; }' succeeds in a store path with the specified NAR hash exists or is substitutable, but fails with error: fetching final input '{"final":true,"narHash":"sha256-FSoxTcRZMGHNJh8dNtKOkcUtjhmhU6yQXcZZfUPLhQM=","owner":"NixOS","repo":"patchelf","type":"github"}' resulted in different input '{"final":true,"lastModified":1718457448,"narHash":"sha256-FSoxTcRZMGHNJh8dNtKOkcUtjhmhU6yQXcZZfUPLhQM=","owner":"NixOS","repo":"patchelf","rev":"a0f54334df36770b335c051e540ba40afcbf8378","type":"github"}'

src/libfetchers/fetchers.cc

Co-authored-by: Cole Helbling <cole.e.helbling@outlook.com>

We haven't added the narHash attribute yet at that point. And if the caller uses getAccesor() instead of fetchToStore() (e.g. in `nix registry pin`), the narHash attribute will never be added. This could lead to a mismatch.

…astModified Fixes flake-regressions/tests/DeterminateSystems/fh/0.1.10: error: fetching final input '{"final":true,"narHash":"sha256-0dZpggYjjmWEk+rGixiBHOHuQfLzEzNfrtjSig04s6Q=","rev":"9ccae1754eec0341b640d5705302ac0923d22875","revCount":1618,"type":"tarball","url":"https://api.flakehub.com/f/pinned/nix-community/fenix/0.1.1618%2Brev-9ccae1754eec0341b640d5705302ac0923d22875/018aea4c-03c9-7734-95d5-b84cc8881e3d/source.tar.gz"}' resulted in different input '{"final":true,"lastModified":1696141234,"narHash":"sha256-0dZpggYjjmWEk+rGixiBHOHuQfLzEzNfrtjSig04s6Q=","rev":"9ccae1754eec0341b640d5705302ac0923d22875","revCount":1618,"type":"tarball","url":"https://api.flakehub.com/f/pinned/nix-community/fenix/0.1.1618%2Brev-9ccae1754eec0341b640d5705302ac0923d22875/018aea4c-03c9-7734-95d5-b84cc8881e3d/source.tar.gz"}'

Fixes flake-regressions/tests/DeterminateSystems/eva/0.1.0: error: 'lastModified' attribute mismatch in input 'https://api.flakehub.com/f/pinned/ipetkov/crane/0.14.1/018ac45c-ff5e-7076-b956-d478a0336516/source.tar.gz?narHash=sha256-mnE14re43v3/Jc50Jv0BKPMtEk7FEtDSligP6B5HwlI%3D', expected 1695511445

roberth

I think we're onto something, but I'm not completely certain that this solves the whole problem, and a partial solution will lead to complexity we'll have to maintain indefinitely. Let me explain.

A related problem that's not solved by this feature is the forward compatibility to add new metadata attributes (like lastModified but new) to an input type.
A possible solution to that is to have an attributes = ["lastModified" "fancyNewAttr"]; argument, and then only return the requested attributes.
The default attributes (when unset) could then be compatible with the current behavior.
This way, we only return attributes that are expected by the caller, and final, as used in fetching/substitution becomes a derived property: whether all requested attributes are already specified in the input attrset.

(This PR also needs docs and tests, and while particularly the prior could be a useful way to think about the design, these are of secondary concern to the design question)

src/libfetchers/fetchers.hh

src/libflake/flake/lockfile.cc

src/libfetchers/fetchers.cc

src/libfetchers/fetchers.hh

edolstra · 2024-10-21T13:37:45Z

A related problem that's not solved by this feature is the forward compatibility to add new metadata attributes (like lastModified but new) to an input type.

That's not a big problem: the fetcher can simply not return the new attribute if the input is marked as final. In fact, this PR does just that in the tarball fetcher.

nixos-discourse · 2024-10-23T20:37:55Z

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

https://discourse.nixos.org/t/2024-10-23-nix-team-meeting-minutes-189/54841/1

This fixes the error '{"__final":true,"lastModified":1686592866,"narHash":"sha256-riGg89eWhXJcPNrQGcSwTEEm7CGxWC06oSX44hajeMw","owner":"nixos","repo":"nixpkgs","rev":"0eeebd64de89e4163f4d3cf34ffe925a5cf67a05","type":"github"}' resulted in different input '{"__final":true,"lastModified":1686592866,"narHash":"sha256-riGg89eWhXJcPNrQGcSwTEEm7CGxWC06oSX44hajeMw=","owner":"nixos","repo":"nixpkgs","rev":"0eeebd64de89e4163f4d3cf34ffe925a5cf67a05","type":"github"}' in flake-regressions/tests/nix-community/patsh/0.2.1 (note the lack of a trailing '=' in the NAR hash in the lock file).

edolstra · 2024-11-01T14:42:47Z

Belated team discussion notes from 2 weeks ago:

We'll rename final to __final and leave it undocumented for now. It's only needed internally for call-flake.nix, but having a internal fetchTree builtin that exposes __final only to call-flake.nix was deemed overkill.

We now just check that the fetcher doesn't change any attributes in the input, and return all the original attributes (i.e. discarding any new attributes and keeping any attributes that the fetcher didn't keep).

edolstra · 2024-11-01T15:48:48Z

Simplified the semantics of __final: It now means that nothing is add/removed/changed in a "final" input. So only the attributes in the lock file are passed to the flake.

roberth

Making final a non-Input argument may be a lot cleaner.
Otherwise, some comments.

src/libfetchers/fetchers.hh

src/libflake/flake/lockfile.cc

edolstra · 2024-11-04T16:02:32Z

This passed the full flake regressions test suite.

It's now only allowed in fetchFinalTree, which is not exposed to users but only to call-flake.nix.

nixos-discourse · 2024-11-10T20:35:50Z

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

https://discourse.nixos.org/t/2024-11-06-nix-team-meeting-minutes-192/55847/1

roberth

One more suggestion, but let's move forward.

roberth · 2024-11-10T21:43:10Z

src/libexpr/primops/fetchTree.cc

+static RegisterPrimOp primop_fetchFinalTree({
+    .name = "fetchFinalTree",
+    .args = {"input"},
+    .fun = prim_fetchFinalTree,
+    .internal = true,
+});
+


Suggested change

static RegisterPrimOp primop_fetchFinalTree({

.name = "fetchFinalTree",

.args = {"input"},

.fun = prim_fetchFinalTree,

.internal = true,

});

Couldn't we construct the primop on the fly when loading call-flake.nix?
That way we don't need internal and internalPrimOps.

That seemed tricky since this happens in libfetcher rather than libexpr, so it's not clear how to construct a singleton value. Or we could allocate a new PrimOp every time, but that would leak.

nixos-discourse · 2025-03-07T19:43:07Z

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

https://discourse.nixos.org/t/nix-copying-a-store-path-into-the-store/60409/10

github-actions bot added the fetching Networking with the outside (non-Nix) world, input locking label Oct 15, 2024

edolstra mentioned this pull request Oct 15, 2024

Input::fetchToStore(): Don't try to substitute #10612

Merged

cole-h reviewed Oct 15, 2024

View reviewed changes

src/libfetchers/fetchers.cc Outdated Show resolved Hide resolved

edolstra and others added 3 commits October 16, 2024 15:17

Typo

fc09815

Co-authored-by: Cole Helbling <cole.e.helbling@outlook.com>

Don't mark inputs as final in getAccessorUnchecked()

ed1f9dd

We haven't added the narHash attribute yet at that point. And if the caller uses getAccesor() instead of fetchToStore() (e.g. in `nix registry pin`), the narHash attribute will never be added. This could lead to a mismatch.

edolstra marked this pull request as ready for review October 17, 2024 12:13

roberth suggested changes Oct 21, 2024

View reviewed changes

roberth added this to the nix-2.25 milestone Oct 23, 2024

edolstra added 3 commits October 30, 2024 20:54

Rename final -> __final

a7a0767

Merge remote-tracking branch 'origin/master' into flake-substitution

4fcd458

Simplify "final" inputs

f314e35

We now just check that the fetcher doesn't change any attributes in the input, and return all the original attributes (i.e. discarding any new attributes and keeping any attributes that the fetcher didn't keep).

edolstra requested a review from roberth November 1, 2024 15:43

roberth reviewed Nov 3, 2024

View reviewed changes

src/libfetchers/fetchers.hh Outdated Show resolved Hide resolved

src/libfetchers/fetchers.hh Show resolved Hide resolved

src/libflake/flake/lockfile.cc Show resolved Hide resolved

Document "final" semantics

a150798

edolstra requested a review from fricklerhandwerk as a code owner November 6, 2024 12:06

github-actions bot added documentation new-cli Relating to the "nix" command labels Nov 6, 2024

edolstra added 2 commits November 6, 2024 13:12

More docs

0401e27

Rename argument "final" to "result" to avoid ambiguity

b7882d5

edolstra requested a review from roberth November 7, 2024 12:54

Merge remote-tracking branch 'origin/master' into flake-substitution

ecb418e

edolstra added 2 commits November 8, 2024 17:31

Remove no longer needed hack

d90b56d

Don't allow __final in fetchTree

4dceca5

It's now only allowed in fetchFinalTree, which is not exposed to users but only to call-flake.nix.

roberth approved these changes Nov 10, 2024

View reviewed changes

edolstra added 2 commits November 11, 2024 13:57

Merge remote-tracking branch 'origin/master' into flake-substitution

67a42e6

Remove release note about flake substitution

036359a

edolstra enabled auto-merge November 11, 2024 12:58

edolstra merged commit fa4bd39 into NixOS:master Nov 11, 2024

edolstra deleted the flake-substitution branch November 11, 2024 13:40

Tom-Hubrecht mentioned this pull request Apr 11, 2025

fix: Add lastModified to git sources nikstur/lon#3

Merged

coderabbitai bot mentioned this pull request Dec 17, 2025

fetchTree: Implicitly set __final = true when a narHash is supplied DeterminateSystems/nix-src#297

Merged

Uh oh!

Conversation

edolstra commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Context

Priorities and Process

Uh oh!

Uh oh!

roberth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

edolstra commented Oct 21, 2024

Uh oh!

nixos-discourse commented Oct 23, 2024

Uh oh!

edolstra commented Nov 1, 2024

Uh oh!

edolstra commented Nov 1, 2024

Uh oh!

roberth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

edolstra commented Nov 4, 2024

Uh oh!

nixos-discourse commented Nov 10, 2024

Uh oh!

roberth left a comment

Choose a reason for hiding this comment

Uh oh!

roberth Nov 10, 2024

Choose a reason for hiding this comment

Uh oh!

edolstra Nov 11, 2024

Choose a reason for hiding this comment

Uh oh!

nixos-discourse commented Mar 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

edolstra commented Oct 15, 2024 •

edited

Loading