Also pass unwanted outputs to post-build-hook by yorickvP · Pull Request #7721 · NixOS/nix

yorickvP · 2023-01-31T11:55:57Z

Motivation

#6311 caused only wanted outputs to be passed to the post-build-hook, causing #6960.

Context

Fixes #6960

For a long time, the post-build-hook was passed all built store paths. This changed, causing some breakage.

Add a test to see if unwanted outputs are passed to the post-build-hook.
Collect wanted+unwanted outputs in a separate set and thread them through DerivationGoal::checkPathValidity, DerivationGoal::registerOutputs, DerivationGoal::assertPathValidity.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

agreed on idea
agreed on implementation strategy
tests, as appropriate
- functional tests - tests/**.sh
- unit tests - src/*/tests
- integration tests - tests/nixos/*
documentation in the manual
code and comments are self-explanatory
commit message explains why the change was made
new feature or bug fix: updated release notes

nixos-discourse · 2023-02-03T09:54:07Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-43/25185/1

Ericson2314 · 2023-02-06T17:20:52Z

What does the post-build-hook if the paths are substituted instead of freshly built?

Yes, this fixes a practical problem, but if the semantics of the hook differ based on how the store object was obtained, I feel a bit uncomfortable about that.

ollie-etl · 2023-02-17T10:50:41Z

Is this likely the underlying bug for my observation that only a single output from a multi-output derivation is uploaded to s3 cache?
This used to work.

post-hook is

#!/bin/sh
set -eu
set -f # disable globbing
export IFS=' '
exec nix copy --to "s3://<store-uri>?endpoint=https://storage.googleapis.com" $OUT_PATHS

I'm using nix 2.8.1

yorickvP · 2023-02-17T11:23:15Z

@Ericson2314 the post-build-hook was never called for substituted paths, and doing so wouldn't really fit common usage (uploading everything you build to a cache somewhere).

@ollie-etl the difference comes because, before this bug, a build of a.dev would cause the build hook to be invoked with the paths of a.out, a.dev. (even though building a.out is a side-effect).
With the bug, the post-build hook would only get a.dev and a.out would go unnoticed.

ollie-etl · 2023-02-17T11:53:38Z

@yorickvP Here i'm building a, and a.out is being uploaded, but not a.dev.

Ericson2314 · 2023-02-17T16:08:36Z

@yorickvP BTW check out #6312 and in particular the settings wantedOuputs = Outputs::All { }; I used to do (and Nix originally did). I think that might be a simpler way to achieve this PR too?

yorickvP · 2023-02-22T16:15:09Z

Refactored it quite a bit, moved the wantedOutputs filtering to the end of the pipeline in done().
CA tests fail as @thufschmitt predicted. Will look into it tomorrow.

src/libstore/outputs-spec.hh

yorickvP · 2023-02-24T15:36:46Z

I spent a while debugging the test, but copying only the outputs doesn't seem to be sufficient for the CA test in any case (without my patch aswell).

@thufschmitt do you know if that's correct and supposed to be the case?

out=nix-build ./ca-thing.nix
nix copy --to cache $out
nix copy --from cache -f ca-thing.nix
    error: unable to build with a primary store that isn't a local store; either pass a different '--store' or enable remote builds.

thufschmitt · 2023-02-25T09:46:20Z

@thufschmitt do you know if that's correct and supposed to be the case?

out=nix-build ./ca-thing.nix
nix copy --to cache $out
nix copy --from cache -f ca-thing.nix
    error: unable to build with a primary store that isn't a local store; either pass a different '--store' or enable remote builds.

Yes, it is. It's explained a bit more in https://www.tweag.io/blog/2021-02-17-derivation-outputs-and-output-paths/, but the gist of it is that with CA derivations there's a split between the output path and the information “this Derivation built this output path”. So if you want to be able to fetch “the output of this derivation” then you must copy exactly that (nix copy --to cache -f ca-thing.nix or nix copy --to cache /nix/store/....-ca-thing.drv) and not just the output path

yorickvP · 2023-02-28T09:56:49Z

Okay, in that case this will have to suffice

alyssais · 2023-03-30T19:35:15Z

Any news here?

yorickvP · 2023-04-11T15:18:18Z

@thufschmitt rebased, please review

ollie-etl · 2023-05-03T08:06:00Z

@thufschmitt @yorickvP Is this dead?

thufschmitt

Sorry for taking so long at reaching here.

I'm still a bit sad this can't be a more local change, but it does fix the problem while making the code marginally more sensible. So let's roll!

thufschmitt · 2023-05-03T12:13:17Z

tests/push-to-store-old.sh

That's unrelated to this PR, but this file is weird 🤔

thufschmitt · 2023-05-03T12:20:36Z

@yorickvP can I let you rebase one last time? The conflicts seem mostly benign

fe5509d caused only wanted outputs to be passed to the post-build-hook, which resulted in paths being built without ever going into the hook. This commit adds a (currently failing) test for this.

yorickvP · 2023-05-08T11:00:28Z

Rebased, but the DrvOutputs -> SingleDrvOutputs thing was not entirely trivial. Let's see if the tests pass.

thufschmitt

It's all passing, and looking good 🚀

thufschmitt

It's all passing, and looking good 🚀

yorickvP requested review from edolstra and thufschmitt as code owners January 31, 2023 11:55

yorickvP force-pushed the post-build-hook branch 4 times, most recently from f4aaefb to 0096153 Compare February 22, 2023 16:13

Ericson2314 reviewed Feb 22, 2023

View reviewed changes

src/libstore/outputs-spec.hh Outdated Show resolved Hide resolved

yorickvP force-pushed the post-build-hook branch from 0249983 to afa1b32 Compare February 24, 2023 15:33

yorickvP force-pushed the post-build-hook branch from 69fc5ec to d0ad022 Compare April 11, 2023 15:09

github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Apr 11, 2023

yorickvP force-pushed the post-build-hook branch from d0ad022 to 4620d92 Compare April 11, 2023 15:17

thufschmitt approved these changes May 3, 2023

View reviewed changes

tests/push-to-store-old.sh

Copy link

Member

thufschmitt May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's unrelated to this PR, but this file is weird 🤔

yorickvP added 4 commits May 8, 2023 12:43

tests/post-hook: test to see if all outputs are passed

869fb1a

fe5509d caused only wanted outputs to be passed to the post-build-hook, which resulted in paths being built without ever going into the hook. This commit adds a (currently failing) test for this.

libstore: also pass unwanted outputs to the post-build-hook

2ca2c80

CA: rewrite hashes for all outputs, not just the wanted ones

12685ef

tests: copying only the out paths is not enough information for CA

5e332aa

tests/post-hook: remove TODO and --derivation upload

d1ff33d

yorickvP force-pushed the post-build-hook branch from 4620d92 to d1ff33d Compare May 8, 2023 10:59

thufschmitt approved these changes May 10, 2023

View reviewed changes

thufschmitt merged commit 85ff212 into NixOS:master May 10, 2023

Uh oh!

Conversation

yorickvP commented Jan 31, 2023

Motivation

Context

Checklist for maintainers

Uh oh!

nixos-discourse commented Feb 3, 2023

Uh oh!

Ericson2314 commented Feb 6, 2023

Uh oh!

ollie-etl commented Feb 17, 2023

Uh oh!

yorickvP commented Feb 17, 2023

Uh oh!

ollie-etl commented Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ericson2314 commented Feb 17, 2023

Uh oh!

yorickvP commented Feb 22, 2023

Uh oh!

Uh oh!

yorickvP commented Feb 24, 2023

Uh oh!

thufschmitt commented Feb 25, 2023

Uh oh!

yorickvP commented Feb 28, 2023

Uh oh!

alyssais commented Mar 30, 2023

Uh oh!

yorickvP commented Apr 11, 2023

Uh oh!

ollie-etl commented May 3, 2023

Uh oh!

thufschmitt left a comment

Choose a reason for hiding this comment

Uh oh!

thufschmitt May 3, 2023

Choose a reason for hiding this comment

Uh oh!

thufschmitt commented May 3, 2023

Uh oh!

yorickvP commented May 8, 2023

Uh oh!

thufschmitt left a comment

Choose a reason for hiding this comment

Uh oh!

thufschmitt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ollie-etl commented Feb 17, 2023 •

edited

Loading