Copy ca derivation outputs by thufschmitt · Pull Request #4487 · NixOS/nix

thufschmitt · 2021-01-28T16:38:51Z

Yet another take on #4220

Depends on #4340 and #4372

(the diff includes the one of #4372 because I can only target the PR at one branch)

src/libstore/store-api.cc

dpulls · 2021-02-19T14:48:33Z

🎉 All dependencies have been resolved !

Ericson2314 · 2021-02-19T15:04:25Z

src/libstore/store-api.cc


+std::map<StorePath, StorePath> copyPaths(ref<Store> srcStore, ref<Store> dstStore, const RealisedPath::Set & paths,
+    RepairFlag repair, CheckSigsFlag checkSigs, SubstituteFlag substitute)
+{


settings.isExperimentalFeatureEnabled("ca-derivations") somewhere in here?

For the local side. I guess don't even attempt to copy realizations in that case?

I think in practice this always hold (because the RealisedPathsCommand only uses OpaquePaths if the flag isn't set. But I've added a check here too in 00a30ea just for security

Ericson2314 · 2021-02-19T15:10:23Z

src/libstore/store-api.cc

+    }
+    auto pathsMap = copyPaths(srcStore, dstStore, storePaths, repair, checkSigs, substitute);
+    for (auto& realisation : realisations) {
+        dstStore->registerDrvOutput(realisation);


separately arrange for this to fail nicely if it isn't enabled remotely

Fixed in e1c0bf9

(I made it just log the error because as we can't control the remote side, it might make sense to just copy the output paths without failing at the end)

edolstra · 2021-02-25T15:25:51Z

src/libstore/globals.hh

          currently recognized commands are:

-            - `extra-sandbox-paths`  
+            - `extra-sandbox-paths`


IIRC this trailing whitespace is significant, it causes it to be rendered as a definition list.

Aaaaaargh thanks. Fixed in a21de51

edolstra

LGTM apart from some comments.

I wonder though whether copyPaths should be overloaded like this. Maybe it makes more sense to separate copying paths from copying realisations. There might be cases where you just want to copy a store path closure without registering realisation mappings.

src/libstore/store-api.cc

src/libstore/store-api.hh

src/libstore/globals.hh

thufschmitt · 2021-02-25T16:18:30Z

I wonder though whether copyPaths should be overloaded like this. Maybe it makes more sense to separate copying paths from copying realisations. There might be cases where you just want to copy a store path closure without registering realisation mappings.

That's a fair point.

For copyPaths itself, it's an overload as you say, copyPaths(..., StorePathsSet) still exists (and is used in a number of places). Maybe we could rename the overload as copyRealisations or something like that.
For nix copy, currently the only way to only copy a store path is to run nix copy /nix/store/foobar (as all the other forms of installable will yield a Realisation). Maybe we could add a new OperateOn enum member to operate on realisations rather than just store paths (along with a flag similar to --derivations to change the OperateOn we want to use). If that sounds good I can do that in a follow-up PR

That way we can copy the realisations too (in addition to the store paths themselves)

Rather throw a proper exception, and catch&log it on the client side

This should already hold, but better ensure it for future-proof-nees

Mostly removing useless comments and adding spaces before `&` Co-authored-by: Eelco Dolstra <[email protected]>

The experimental feature was by mistake required for `nix copy` to work at oll

thufschmitt · 2021-02-25T16:33:37Z

Rebased on top of master

nixos-discourse · 2021-03-02T14:29:32Z

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

https://discourse.nixos.org/t/tweag-nix-dev-update-8/11758/1

thufschmitt added this to the ca-derivations-mvp milestone Jan 28, 2021

thufschmitt added ca-derivations Derivations with content addressed outputs feature Feature request or proposal labels Jan 28, 2021

thufschmitt force-pushed the ca/copy-drv-outputs branch 2 times, most recently from 159205a to f420043 Compare January 28, 2021 16:45

thufschmitt mentioned this pull request Jan 28, 2021

Substitute drv outputs #4238

Closed

thufschmitt force-pushed the ca/store-unresolved-outputs branch from 3c8b629 to 03e02e4 Compare February 8, 2021 12:37

thufschmitt force-pushed the ca/store-unresolved-outputs branch from a7829c7 to da404b5 Compare February 16, 2021 07:29

thufschmitt force-pushed the ca/copy-drv-outputs branch from f420043 to e38bc4e Compare February 16, 2021 07:36

Ericson2314 reviewed Feb 19, 2021

View reviewed changes

src/libstore/store-api.cc Show resolved Hide resolved

Base automatically changed from ca/store-unresolved-outputs to master February 19, 2021 14:48

thufschmitt force-pushed the ca/copy-drv-outputs branch 2 times, most recently from abf404b to 8ac4133 Compare February 19, 2021 14:50

thufschmitt requested a review from edolstra February 19, 2021 14:54

Ericson2314 reviewed Feb 19, 2021

View reviewed changes

edolstra reviewed Feb 25, 2021

View reviewed changes

edolstra approved these changes Feb 25, 2021

View reviewed changes

thufschmitt and others added 6 commits February 25, 2021 17:18

Use RealisedPaths in copyPaths

2e19967

That way we can copy the realisations too (in addition to the store paths themselves)

Add a test for the copy of CA paths

aead355

Don't crash when copying realisations to a non-ca remote

f67ff1f

Rather throw a proper exception, and catch&log it on the client side

Ensure that the ca-derivations bit is set when copying realisations

3b76f8f

This should already hold, but better ensure it for future-proof-nees

Apply @edolstra stylistic suggestions

c182aac

Mostly removing useless comments and adding spaces before `&` Co-authored-by: Eelco Dolstra <[email protected]>

Make nix copy work without the ca-derivations flag

c43f446

The experimental feature was by mistake required for `nix copy` to work at oll

thufschmitt force-pushed the ca/copy-drv-outputs branch from 75bd078 to c43f446 Compare February 25, 2021 16:32

edolstra merged commit d280340 into master Feb 26, 2021

edolstra deleted the ca/copy-drv-outputs branch February 26, 2021 11:20

thufschmitt mentioned this pull request Feb 27, 2021

Copy the outputs of ca-derivations with the outputId->outPath mapping #4142

Closed

ldesgoui mentioned this pull request Aug 1, 2021

Write Nix 2.4 release notes #4521

Closed

Uh oh!

Comments

Conversation

thufschmitt commented Jan 28, 2021

Uh oh!

Uh oh!

dpulls bot commented Feb 19, 2021

Uh oh!

Ericson2314 Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Feb 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thufschmitt Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

thufschmitt Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

edolstra Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

thufschmitt Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

edolstra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thufschmitt commented Feb 25, 2021

Uh oh!

thufschmitt commented Feb 25, 2021

Uh oh!

nixos-discourse commented Mar 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Ericson2314 Feb 19, 2021 •

edited

Loading