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

Properly report build errors on chrooted stores #8399

Merged
merged 1 commit into from
May 27, 2023

Conversation

thufschmitt
Copy link
Member

@thufschmitt thufschmitt commented May 25, 2023

Motivation

When encountering a build error, Nix moves the output paths out of the chroot into their final location (for “easier debugging of build failures”). However this was broken for chroot stores as it was moving it to the logical location, not the physical one.

Fix it by moving to the physical (real) location.

Context

Fix #8395

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
  • documentation in the internal API docs
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

When encountering a build error, Nix moves the output paths out of the
chroot into their final location (for “easier debugging of build
failures”). However this was broken for chroot stores as it was moving
it to the _logical_ location, not the _physical_ one.

Fix it by moving to the physical (_real_) location.

Fix NixOS#8395
@Ericson2314
Copy link
Member

Everything looks good....except we need a test :)

@thufschmitt
Copy link
Member Author

Everything looks good....except we need a test :)

Indeed. I left out the test because

  1. It was a bit annoying to write, and I'm lazy
  2. I'd like to first understand whether this move is useful at all, and I couldn't find where it was used. So I'd like to make sure it's the case before spending time writing a potentially useless test

@edolstra edolstra merged commit 61ddfa1 into NixOS:master May 27, 2023
@Ericson2314
Copy link
Member

Actually is this right? Shouldn't it be the fake store dir inside the choot, but the real store dir outside the chroot?

@thufschmitt
Copy link
Member Author

Actually is this right? Shouldn't it be the fake store dir inside the choot, but the real store dir outside the chroot?

Well, for the copy it definitely should be the real store dir that we want

@Ericson2314
Copy link
Member

Right but the source dir I think should continue to be the source dir.

@github-actions
Copy link

Successfully created backport PR for 2.13-maintenance:

@github-actions
Copy link

github-actions bot commented Jun 6, 2023

Backport failed for 2.13-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin 2.13-maintenance
git worktree add -d .worktree/backport-8399-to-2.13-maintenance origin/2.13-maintenance
cd .worktree/backport-8399-to-2.13-maintenance
git checkout -b backport-8399-to-2.13-maintenance
ancref=$(git merge-base f41dd2c306a5986340b04c1635bd674e4a01b78d d16a1994fb6048d4ea48090c5aabafb7ad89c84f)
git cherry-pick -x $ancref..d16a1994fb6048d4ea48090c5aabafb7ad89c84f

@github-actions
Copy link

github-actions bot commented Jun 6, 2023

Successfully created backport PR for 2.15-maintenance:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spurious (unrelated) XDEV errors while building things for which you cannot "bootstrap"
4 participants