Skip to content
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

Merged
merged 31 commits into from Sep 16, 2020
Merged

Conversation

@Ericson2314
Copy link
Member

Ericson2314 commented Jul 31, 2020

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 #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 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 Ericson2314 force-pushed the obsidiansystems:single-ca-drv-build branch from 5f1aa4b to 4e37ab5 Aug 7, 2020
@Ericson2314 Ericson2314 force-pushed the obsidiansystems:single-ca-drv-build branch from 4e37ab5 to e913a29 Aug 7, 2020
@Ericson2314 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
Copy link
Member Author

Ericson2314 commented Aug 7, 2020

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!

@Ericson2314 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 Aug 8, 2020
Path is null when not known statically.
…ems/nix into single-ca-drv-build
@Ericson2314 Ericson2314 changed the title Can build single CA derivations --- contains #3895 Can build single CA derivations --- contains #3895 and #3914 Aug 10, 2020
@Ericson2314 Ericson2314 changed the title Can build single CA derivations --- contains #3895 and #3914 Can build single CA derivations --- contains #3895 & #3914 Aug 10, 2020
Forgot to add this hunk!
Copy link
Contributor

regnat left a comment

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

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

This comment has been minimized.

@regnat

regnat Aug 11, 2020 Contributor

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?

This comment has been minimized.

@Ericson2314

Ericson2314 Aug 11, 2020 Author Member

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 Outdated Show resolved Hide resolved
src/libstore/build.cc Outdated Show resolved Hide resolved
src/libstore/build.cc Outdated Show resolved Hide resolved
src/libstore/build.cc Outdated Show resolved Hide resolved
src/libstore/build.cc Outdated Show resolved Hide resolved
src/libstore/build.cc Outdated Show resolved Hide resolved
needsMove = false;
}
rewriteOutput();
/* FIXME optimize and deduplicate with addToStore */

This comment has been minimized.

@regnat

regnat Aug 11, 2020 Contributor

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)

This comment has been minimized.

@Ericson2314

Ericson2314 Aug 11, 2020 Author Member

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.

This comment has been minimized.

@regnat

regnat Aug 14, 2020 Contributor

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)

This comment has been minimized.

@Ericson2314

Ericson2314 Sep 1, 2020 Author Member

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.

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

This comment has been minimized.

@regnat

regnat Aug 11, 2020 Contributor

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)

This comment has been minimized.

@Ericson2314

Ericson2314 Aug 11, 2020 Author Member

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.

This comment has been minimized.

@Ericson2314

Ericson2314 Sep 1, 2020 Author Member

#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 Outdated Show resolved Hide resolved
Ericson2314 and others added 4 commits Aug 11, 2020
Thanks!!

Co-authored-by: Théophane Hufschmitt <regnat@users.noreply.github.com>
Thanks for the idea, @regnat!
@Ericson2314 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 Aug 11, 2020
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.
@Ericson2314 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 Aug 20, 2020
 - `queryDerivationOutputMapAssumeTotal` -> `queryPartialDerivationOutputMap`
 - `queryDerivationOutputMapAssumeTotal` -> `queryDerivationOutputMap
Insead they should be opaque `/<hash>` like the placeholders we already
have.
@Ericson2314 Ericson2314 changed the title Can build single CA derivations --- contains #3859 Can build single CA derivations Aug 28, 2020
@regnat
Copy link
Contributor

regnat commented Sep 4, 2020

I like your plan. What getDerivation is this? I think the protocol should allow fetching the underlying drv file like any other store path, right?

Not right out of the box :

  • There's a store method called getDerivation, but that's not implemented for RemoteStore
  • There's also Store::narFromPath that's also not implented 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
@regnat
Copy link
Contributor

regnat commented Sep 4, 2020

I tried to bootstrap gcc with this branch (without marking anything as CA, just to ensure backwards compat) and uncovered a couple issues:

  1. Fetching the patch of attr failed with a hash mismatch
  2. The build of binutils and gcc (the wrappers around binutils-unwrapped and gcc-unwrapped) fail with
    changing mode of '/tmp/store/nix/store/d1rj0n6y0bysiddjrbsf9niap94vdxmz-binutils-wrapper-2.31.1.drv.chroot/nix/store/qd6nbwmx0hsp8k6z4p8vf55z80f8sf1s-binutils-wrapper-2.31.1-info' to 120777: No such file or directory
    

The first error is a bit fishy because NixOS/nixpkgs@f36899e seems to indicate that the hash error is legit, but running it with my system nix-build doesn't show the error. So I'm not sure what to think about it.

The tests were ran with NIX_PATH set to nixpkgs=https://github.com/nixos/nixpkgs/archive/e9bd19d4d352f45e871cfc3e91f854cb76ff48e1.tar.gz, running ./inst/bin/nix-build '<nixpkgs>' -A gcc --store /tmp/store --option substitute false (where /tmp/store was empty at the begining). To bypass the failures and get the build going I built the failing drvs with my system nix (also with substitution disabled so that I could make sure that the build was expected to suceed) and restarted from that state.

@regnat
Copy link
Contributor

regnat commented Sep 4, 2020

2. The build of `binutils` and `gcc` (the wrappers around `binutils-unwrapped` and `gcc-unwrapped`) fail with
   ```
   changing mode of '/tmp/store/nix/store/d1rj0n6y0bysiddjrbsf9niap94vdxmz-binutils-wrapper-2.31.1.drv.chroot/nix/store/qd6nbwmx0hsp8k6z4p8vf55z80f8sf1s-binutils-wrapper-2.31.1-info' to 120777: No such file or directory
   ```

This is raised at

throw SysError("changing mode of '%1%' to %2$o", actualPath, mode);

It probably mostly happens in a chroot store : The reason is that the file in question is a symlink (to /nix/store/blahblah-binutils-2.31.1-info), and the symlink isn't valid outside of the chroot (but chmod tries to follow it).

The following patch fixes it:

diff --git a/src/libstore/build.cc b/src/libstore/build.cc
index ba28e78c8..e01ce9d62 100644
--- a/src/libstore/build.cc
+++ b/src/libstore/build.cc
@@ -2065,7 +2065,7 @@ void linkOrCopy(const Path & from, const Path & to)
            file (e.g. 32000 of ext3), which is quite possible after a
            'nix-store --optimise'. FIXME: actually, why don't we just
            bind-mount in this case?
-           
+
            It can also fail with EPERM in BeegFS v7 and earlier versions
            which don't allow hard-links to other directories */
         if (errno != EMLINK && errno != EPERM)
@@ -4101,7 +4101,11 @@ void DerivationGoal::registerOutputs()
                     if (lstat(actualPath.c_str(), &st))
                         throw SysError("getting attributes of path '%1%'", actualPath);
                     mode |= 0200;
-                    if (chmod(actualPath.c_str(), mode) == -1)
+                    // Try to change the perms, but only if the file isn't a
+                    // symlink as symlinks permissions are mostly ignored
+                    // and calling `chmod` on it will just forward the call to
+                    // the target of the link
+                    if (!S_ISLNK(st.st_mode) && chmod(actualPath.c_str(), mode) == -1)
                         throw SysError("changing mode of '%1%' to %2$o", actualPath, mode);
                 }
                 if (rename(
tests/content-addressed.nix Outdated Show resolved Hide resolved
@regnat
Copy link
Contributor

regnat commented Sep 4, 2020

Fetching the patch of attr failed with a hash mismatch

This is probably due to 3e3eaa9 (which has been partially reverted in 641c950 but without adding back tarballs.nixos.orgas the default of hashed-mirrors), so nothing to do with this PR

src/libstore/derivations.hh Outdated Show resolved Hide resolved
src/libstore/derivations.hh Show resolved Hide resolved
src/libstore/derivations.hh Show resolved Hide resolved
src/libstore/local-store.hh Outdated Show resolved Hide resolved
src/nix/command.cc Outdated Show resolved Hide resolved
src/libstore/build.cc Outdated Show resolved Hide resolved
src/libstore/build.cc Outdated Show resolved Hide resolved
src/libstore/build.cc Outdated Show resolved Hide resolved
src/libstore/build.cc Outdated Show resolved Hide resolved
src/libstore/build.cc Outdated Show resolved Hide resolved
Ericson2314 and others added 5 commits Sep 4, 2020
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
@Ericson2314
Copy link
Member Author

Ericson2314 commented Sep 4, 2020

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

@regnat
Copy link
Contributor

regnat commented Sep 7, 2020

  • 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)

@regnat
Copy link
Contributor

regnat commented Sep 7, 2020

So I think we fix this right away!

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

@Ericson2314
Copy link
Member Author

Ericson2314 commented Sep 7, 2020

@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
Copy link
Member Author

Ericson2314 commented Sep 14, 2020

@regnat see my previous comment?

@regnat
Copy link
Contributor

regnat commented Sep 15, 2020

@regnat see my previous comment?

Uh sorry, totally missed it.

Here you are: #4014

Ericson2314 added 2 commits Sep 15, 2020
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
Copy link
Member Author

Ericson2314 commented Sep 15, 2020

@edolstra it looks like this was accidentally closed because GitHub got excited about the word "fix". Can you repopen it?

@edolstra edolstra reopened this Sep 15, 2020
@edolstra edolstra merged commit 609a6d6 into NixOS:master Sep 16, 2020
2 checks passed
2 checks passed
tests (ubuntu-latest)
Details
tests (macos-latest)
Details
@edolstra
Copy link
Member

edolstra commented Sep 16, 2020

Thanks, merged!

@Ericson2314 Ericson2314 deleted the obsidiansystems:single-ca-drv-build branch Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.