fix the hash rewriting for ca-derivations by thufschmitt · Pull Request #4282 · NixOS/nix

thufschmitt · 2020-11-25T10:42:25Z

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).

thufschmitt · 2020-11-25T10:42:57Z

/cc @Ericson2314

edolstra · 2020-11-26T13:13:07Z

src/libstore/build/derivation-goal.cc

+                Path tmpPath = actualPath + ".tmp";
+                restorePath(tmpPath, *source);
+                deletePath(actualPath);
+                movePath(tmpPath, actualPath);


Maybe a call to canonicalisePathMetaData() is needed here. (See rewriteOutput() which does so after doing a rewrite.)

Indeed, thanks

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() }, });

Duh indeed. Thanks, should be fixed now

Ericson2314 · 2020-11-26T17:02:02Z

Wait isn't

nix/src/libstore/build/derivation-goal.cc

Lines 1270 to 1272 in 13c557f

    
                   /* Substitute output placeholders with the scratch output paths. 
        
                      We'll use during the build. */ 
        
                   inputRewrites[hashPlaceholder(outputName)] = worker.store.printStorePath(scratchPath);

supposed to do it?

thufschmitt · 2020-11-27T14:44:51Z

Wait isn't

nix/src/libstore/build/derivation-goal.cc

Lines 1270 to 1272 in 13c557f

/* Substitute output placeholders with the scratch output paths.

We'll use during the build. */

inputRewrites[hashPlaceholder(outputName)] = worker.store.printStorePath(scratchPath);

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

Ericson2314 · 2020-11-27T21:56:54Z

@regnat sorry, here's the output one

nix/src/libstore/build/derivation-goal.cc

Line 3015 in 05d9442

    
           outputRewrites[std::string { scratchPath.hashPart() }] = std::string { finalStorePath.hashPart() };

But I don't think the finish one is being called in enough places?

thufschmitt · 2020-11-30T08:36:36Z

@regnat sorry, here's the output one

nix/src/libstore/build/derivation-goal.cc

Line 3015 in 05d9442

outputRewrites[std::string { scratchPath.hashPart() }] = std::string { finalStorePath.hashPart() };

But I don't think the finish one is being called in enough places?

I think it is, but it's called after rewriteOutput() (and needs to be, because getting the final hash can only happen after we've rewritten the hashes of the dependencies). We could just call rewriteOutput() (once for rewriting the dependencies and once for rewriting the self-references) that should be equivalent − except that we'd need to make sure that we only warn once about the hash rewriting.

Ericson2314 · 2020-11-30T17:07:37Z

src/libstore/build/derivation-goal.cc

+                rewriteOutput({{
+                    std::string(scratchPath.hashPart()),
+                    std::string(newInfo0.path.hashPart())
+                }});


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?

Suggested change

rewriteOutput({{

std::string(scratchPath.hashPart()),

std::string(newInfo0.path.hashPart())

}});

rewriteOutput({

{ std::string(scratchPath.hashPart()), std::string(newInfo0.path.hashPart()) }

});

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

Wait, how does that work, isn't StringMap the outer set of braces?

(Fine with the idea though!)

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 🥴

Ericson2314 · 2020-11-30T17:14:52Z

src/libstore/build/derivation-goal.cc

+            if (scratchPath != newInfo0.path) {
+                // Also rewrite the output path


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 :).

Suggested change

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.

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).

Looks good to me!

Ericson2314

Thanks for catching this mistake of mine @regnat :)

Ericson2314 · 2020-12-01T21:49:53Z

See #4284 (comment), I guess this one can just be the code de-dup.

Ericson2314 · 2020-12-16T21:19:06Z

Still keen on this one, lest the rewrite logic drift apart.

edolstra · 2021-06-23T09:34:33Z

@regnat I guess this is still needed? If so, could you resolve the merge conflict please?

Ericson2314 · 2021-10-01T17:38:34Z

It would be good to get this in before 2.4 too, I think.

stale · 2022-04-16T01:26:29Z

I marked this as stale due to inactivity. → More info

Ericson2314 · 2023-03-11T23:03:34Z

@thufschmitt I rebased this https://github.com/Ericson2314/nix/commits/fix-ca-hash-rewriting it would be good to get it merged!

thufschmitt · 2023-03-17T14:51:40Z

Thanks a lot @Ericson2314 ! I've re-rebased them and force-pushed

Ericson2314 · 2023-04-17T23:43:29Z

Weird this failed, but I rebased it yet again.

thufschmitt · 2023-04-18T06:04:12Z

The darwin CI failure looks quite legit and related:

nix-tests> error: ca hash mismatch importing path '/build/nix-test/tests/fetchClosure/store/y0af26shqb7ag6jplsjffw3lkbq4391k-dependencies-top';
nix-tests> specified: sha256:1k6givbgikzbx4a399vn6y5lvnysa2iw2gfm71fmihqkaqdspdd6
nix-tests> got: sha256:0jdn54c8rcjpv1y5wn1rwqfrfrxli4m2lgiba8qidahmi6mr3slv

Can you have another look?

Ericson2314 · 2023-04-19T16:33:32Z

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.

thufschmitt · 2023-04-19T19:47:43Z

Mh sorry, my brain broke down, ignore my last comment. Thanks for the rebase, I'll look at the failing test :)

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

thufschmitt · 2023-05-25T07:45:15Z

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)

Ericson2314

Sorry for the belated review!

nixos-discourse · 2023-06-29T13:37:19Z

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

nixos-discourse · 2023-06-29T15:35:25Z

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

thufschmitt requested a review from edolstra November 25, 2020 10:42

thufschmitt added the ca-derivations Derivations with content addressed outputs label Nov 25, 2020

edolstra reviewed Nov 26, 2020

View reviewed changes

thufschmitt mentioned this pull request Nov 26, 2020

Minimal implementation of a drv outputs copy #4225

Closed

thufschmitt force-pushed the fix-ca-hash-rewriting branch from 790843e to fcbbf07 Compare November 27, 2020 14:49

thufschmitt force-pushed the fix-ca-hash-rewriting branch 2 times, most recently from 836d457 to 9715a41 Compare November 30, 2020 16:43

Ericson2314 reviewed Nov 30, 2020

View reviewed changes

thufschmitt force-pushed the fix-ca-hash-rewriting branch from 02ecd14 to 24dd5ad Compare November 30, 2020 19:09

Ericson2314 approved these changes Nov 30, 2020

View reviewed changes

thufschmitt force-pushed the fix-ca-hash-rewriting branch from 24dd5ad to f3bbfbf Compare December 1, 2020 09:41

Ericson2314 mentioned this pull request Dec 1, 2020

Allow fixed-output derivations to depend on (floating) content-addressed ones #4284

Merged

thufschmitt mentioned this pull request Dec 3, 2020

Properly test the early cutoff for CA derivations #4308

Merged

thufschmitt added this to the ca-derivations-mvp milestone Dec 11, 2020

thufschmitt removed this from the ca-derivations-mvp milestone May 5, 2021

thufschmitt force-pushed the fix-ca-hash-rewriting branch from f3bbfbf to 96a14d5 Compare May 10, 2021 14:42

Ericson2314 approved these changes May 10, 2021

View reviewed changes

thufschmitt force-pushed the fix-ca-hash-rewriting branch from 96a14d5 to 073a9ae Compare June 23, 2021 12:31

stale bot added the stale label Apr 16, 2022

Ericson2314 added the bug label Mar 11, 2023

stale bot removed the stale label Mar 11, 2023

thufschmitt force-pushed the fix-ca-hash-rewriting branch from 073a9ae to a5c970a Compare March 17, 2023 14:51

Ericson2314 self-assigned this Apr 17, 2023

Ericson2314 mentioned this pull request Apr 17, 2023

ca-derivations can create malformed NAR serializations #8113

Open

thufschmitt force-pushed the fix-ca-hash-rewriting branch from a5c970a to f24c182 Compare May 10, 2023 12:14

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

Théophane Hufschmitt added 4 commits May 24, 2023 14:11

Make RewritingSink accept a map of rewrites

3ebe134

Giving it the same semantics as `rewriteStrings`. Also add some tests for it

Use a RewritingSink in derivation goal

a917fb0

Possibly this will make it stream

Normalize the hash-rewriting process when building derivations

34e1b46

Disable the fetchClosure test for old daemons

d0cecbe

Broken because of the change introduced by NixOS#4282

thufschmitt force-pushed the fix-ca-hash-rewriting branch from f24c182 to d0cecbe Compare May 24, 2023 13:36

thufschmitt requested a review from Ericson2314 June 1, 2023 07:52

Ericson2314 approved these changes Jun 14, 2023

View reviewed changes

Ericson2314 merged commit 61a3e1f into NixOS:master Jun 14, 2023

infinisil mentioned this pull request Nov 20, 2023

CA hash mismatch fetching from a substituter populated by an old nix #9397

Open

		if (scratchPath != newInfo0.path) {
		// Also rewrite the output path

-            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.

Uh oh!

Comments

Conversation

thufschmitt commented Nov 25, 2020

Uh oh!

thufschmitt commented Nov 25, 2020

Uh oh!

edolstra Nov 26, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Nov 30, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Ericson2314 commented Nov 26, 2020

Uh oh!

thufschmitt commented Nov 27, 2020

Uh oh!

Ericson2314 commented Nov 27, 2020

Uh oh!

thufschmitt commented Nov 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Nov 30, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ericson2314 left a comment

Choose a reason for hiding this comment

Uh oh!

Ericson2314 commented Dec 1, 2020

Uh oh!

Ericson2314 commented Dec 16, 2020

Uh oh!

edolstra commented Jun 23, 2021

Uh oh!

Ericson2314 commented Oct 1, 2021

Uh oh!

stale bot commented Apr 16, 2022

Uh oh!

Ericson2314 commented Mar 11, 2023

Uh oh!

thufschmitt commented Mar 17, 2023

Uh oh!

Ericson2314 commented Apr 17, 2023

Uh oh!

thufschmitt commented Apr 18, 2023

Uh oh!

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

Uh oh!

Uh oh!

thufschmitt commented Apr 19, 2023

Uh oh!

thufschmitt commented May 25, 2023

Uh oh!

Ericson2314 left a comment

Choose a reason for hiding this comment

Uh oh!

nixos-discourse commented Jun 29, 2023

Uh oh!

nixos-discourse commented Jun 29, 2023

Uh oh!

Reviewers

Assignees

edolstra Nov 26, 2020 •

edited

Loading

Ericson2314 Nov 30, 2020 •

edited

Loading

Ericson2314 Nov 30, 2020 •

edited

Loading

Ericson2314 commented Apr 19, 2023 •

edited

Loading