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

hashDerivationModulo: Generalize for multiple fixed ouputs per drv #3424

Merged

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Mar 19, 2020

See documentation in header and comments in implementation for details.

This is actually done in preparation for floating ca derivations, not
multi-output fixed ca derivations, but the distinction doesn't yet
matter.

CC @regnat @edolstra @kmicklas

Copy link
Member

@cole-h cole-h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of English nitpicks, a few dropped characters here and there, and some changes that may not even make sense down below. Feel free to use as little or as many of the suggestions as you deem appropriate.

I'm as yet unfamiliar with this codebase, but saw this pop up in IRC so I figured I'd help in what ways I can. Sorry I can't actually review the code. Please let me know if anything I've done is wrong or otherwise bad form.

src/libstore/derivations.cc Outdated Show resolved Hide resolved
src/libstore/derivations.cc Outdated Show resolved Hide resolved
src/libstore/derivations.cc Show resolved Hide resolved
src/libstore/derivations.cc Outdated Show resolved Hide resolved
// CA derivation's output hashes
std::set justOut = { std::string("out") };
for (auto & output : i.second) {
/* Put each one in with a single "out" output.. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm certainly no C++ wizard, nor am I familiar with this project's style, but is there a reason you use // comments above and /* */ here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just because it was used elsewhere a lot. I would personally always use // too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seemed to be used in multi-line statements mostly.

src/libstore/derivations.hh Outdated Show resolved Hide resolved
src/libstore/derivations.hh Outdated Show resolved Hide resolved
src/libstore/local-store.cc Outdated Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/libstore/derivations.hh Outdated Show resolved Hide resolved
@Ericson2314
Copy link
Member Author

@cole-h Thanks for catching all my types. I guess this is what happens when I bang out some prose just before going to bed.

@Ericson2314 Ericson2314 force-pushed the multi-output-hashDerivationModulo branch 3 times, most recently from cbcdb9e to 17b991d Compare March 19, 2020 14:24
See documentattion in header and comments in implementation for details.

This is actually done in preparation for floating ca derivations, not
multi-output fixed ca derivations, but the distinction doesn't yet
mattter.

Thanks @cole-h for finding and fixing a bunch of typos.
@Ericson2314 Ericson2314 force-pushed the multi-output-hashDerivationModulo branch from 17b991d to f1cf3ab Compare March 19, 2020 14:31
@Ericson2314
Copy link
Member Author

OK fixed the typos (thanks again @cole-h!) and also added a typedef for all the identical new std::variants.

Copy link
Member

@cole-h cole-h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more nitpicks for ya.

(Hooray for artificially inflating the comment count with few-character suggestions!)

src/libstore/derivations.cc Outdated Show resolved Hide resolved
src/libstore/derivations.cc Outdated Show resolved Hide resolved
is fixed-output, however, it takes each output hash and pretends it
is a derivation hash producing a single "out" output. This is so we
don't leak the provenance of fixed outputs, reducing pointless cache
misses as the build itself won't know this.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This remark seems more like an aside.

Suggested change
misses as the build itself won't know this.
misses (as the build itself won't know this).

or

Suggested change
misses as the build itself won't know this.
misses, because the build itself won't know this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me it's a bit more mandatory/essential than an aside, so I think I want to keep it as it is.

src/libstore/derivations.hh Outdated Show resolved Hide resolved
src/libstore/derivations.hh Outdated Show resolved Hide resolved
src/libstore/derivations.hh Outdated Show resolved Hide resolved
src/libstore/derivations.hh Outdated Show resolved Hide resolved
each output to hashes unique up to the outputs' contents.

For regular derivations, it returns a single hash of the derivation
ATerm, after subderivations have been likewise expunged from that
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing needs to be changed here, but what does ATerm mean in this context? If somebody could give a brief description, I would really appreciate that :)

(Or pointing to its documentation or something and telling me to RTFM works too.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hahaha we are in the bowel's of Nix history now. It come from https://strategoxt.org/Stratego/ATermLibrary, which was something Eelco's adviser was working on IIRC. I think the thesis mentions this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look in the git history you can find references to that library. Since Nix is using just a small subset, it has been replaced by its own implementation since.

@edolstra edolstra merged commit 5cb8405 into NixOS:master Aug 5, 2020
@Ericson2314 Ericson2314 deleted the multi-output-hashDerivationModulo branch August 5, 2020 15:40
@Ericson2314 Ericson2314 restored the multi-output-hashDerivationModulo branch August 8, 2020 18:28
@Ericson2314 Ericson2314 deleted the multi-output-hashDerivationModulo branch August 8, 2020 18:29
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

5 participants