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
Copy ca derivation outputs #4487
Conversation
159205a
to
f420043
Compare
3c8b629
to
03e02e4
Compare
a7829c7
to
da404b5
Compare
f420043
to
e38bc4e
Compare
🎉 All dependencies have been resolved ! |
abf404b
to
8ac4133
Compare
@@ -783,6 +783,24 @@ void copyStorePath(ref<Store> srcStore, ref<Store> dstStore, | |||
} | |||
|
|||
|
|||
std::map<StorePath, StorePath> copyPaths(ref<Store> srcStore, ref<Store> dstStore, const RealisedPath::Set & paths, | |||
RepairFlag repair, CheckSigsFlag checkSigs, SubstituteFlag substitute) | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
settings.isExperimentalFeatureEnabled("ca-derivations")
somewhere in here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the local side. I guess don't even attempt to copy realizations in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in practice this always hold (because the RealisedPathsCommand
only uses OpaquePath
s if the flag isn't set. But I've added a check here too in 00a30ea just for security
src/libstore/store-api.cc
Outdated
} | ||
auto pathsMap = copyPaths(srcStore, dstStore, storePaths, repair, checkSigs, substitute); | ||
for (auto& realisation : realisations) { | ||
dstStore->registerDrvOutput(realisation); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
separately arrange for this to fail nicely if it isn't enabled remotely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
src/libstore/globals.hh
Outdated
@@ -672,7 +682,7 @@ public: | |||
send a series of commands to modify various settings to stdout. The | |||
currently recognized commands are: | |||
|
|||
- `extra-sandbox-paths` | |||
- `extra-sandbox-paths` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC this trailing whitespace is significant, it causes it to be rendered as a definition list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aaaaaargh thanks. Fixed in a21de51
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
That's a fair point.
|
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 <edolstra@gmail.com>
The experimental feature was by mistake required for `nix copy` to work at oll
75bd078
to
c43f446
Compare
Rebased on top of master |
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 |
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)