`fetchClosure`: input addressed and pure #8370

roberth · 2023-05-19T09:34:35Z

Motivation

Input addressed binary dependencies are not impure, so requiring --impure for them would open users up to actual impurities, which is counterproductive. Instead, we accept the fact that a trusted key may be required, but we do remind expression authors that CA does not require this.

Context

Follow-up to #8090 (comment)

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

agreed on idea
agreed on implementation strategy
tests, as appropriate
- functional tests - tests/**.sh
- unit tests - src/*/tests
- integration tests - tests/nixos/*
documentation in the manual
documentation in the internal API docs
code and comments are self-explanatory
commit message explains why the change was made
new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

src/libexpr/primops/fetchClosure.cc

Ericson2314 · 2023-05-22T11:27:19Z

Last commit I think warrants additional tests.

src/libexpr/primops/fetchClosure.cc

Ericson2314 · 2023-05-22T11:42:34Z

After this PR, I think it would be good to break up the implementation function into multiple functions. We could use a std::variant perhaps to represent multiple cases, and avoid degrees of freedom (e.g. std::optionals) that are only needed during argument processing before all the fields are parsed.

nixos-discourse · 2023-05-25T08:15:36Z

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

https://discourse.nixos.org/t/2023-05-19-nix-team-meeting-minutes-56/28446/1

nixos-discourse · 2023-05-25T08:21:23Z

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

https://discourse.nixos.org/t/2023-05-22-nix-team-meeting-minutes-57/28447/1

roberth · 2023-06-06T10:24:43Z

src/libexpr/primops/fetchClosure.cc

-      If `fromPath` is already content-addressed, or if you are
-      allowing impure evaluation (`--impure`), then `toPath` may be
-      omitted.
+      By rewriting the store paths, you can add the contents to any store without requiring that you or perhaps your users configure any extra trust.


Explanation of trust not entirely in scope for this piece of documentation. New section TBD:

What does it take for input addressed path to be added to the store? #8459

src/libstore/make-content-addressed.hh

src/libexpr/primops/fetchClosure.cc

Ericson2314 · 2023-06-06T23:03:39Z

Code looks great now, but do we need more tests and release notes?

roberth · 2023-06-06T23:55:09Z

Code looks great now, but do we need [...] and release notes?

I did add a little release note

The function builtins.fetchClosure can now fetch input-addressed paths in pure mode.

Does that look ok to you? All I could think of adding is instructions, but I think the linked reference docs should cover that, as well as the error messages.

more tests

The basics are covered, but I'll have another look at it.

fricklerhandwerk

The error mechanism is really good, as it's largely self-explanatory! Lots of suggestions to make the wording as clear and concise as possible, I hope they're straightforward to commit if not contentious.

doc/manual/src/release-notes/rl-next.md

src/libexpr/primops/fetchClosure.cc

fricklerhandwerk · 2023-06-07T10:21:24Z

src/libexpr/primops/fetchClosure.cc

+      **Rewriting a store path**
+
+      This first example fetches `/nix/store/r2jd...` from the specified binary cache,
      and rewrites it into the content-addressed store path
      `/nix/store/ldbh...`.

-      If `fromPath` is already content-addressed, or if you are
-      allowing impure evaluation (`--impure`), then `toPath` may be
-      omitted.
+      By rewriting the store paths, you can add the contents to any store without requiring that you or perhaps your users configure any extra trust.


I'd omit all this. The first example is explained by the line introducing it. Let's not go into recommendations here until we explain trust and that stuff somewhere first. Let's focus on providing the mechanism and showing how it's used.

src/libexpr/primops/fetchClosure.cc

Ericson2314 · 2023-06-07T11:56:32Z

Sorry I missed the release note: I was reviewing too tired :/. For the tests I might be underestimating how good they were before, but it seems now are more systematic about error checking and thus there are a number of failure modes to test?

src/libexpr/primops/fetchClosure.cc

nixos-discourse · 2023-06-09T10:04:30Z

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

https://discourse.nixos.org/t/2023-06-05-nix-team-meeting-minutes-60/28933/1

Ericson2314 · 2023-06-22T17:15:15Z

I'll make this draft and then @roberth can undraft it when I should review it again.

@edolstra

When explicitly requested by the caller, as suggested in the meeting (NixOS#8090 (comment)) > @edolstra: { toPath } vs { fromPath } is too implicit I've opted for the `inputAddressed = true` requirement, because it we did not agree on renaming the path attributes. > @roberth: more explicit > @edolstra: except for the direction; not immediately clear in which direction the rewriting happens This is in fact the most explicit syntax and a bit redundant, which is good, because that redundancy lets us deliver an error message that reminds expression authors that CA provides a better experience to their users.

A single variable is nice and self-contained.

Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>

We should check error messages, so that we know the command fails for the right reason. Alternatively, a mere typo can run the test undetected.

fricklerhandwerk

The other comments about moving the example explanations into the examples still apply.

src/libexpr/primops/fetchClosure.cc

Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>

roberth · 2023-07-07T09:46:13Z

moving the example explanations into the examples

Done. They are now interleaved with the docs that describe those invocations; no more "overview" means less switching between the three "use cases" and less repetition. You can still get the overview by looking at just the headers and/or just the examples.

done

roberth requested a review from edolstra as a code owner May 19, 2023 09:34

roberth added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label May 19, 2023

github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label May 19, 2023

roberth changed the title ~~Fetch closure input addressed~~ fetchClosure: input addressed and pure May 19, 2023

roberth force-pushed the fetchClosure-input-addressed branch from 6542be8 to 3e3ad34 Compare May 22, 2023 09:37

edolstra reviewed May 22, 2023

View reviewed changes

src/libexpr/primops/fetchClosure.cc Outdated Show resolved Hide resolved

Ericson2314 reviewed May 22, 2023

View reviewed changes

src/libexpr/primops/fetchClosure.cc Outdated Show resolved Hide resolved

Ericson2314 reviewed May 22, 2023

View reviewed changes

src/libexpr/primops/fetchClosure.cc Outdated Show resolved Hide resolved

thufschmitt previously requested changes May 22, 2023

View reviewed changes

src/libexpr/primops/fetchClosure.cc Outdated Show resolved Hide resolved

fricklerhandwerk assigned Ericson2314 Jun 5, 2023

roberth force-pushed the fetchClosure-input-addressed branch from 3e3ad34 to 879ebb6 Compare June 5, 2023 14:25

roberth requested a review from fricklerhandwerk as a code owner June 5, 2023 14:25

roberth force-pushed the fetchClosure-input-addressed branch from f4c8ee3 to 3bb5c6c Compare June 5, 2023 16:32

roberth requested a review from Ericson2314 June 6, 2023 10:23

roberth commented Jun 6, 2023

View reviewed changes

Ericson2314 reviewed Jun 6, 2023

View reviewed changes

src/libstore/make-content-addressed.hh Outdated Show resolved Hide resolved

Ericson2314 reviewed Jun 6, 2023

View reviewed changes

src/libexpr/primops/fetchClosure.cc Show resolved Hide resolved

roberth force-pushed the fetchClosure-input-addressed branch from 3bb5c6c to e7053b9 Compare June 6, 2023 23:50

fricklerhandwerk approved these changes Jun 7, 2023

View reviewed changes

Ericson2314 reviewed Jun 7, 2023

View reviewed changes

src/libexpr/primops/fetchClosure.cc Outdated Show resolved Hide resolved

roberth force-pushed the fetchClosure-input-addressed branch 2 times, most recently from e3b22c3 to 1224fbd Compare June 19, 2023 21:25

roberth requested a review from Ericson2314 June 22, 2023 15:04

Ericson2314 marked this pull request as draft June 22, 2023 17:14

roberth marked this pull request as ready for review June 30, 2023 12:35

roberth and others added 13 commits June 30, 2023 18:19

fetchClosure: Factor out attribute hint

0f6d596

fetchClosure: Refactor: rename toCA -> enableRewriting

7e5b6d2

fetchClosure: Always check that inputAddressed matches the result

508aa58

fetchClosure: Disallow toPath for inputAddressed = true

8dca953

makeContentAddressed: Add single path helper

5588863

fetchClosure: Split into three cases

5bdca46

fetchClosure: Refactor: replace enableRewriting

dc79636

A single variable is nice and self-contained.

doc: Typo

32c69e2

doc: Improve fetchClosure documentation

50de11d

fetchClosure: Docs and error message improvements

40052c7

Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>

tests/fetchClosure: Improve coverage of new and some existing flows

1db81f7

tests/signing.sh: Check signature checking error message

fefb947

We should check error messages, so that we know the command fails for the right reason. Alternatively, a mere typo can run the test undetected.

roberth force-pushed the fetchClosure-input-addressed branch from 1224fbd to fefb947 Compare June 30, 2023 16:32

github-actions bot added the tests label Jun 30, 2023

fricklerhandwerk reviewed Jun 30, 2023

View reviewed changes

roberth and others added 2 commits July 7, 2023 11:00

fetchClosure: Apply suggestions from code review

537e8be

Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>

fetchClosure: Interleave the examples in the docs

b4b02d0

signing.sh: Revert test improvement because it fails on GHA + macOS

9fc82de

roberth force-pushed the fetchClosure-input-addressed branch from 940e9c9 to 9fc82de Compare July 7, 2023 13:37

Ericson2314 merged commit 028b26a into NixOS:master Jul 10, 2023

roberth mentioned this pull request Jul 14, 2023

Allow builtins.storePath in pure mode #5868

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`fetchClosure`: input addressed and pure #8370

`fetchClosure`: input addressed and pure #8370

roberth commented May 19, 2023 •

edited by Ericson2314

Loading

Ericson2314 commented May 22, 2023

Ericson2314 commented May 22, 2023

nixos-discourse commented May 25, 2023

nixos-discourse commented May 25, 2023

roberth Jun 6, 2023

Ericson2314 commented Jun 6, 2023

roberth commented Jun 6, 2023

fricklerhandwerk left a comment

fricklerhandwerk Jun 7, 2023

Ericson2314 commented Jun 7, 2023

nixos-discourse commented Jun 9, 2023

Ericson2314 commented Jun 22, 2023

fricklerhandwerk left a comment

roberth commented Jul 7, 2023

fetchClosure: input addressed and pure #8370

fetchClosure: input addressed and pure #8370

Conversation

roberth commented May 19, 2023 • edited by Ericson2314 Loading

Motivation

Context

Checklist for maintainers

Priorities

Ericson2314 commented May 22, 2023

Ericson2314 commented May 22, 2023

nixos-discourse commented May 25, 2023

nixos-discourse commented May 25, 2023

roberth Jun 6, 2023

Choose a reason for hiding this comment

Ericson2314 commented Jun 6, 2023

roberth commented Jun 6, 2023

fricklerhandwerk left a comment

Choose a reason for hiding this comment

fricklerhandwerk Jun 7, 2023

Choose a reason for hiding this comment

Ericson2314 commented Jun 7, 2023

nixos-discourse commented Jun 9, 2023

Ericson2314 commented Jun 22, 2023

fricklerhandwerk left a comment

Choose a reason for hiding this comment

roberth commented Jul 7, 2023

`fetchClosure`: input addressed and pure #8370

`fetchClosure`: input addressed and pure #8370

roberth commented May 19, 2023 •

edited by Ericson2314

Loading