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

Simplify the handling of the hash modulo #6268

Merged
merged 1 commit into from Mar 29, 2022

Conversation

thufschmitt
Copy link
Member

Rather than having four different but very similar types of hashes, make
only one, with a tag indicating whether it corresponds to a regular of
deferred derivation.

This implies a slight logical change: The original Nix+multiple-outputs
model assumed only one hash-modulo per derivation. Adding
multiple-outputs CA derivations changed this as these have one
hash-modulo per output. This change is now treating each derivation as
having one hash modulo per output.
This obviously means that we internally loose the guaranty that
all the outputs of input-addressed derivations have the same hash
modulo. But it turns out that it doesn’t matter because there’s nothing
in the code taking advantage of that fact (and it probably shouldn’t
anyways).

The upside is that it is now much easier to work with these hashes, and
we can get rid of a lot of useless std::visit{ overloaded.

@Ericson2314
Copy link
Member

Ericson2314 commented Mar 17, 2022

Mmm first of all, I think (floating) CA derivations has nothing to do with this. CaOutputHashes I think is misnamed by me, because it only is used for fixed output derivations.

It might be good to rename that in a separate commit/PR, and then rework the motivation here.

@Ericson2314
Copy link
Member

Aye, part of my is still partial to the original, because while it is more verbose, the std::variant helps me understand/remember how the two "different" strategies work, and what their invariants are.

Still, I agreed before we could do this that after #6229, and I shouldn't reneg on the deal, so 👍.

@Ericson2314
Copy link
Member

master...Ericson2314:remove-the-variant-in-hashmodulo fixed conflicts again.

@Ericson2314
Copy link
Member

Note that #6227's NoHash conflicts with this, but I don't think we need the NoHash case.

@edolstra
Copy link
Member

@thufschmitt Can you have a look at the merge conflict?

Rather than having four different but very similar types of hashes, make
only one, with a tag indicating whether it corresponds to a regular of
deferred derivation.

This implies a slight logical change: The original Nix+multiple-outputs
model assumed only one hash-modulo per derivation. Adding
multiple-outputs CA derivations changed this as these have one
hash-modulo per output. This change is now treating each derivation as
having one hash modulo per output.
This obviously means that we internally loose the guaranty that
all the outputs of input-addressed derivations have the same hash
modulo. But it turns out that it doesn’t matter because there’s nothing
in the code taking advantage of that fact (and it probably shouldn’t
anyways).

The upside is that it is now much easier to work with these hashes, and
we can get rid of a lot of useless `std::visit{ overloaded`.

Co-authored-by: John Ericson <John.Ericson@Obsidian.Systems>
@thufschmitt thufschmitt force-pushed the remove-the-variant-in-hashmodulo branch from 8aab23c to 390269e Compare March 29, 2022 16:18
@thufschmitt
Copy link
Member Author

@thufschmitt Can you have a look at the merge conflict?

Oh thanks, didn’t notice it. Done

@edolstra edolstra merged commit 03be091 into NixOS:master Mar 29, 2022
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/tweag-nix-dev-update-27/18433/1

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.

None yet

4 participants