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
hashDerivationModulo: Generalize for multiple fixed ouputs per drv #3424
Conversation
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.
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
// 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.. */ |
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'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?
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.
Just because it was used elsewhere a lot. I would personally always use //
too.
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.
It seemed to be used in multi-line statements mostly.
@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. |
cbcdb9e
to
17b991d
Compare
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.
17b991d
to
f1cf3ab
Compare
OK fixed the typos (thanks again @cole-h!) and also added a |
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.
A few more nitpicks for ya.
(Hooray for artificially inflating the comment count with few-character suggestions!)
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. |
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.
This remark seems more like an aside.
misses as the build itself won't know this. | |
misses (as the build itself won't know this). |
or
misses as the build itself won't know this. | |
misses, because the build itself won't know this. |
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.
To me it's a bit more mandatory/essential than an aside, so I think I want to keep it as it is.
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 |
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.
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.)
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.
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.
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 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.
Co-Authored-By: Cole Helbling <cole.e.helbling@outlook.com>
I think this is clearer
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