hashDerivationModulo: Generalize for multiple fixed ouputs per drv#3424
Conversation
cole-h
left a comment
cole-h
left a comment
There was a problem hiding this comment.
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.
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.
Just because it was used elsewhere a lot. I would personally always use // too.
There was a problem hiding this comment.
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. |
Ericson2314
force-pushed
the
multi-output-hashDerivationModulo
branch
3 times, most recently
from
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.
Ericson2314
force-pushed
the
multi-output-hashDerivationModulo
branch
from
17b991d to
f1cf3ab
Compare
|
OK fixed the typos (thanks again @cole-h!) and also added a |
cole-h
left a comment
cole-h
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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 <[email protected]>
I think this is clearer
5 participants
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