Only store hash in DerivationOutput for fixed output derivations #3795
Conversation
We always have a name for BasicDerivation, since we have a derivation store path that has a name.
we don’t need a full storepath for a fixedoutput derivation. So just putting the ingestion method + the hash is sufficient.
|
@regnat This is the preparatory step from #3776 (comment) . I thought this preparatory change would just be "getting the churn out of the way", switching from the |
| std::optional<FixedOutputHash> hash; /* hash used for expected hash computation */ | ||
| }; | ||
|
|
||
| struct DerivationOutputFixed |
There was a problem hiding this comment.
Oh, it just occurred to me I guess we could get rid of this and just use FixedOutputHash directly.
There was a problem hiding this comment.
yeah, but it would be less type safe right? we could do the same with DerivationOutputExtensional and StorePath
There was a problem hiding this comment.
I wouldn't say "less type safe", just a matter of preference. I don't have a preference, just pointing out the possibility for others that might.
src/libstore/derivations.cc
Outdated
| @@ -129,9 +129,11 @@ static DerivationOutput parseDerivationOutput(const Store & store, istringstream | |||
| } | |||
|
|
|||
|
|
|||
| static Derivation parseDerivation(const Store & store, const string & s) | |||
| static Derivation parseDerivation(const Store & store, const string & s, string name) | |||
There was a problem hiding this comment.
Shouldn't we rather read the name from drv.env["name"] to make them more self-contained ? Atm it's technically possible to realize a derivation that doesn't have a name field (or has a name field that's different from the name of the store path), but there's no way to build such a derivation (even manually because it won't be possible to import it into the store)
There was a problem hiding this comment.
@regnat I would consider magic things in env a libexpr convention rather than it's nice for libstore to not be aware of.
Matt and I talked about getting the name from the derivation output paths during parsing, which would use libstore-aware conventions. We didn't do that in this PR cause we could get away with it, but if we need to get rid of this new argument we can do that.
There was a problem hiding this comment.
I would consider magic things in env a libexpr convention rather than it's nice for libstore to not be aware of.
I agree in principle, but given that the drv format is not extensible that's the best way we have to pass meta information about a derivation. And there's quite a bit of precedent about reading information from the env field (preferLocalBuild, __json, requiredSystemFeatures…), so it seems legit to also do that for the name
…on-output-storepath
Thanks @regnat for the great name.
This comment has been minimized.
This comment has been minimized.
matthewbauer
changed the title
matthewbauer
changed the title
This comment has been minimized.
This comment has been minimized.
…n-output-storepath
|
Could you look at the merge conficts? Otherwise LGTM. One possible improvement: it's a bit ugly to have to pass |
…on-output-storepath
Done
Yes that we bugging me too. Happy to do that, would you mind if it was a separate PR? If I do it in this I will just get a bunch of conflicts for #3830, so I rather do it on top of #3830. |
|
@Ericson2314 Sure! |
We just need the hash here because we can calculate the store path from its fixed output hash (with some extra parameters that is). This leads to less data duplication.
To calculate the path, we need a name, a storeDir, a hash, and a hash method. We get the storeDir from the store and the drvName from the BasicDerivation. Previously, BasicDerivation didn't have a name, so had to add this in as well.