Skip to content
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

Overhaul derivation hash modulo somewhat #6229

Merged
merged 1 commit into from
Mar 15, 2022

Conversation

Ericson2314
Copy link
Member

These changes were taken from dynamic derivation (#4628). They somewhat
undo the the refactors I first did for floating CA derivations, as the
benefit of hindsight + requirements of dynamic derivations made me
reconsider some things.

They aren't to consequential, but I figured they might be good to land
first, before the more profound changes @thufschmitt has in the works.

@Ericson2314 Ericson2314 force-pushed the refactor-hash-modulo branch 2 times, most recently from 1575119 to 2217537 Compare March 11, 2022 18:56
This changes was taken from dynamic derivation (NixOS#4628). It` somewhat
undoes the refactors I first did for floating CA derivations, as the
benefit of hindsight + requirements of dynamic derivations made me
reconsider some things.

They aren't to consequential, but I figured they might be good to land
first, before the more profound changes @thufschmitt has in the works.
@Ericson2314
Copy link
Member Author

Ericson2314 commented Mar 11, 2022

@thufschmitt I did rebase your PR on top of Ericson2314@thufschmitt-hash-modulo (and take a few things I liked from it :)), but I didn't add it to this PR because yours changes behavior: the resolution of input drvs no longer "flattens" the outputs of fixed output and and floating output derivations:

Ericson2314@thufschmitt-hash-modulo#diff-f8123e7b61e5f3a5929dd7d336f7ebc98d59a8fa158dda521b62d2db7fb8ce8aL540-R543

https://github.com/Ericson2314/nix/blob/2217537e9cd67eae0e68bf7b6ddee002d5d6d353/src/libstore/derivations.cc#L537-L560

This original version makes each CA output hash its own fake (hash) "drv", with a single "out" output. This mimicks the resolution of CA derivations. Conversely all the input-addressed outputs are kept together.

https://github.com/Ericson2314/nix/blob/28144bc96397015c9637fe04d9d373db943db8c7/src/libstore/derivations.cc#L533-L544

This puts all the output names (i.second) with each output as DRV, so one ends up with sum_i (outputs_i ^ 2) outputs across sum_i (outputs_i) fake (hash) "drvs.


I think the moral of the story is we might need tests with hard-coded derivations, so we can catch when we actually change this stuff by mistake!

@Ericson2314 Ericson2314 changed the title Overhaul derivation hash modulo & placeholders somewhat Overhaul derivation hash modulo somewhat Mar 13, 2022
@thufschmitt
Copy link
Member

the resolution of input drvs no longer "flattens" the outputs of fixed output and and floating output derivations:

Oh thanks, good catch. Actually I think this is plain wrong because it means that only the last output will be taken into account (since it’s a map and they all share the same key).

I think the moral of the story is we might need tests with hard-coded derivations, so we can catch when we actually change this stuff by mistake!

Yes, we do. There’s already a handful of these (one in the testsuite and a hydra job testing that evaluation of the whole of nixpkgs is unchanged), but afaik none of them uses CA derivations

(although in the case at hand it should probably have been caught with any test involving multiple-outputs derivations since we’re effectively erasing some dependencies in that case)

@Ericson2314
Copy link
Member Author

@thufschmitt Sounds good. Are you OK merging this part then?

@thufschmitt thufschmitt merged commit 516a7ac into NixOS:master Mar 15, 2022
@Ericson2314 Ericson2314 deleted the refactor-hash-modulo branch March 15, 2022 20:31
@Ericson2314
Copy link
Member Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants