-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix the hash rewriting for ca-derivations #4282
Conversation
/cc @Ericson2314 |
Path tmpPath = actualPath + ".tmp"; | ||
restorePath(tmpPath, *source); | ||
deletePath(actualPath); | ||
movePath(tmpPath, actualPath); |
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.
Maybe a call to canonicalisePathMetaData()
is needed here. (See rewriteOutput()
which does so after doing a rewrite.)
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.
Indeed, thanks
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.
If we give rewriteOutput
a references parameter above, I think we can deduplciate this?
rewriteOutput(outputRewrites);
- calc hahes
-
if (scratchPath != newInfo0.path) rewriteOutput({ std::string { scratchPath.hashPart() }, std::string { newInfo0.path.hashPart() }, });
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.
Duh indeed. Thanks, should be fixed now
Wait isn't nix/src/libstore/build/derivation-goal.cc Lines 1270 to 1272 in 13c557f
supposed to do it? |
This line is a rewriting of the input of the build. What's missing is the final rewrite of the self-references after the build |
790843e
to
fcbbf07
Compare
@regnat sorry, here's the output one nix/src/libstore/build/derivation-goal.cc Line 3015 in 05d9442
But I don't think the finish one is being called in enough places?
|
I think it is, but it's called after |
836d457
to
9715a41
Compare
rewriteOutput({{ | ||
std::string(scratchPath.hashPart()), | ||
std::string(newInfo0.path.hashPart()) | ||
}}); |
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.
Sorry to contradict my own prior suggestion here, but I forgot about the two sets of {..}
, maybe this would be better to visually distinguish input from output?
rewriteOutput({{ | |
std::string(scratchPath.hashPart()), | |
std::string(newInfo0.path.hashPart()) | |
}}); | |
rewriteOutput({ | |
{ std::string(scratchPath.hashPart()), std::string(newInfo0.path.hashPart()) } | |
}); |
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've actually reformated it in yet-another-way 😛 (which makes me wish for #3697 to be a think), with an explicit call to the StringMap
constructor because it's indeed to easy to forget what's going on otherwise
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.
Wait, how does that work, isn't StringMap
the outer set of braces?
(Fine with the idea though!)
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.
Wait, how does that work, isn't StringMap the outer set of braces?
I guess that's why people say that one shouldn't write code too late in the evening 🥴
if (scratchPath != newInfo0.path) { | ||
// Also rewrite the output path |
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.
Ah it just occurred to me, refs.first
is whether we have a self reference, so we can use this to optimize slightly.
I also threw in a comment explaining. Feel free to reword :).
if (scratchPath != newInfo0.path) { | |
// Also rewrite the output path | |
if (refs.first) { | |
// patch has self-references, so should also rewrite them. | |
// Note that this doesn't change the end-goal CA we ca calculated above, since that calculation skips the contents of self-references. | |
// But rather, if we have self-references, relocate from `scratchPath`, and then *don't* do this, | |
// our self-references to `scratchPath` become non-self references, and then the CA is wrong! | |
// Instead we "corrupt" the path at `scratchPath` so when we later relocate it it's thereby uncorrupted, the opposite of the above problem. |
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.
Indeed, res.first
is a nicer option (I kept the schatchPath != newInfo0.path
condition too because I think it's useful to prevent rewriting CA derivations).
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.
Looks good to me!
02ecd14
to
24dd5ad
Compare
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.
Thanks for catching this mistake of mine @regnat :)
24dd5ad
to
f3bbfbf
Compare
See #4284 (comment), I guess this one can just be the code de-dup. |
Still keen on this one, lest the rewrite logic drift apart. |
f3bbfbf
to
96a14d5
Compare
@regnat I guess this is still needed? If so, could you resolve the merge conflict please? |
96a14d5
to
073a9ae
Compare
It would be good to get this in before 2.4 too, I think. |
I marked this as stale due to inactivity. → More info |
@thufschmitt I rebased this https://github.com/Ericson2314/nix/commits/fix-ca-hash-rewriting it would be good to get it merged! |
073a9ae
to
a5c970a
Compare
Thanks a lot @Ericson2314 ! I've re-rebased them and force-pushed |
Weird this failed, but I rebased it yet again. |
The darwin CI failure looks quite legit and related:
Can you have another look? |
Agreed; but I was thinking it might be easier to fix post rebase? If you like I can also just open a fresh PR and take this one over if you are busy. Maybe I can make a pr from this repo so we can both push to it. |
Mh sorry, my brain broke down, ignore my last comment. Thanks for the rebase, I'll look at the failing test :) |
a5c970a
to
f24c182
Compare
Giving it the same semantics as `rewriteStrings`. Also add some tests for it
Possibly this will make it stream
Broken because of the change introduced by NixOS#4282
f24c182
to
d0cecbe
Compare
CI is green now, but I had to disable some test or older daemons because this changes the hash of CA paths with self-references (which is a shame, but also very much needed) |
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.
Sorry for the belated review!
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/tweag-nix-dev-update-50/29793/1 |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/content-addressed-nix-call-for-testers/12881/216 |
Add self-hash rewriting for ca-derivations.
This had apparently been forgotten in #3883, and contrary to what I though there was no test for it (and as it's mostly orthogonal to all the rest I didn't notice it otherwise until now).