Can build single CA derivations by Ericson2314 · Pull Request #3883 · NixOS/nix

Ericson2314 · 2020-07-31T22:34:55Z

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 with src/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.

Ericson2314 · 2020-08-07T20:10:56Z

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!

thufschmitt

I didn't review everything yet (that's a pretty big and dense diff), but looks pretty good overall

thufschmitt · 2020-08-11T08:00:31Z

src/libstore/remote-store.cc

+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);


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?

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.

src/libstore/build.cc

thufschmitt · 2020-08-11T14:07:03Z

src/libstore/build.cc

-                  needsMove = false;
-              }
+            rewriteOutput();
+            /* FIXME optimize and deduplicate with addToStore */


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)

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.

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)

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.

thufschmitt · 2020-08-11T14:32:53Z

src/libstore/build.cc

        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


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)

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.

#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.

src/libstore/derivations.cc

Thanks!! Co-authored-by: Théophane Hufschmitt <[email protected]>

@regnat

Thanks for the idea, @regnat!

@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.

src/libstore/derivations.hh

src/libstore/local-store.hh

src/nix/command.cc

src/libstore/build.cc

Co-authored-by: Théophane Hufschmitt <[email protected]>

Thanks! Co-authored-by: Eelco Dolstra <[email protected]>

- More and better comments - The easier renames

Ericson2314 · 2020-09-04T16:03:42Z

@regnat

* There's a store method called `getDerivation`, but that's not implemented for `RemoteStore`

Sorry, I must be doing something silly, but I really don't see it. Just ones defined elsewhere.

* There's also `Store::narFromPath` that's also not implemented for `RemoteStore`, **but** the worker protocol has already the logic to handle it (because it's used by `SSHStore`), so we just need to upstream `SSHStore::narFromPath` into `RemoteStore` and it should be good. That's a lot of hops for such a workaround, but hopefully that should work

I agree it should be part of RemoteStore, but this isn't a problem in practice: in the only other extant case, using a Unix domain socket, LocalFSStore is also mixed in as a superclass, and it provides narFromPath.

So I think we fix this right away!

thufschmitt · 2020-09-07T08:51:45Z

There's a store method called getDerivation, but that's not implemented for RemoteStore
Sorry, I must be doing something silly, but I really don't see it. Just ones defined elsewhere.

My double-bad, it's readDerivation and not getDerivation.

I agree it should be part of RemoteStore, but this isn't a problem in practice: in the only other extant case, using a Unix domain socket, LocalFSStore is also mixed in as a superclass, and it provides narFromPath.

So I think we fix this right away!

That's a fair point. Actually both SSHStore and LocalFSStore implement readDerivation (using the default implem that calls out to getFSAccessor), so I think we can directly resort to readDerivation
(and after a second look it's actually trivial to implement it for RemoteStore too without changing the protocol if we want)

thufschmitt · 2020-09-07T09:28:59Z

So I think we fix this right away!

I've implemented it in 0b5f339 if you want to cherry-pick it

Ericson2314 · 2020-09-07T19:21:22Z

@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.

Ericson2314 · 2020-09-14T18:22:11Z

@regnat see my previous comment?

thufschmitt · 2020-09-15T08:38:12Z

@regnat see my previous comment?

Uh sorry, totally missed it.

Here you are: #4014

Otherwise, we will associate fixed-output derivations with outputs that they did indeed produce, but which had the wrong hash. That's no good.

Ericson2314 · 2020-09-15T14:55:37Z

@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.

edolstra · 2020-09-16T10:25:49Z

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

Ericson2314 changed the title ~~Can build single CA derivations --- contains many other PRs~~ WIP: Can build single CA derivations --- contains many other PRs Jul 31, 2020

Ericson2314 force-pushed the single-ca-drv-build branch from 5f1aa4b to 4e37ab5 Compare August 7, 2020 19:42

Squashed get CA derivations building

e913a29

Ericson2314 force-pushed the single-ca-drv-build branch from 4e37ab5 to e913a29 Compare August 7, 2020 20:08

Ericson2314 changed the title ~~WIP: Can build single CA derivations --- contains many other PRs~~ WIP: Can build single CA derivations --- contains #3895 Aug 7, 2020

Ericson2314 changed the title ~~WIP: Can build single CA derivations --- contains #3895~~ Can build single CA derivations --- contains #3895 Aug 7, 2020

Ericson2314 added 2 commits August 8, 2020 15:48

Fix perl FFI for floating ca derivations

f7696c6

Path is null when not known statically.

Merge branch 'small-drv-serialize-cleanup' of github.com:obsidiansyst…

1b5c246

…ems/nix into single-ca-drv-build

Ericson2314 changed the title ~~Can build single CA derivations --- contains #3895~~ Can build single CA derivations --- contains #3895 and #3914 Aug 10, 2020

Ericson2314 changed the title ~~Can build single CA derivations --- contains #3895 and #3914~~ Can build single CA derivations --- contains #3895 & #3914 Aug 10, 2020

Fix error in merge breaking floating CA drvs

2a09026

Forgot to add this hunk!

thufschmitt reviewed Aug 11, 2020

View reviewed changes

Ericson2314 and others added 4 commits August 11, 2020 16:49

Apply suggestions from code review

d0f6e33

Thanks!! Co-authored-by: Théophane Hufschmitt <[email protected]>

Float comment to out describe gaveUpOnSubstitution in general

07e3466

Remove redundant equality check

8a068bd

Clarify outputReferences variable with self-describing type

6d57139

Thanks for the idea, @regnat!

Ericson2314 changed the title ~~Can build single CA derivations --- contains #3895 & #3914~~ Can build single CA derivations --- contains #3895 Aug 11, 2020

Ericson2314 added 6 commits August 11, 2020 22:46

Clarify comment on sandbox and temp fresh paths

4c6aac8

Don't assume a total output map in two places in build.cc

2de2012

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.

Recheck path validity after acquiring lock

18834f7

It might have changed, and in any event this is how the cod used to work so let's just keep it.

Break out lambda so output can be matched just once

5f80aea

This is much better.

Merge remote-tracking branch 'upstream/master' into single-ca-drv-build

3c8b5b6

Work around clang bug

f899a7c

Ericson2314 changed the title ~~Can build single CA derivations --- contains #3895~~ Can build single CA derivations --- contains #3859 Aug 20, 2020

Ericson2314 added 3 commits August 20, 2020 18:14

Rename drv output querying functions, like master

45a2f1b

- `queryDerivationOutputMapAssumeTotal` -> `queryPartialDerivationOutputMap` - `queryDerivationOutputMapAssumeTotal` -> `queryDerivationOutputMap

Merge remote-tracking branch 'upstream/master' into single-ca-drv-build

27a3f82

"Downstream placeholders" should not be store paths

3a7b330

Insead they should be opaque `/<hash>` like the placeholders we already have.

Ericson2314 changed the title ~~Can build single CA derivations --- contains #3859~~ Can build single CA derivations Aug 28, 2020

Merge remote-tracking branch 'upstream/master' into single-ca-drv-build

8017fe7

edolstra approved these changes Sep 4, 2020

View reviewed changes

Ericson2314 and others added 5 commits September 4, 2020 10:29

Update tests/content-addressed.nix

b99062b

Co-authored-by: Théophane Hufschmitt <[email protected]>

Don't chmod symlink before moving outputs around

c9f1ed9

Co-authored-by: Théophane Hufschmitt <[email protected]>

Apply suggestions from code review

e86dd59

Thanks! Co-authored-by: Eelco Dolstra <[email protected]>

Fix some of the issues raised by @edolstra

e9fad30

- More and better comments - The easier renames

Document mkOutputString

5aed6f9

thufschmitt mentioned this pull request Sep 15, 2020

gracefully handle old daemon versions #4014

Merged

edolstra closed this in #4014 Sep 15, 2020

Ericson2314 added 2 commits September 15, 2020 14:17

Merge remote-tracking branch 'upstream/master' into single-ca-drv-build

3ba552b

Don't link deriver until after any delayed exception is thrown

c4bf219

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 reopened this Sep 15, 2020

Ericson2314 added 2 commits September 15, 2020 15:19

Get rid of confusing std::optional<bool> for validity

6387550

Rename Derivation::pathOpt to Derivation::path

3a5cdd7

We no longer need the `*Opt` to disambiguate.

edolstra merged commit 609a6d6 into NixOS:master Sep 16, 2020

Ericson2314 deleted the single-ca-drv-build branch September 16, 2020 14:05

thufschmitt mentioned this pull request Sep 17, 2020

CA derivations that depend on other CA derivations #3958

Merged

Ericson2314 mentioned this pull request Sep 28, 2020

Floating content-addressed derivations #4087

Open

9 tasks

thufschmitt added the ca-derivations Derivations with content addressed outputs label Nov 16, 2020

thufschmitt mentioned this pull request Nov 25, 2020

fix the hash rewriting for ca-derivations #4282

Merged

thufschmitt added this to the ca-derivations-mvp milestone Dec 11, 2020

Ericson2314 mentioned this pull request Feb 14, 2023

Generalize Derivation type to better support unresolved derivations #3446

Draft

Uh oh!

Comments

Conversation

Ericson2314 commented Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ericson2314 commented Aug 7, 2020

Uh oh!

thufschmitt left a comment

Choose a reason for hiding this comment

Uh oh!

thufschmitt Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thufschmitt Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thufschmitt Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

thufschmitt Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ericson2314 commented Sep 4, 2020

Uh oh!

thufschmitt commented Sep 7, 2020

Uh oh!

thufschmitt commented Sep 7, 2020

Uh oh!

Ericson2314 commented Sep 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ericson2314 commented Sep 14, 2020

Uh oh!

thufschmitt commented Sep 15, 2020

Uh oh!

Ericson2314 commented Sep 15, 2020

Uh oh!

edolstra commented Sep 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Ericson2314 commented Jul 31, 2020 •

edited

Loading

Ericson2314 Aug 11, 2020 •

edited

Loading

Ericson2314 Aug 11, 2020 •

edited

Loading

Ericson2314 commented Sep 7, 2020 •

edited

Loading