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
Can build single CA derivations #3883
Conversation
5f1aa4b
to
4e37ab5
Compare
4e37ab5
to
e913a29
Compare
OK I now rebased away some PRs this PR previously contained that it doesn't actually needed. That means I can also remove the WIP! |
Path is null when not known statically.
…ems/nix into single-ca-drv-build
Forgot to add this hunk!
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 didn't review everything yet (that's a pretty big and dense diff), but looks pretty good overall
src/libstore/remote-store.cc
Outdated
std::optional<StorePath> read(const Store & store, Source & from, Phantom<std::optional<StorePath>> _) | ||
{ | ||
auto s = readString(from); | ||
return s == "" ? std::optional<StorePath> {} : store.parseStorePath(s); |
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.
Would it make sense to add a fallback to the generic std::optional<T>
implementation just in case we want to remove this special-casing in a future version?
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.
Maybe....? The comparability considerations are numerous and it's hard to keep track of them all. I hope when we discuss in person we can try to get a grasp on those.
that's a pretty big and dense diff
If we can get a grasp on those, maybe we can merge #3895 first, and then this PR should be easier to review.
needsMove = false; | ||
} | ||
rewriteOutput(); | ||
/* FIXME optimize and deduplicate with addToStore */ |
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.
More than just addToStore
, this is morally a duplicate of CmdMakeContentAddressable::run
(modulo a couple of details like the fact that the original path isn't a valid path)
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.
Agreed. I just think those details, while conceptually insignificant, will make abstracting over this and CmdMakeContentAddressable::run
harder. I am thinking start with making Store::addToStoreFromDump
references-aware, maybe then merge with Store::addTextToStore
, and only then re-compare with CmdMakeContentAddressable::run
.
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.
Yep', makes sense. That's roughly what I'd done in b959bae (except that I don't use addToStoreFromDump
because I wanted to keep the original pathInfo)
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.
Yeah, I have found the C++ non-trivial to deduplicate too. I think it's better to land the features and then choose the abstractions to make sure we don't abstract the wrong way----the plethora of addToStore
-like functions kindof implies we've already been abstracting suboptimally, so I think a rethink should be very broad and systematic.
src/libstore/build.cc
Outdated
@@ -3950,7 +4246,13 @@ void DerivationGoal::registerOutputs() | |||
outputs, this will fail. */ | |||
{ | |||
ValidPathInfos infos2; | |||
for (auto & i : infos) infos2.push_back(i.second); | |||
for (auto & [outputName, newInfo] : infos) { | |||
/* FIXME: we will want to track this mapping in the DB whether or |
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 might have forgotten something obvious, but is there anything preventing us from calling linkDeriverToPath
without a derivation? (obviously we don't have the drvPath
, but afaik we can recompute it since derivations are content-addressed)
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.
We have a BasicDerivation
not Derivation
in the general case. We don't currently have an unparse
for those, so it's just a matter of bike-shdding whether we want to take the opportunity to switch to JSON or anything else while we're at it.
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.
#3958 shows we don't need a new unparse
, but there still is the question of whether to either:
- relax foreign key constraint, so we can link without file
- write drv file to store so we can link with current foreign key constraint
I have no string preference. My only concern is we might want to keep around any mapping in the build closure of other mappings, which changes GC semantics.
Thanks!! Co-authored-by: Théophane Hufschmitt <regnat@users.noreply.github.com>
Thanks for the idea, @regnat!
Thanks @regnat for catching one of them. The other follows for many of the same reasons. I'm find fixing others on a need-to-fix basis, provided their are no regressions.
It might have changed, and in any event this is how the cod used to work so let's just keep it.
This is much better.
- `queryDerivationOutputMapAssumeTotal` -> `queryPartialDerivationOutputMap` - `queryDerivationOutputMapAssumeTotal` -> `queryDerivationOutputMap
Insead they should be opaque `/<hash>` like the placeholders we already have.
Co-authored-by: Théophane Hufschmitt <regnat@users.noreply.github.com>
Co-authored-by: Théophane Hufschmitt <regnat@users.noreply.github.com>
Thanks! Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
- More and better comments - The easier renames
Sorry, I must be doing something silly, but I really don't see it. Just ones defined elsewhere.
I agree it should be part of So I think we fix this right away! |
My double-bad, it's
That's a fair point. Actually both |
I've implemented it in 0b5f339 if you want to cherry-pick it |
@regnat What branch is that commit on? I'd love to cherry-pick it, but I'm having trouble fetching it! Alternatively, if you want to make a PR with it (since I don't think it depends on any of this?) I'll make this PR depend on that PR. |
@regnat see my previous comment? |
Otherwise, we will associate fixed-output derivations with outputs that they did indeed produce, but which had the wrong hash. That's no good.
@edolstra it looks like this was accidentally closed because GitHub got excited about the word "fix". Can you repopen it? |
We no longer need the `*Opt` to disambiguate.
Thanks, merged! |
Recently a few internal APIs have changed[1]. The `outputPaths` function has been removed and a lot of data structures are modeled with `std::optional` which broke compilation. This patch updates the code in `hydra-queue-runner` accordingly to make sure that Hydra compiles again. [1] NixOS/nix#3883
Featurewise, we're testing little more than what #3740 can do, but a behind the scenes there is a lot of infra changes in place to make this robust:
Via Mischellaneous changes to prepare for CA drvs -- contains #3807, #3424, and #3418 #3830 the legacy style build hashes are thoroughly excised from the floating CA derivation case--almost everywhere Nix now either handles the case where output paths are not known a priori, or throws an
UnimplementedError
saying it cannont. (I say "almost" because there is one more issue to sort out withsrc/libexpr/get-drvs.hh:getOutPath
.)DerivationGoal::registerOutputs
has been almost completely rewritten. Before it would first calculate content hashes and then references, because we never had references in the CA case. Now, it calculates references and then hashes, so we can do both for CA derivations. This is also integrated with the existing rewriting/relocating we did for--check
and--repair
.The new functionality is ready to go. WIP because of conflicts and because it depends on PRs which still have rough edges.