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

Fix nix-build --check -K in sandbox w/o root #2688

Merged

Conversation

tollb
Copy link
Contributor

@tollb tollb commented Feb 19, 2019

Temporarily add user-write permission to build directory so that it
can be moved out of the sandbox to the store with a .check suffix.

This is necessary because the build directory has already had its
permissions set read-only, but write permission is required
to update the directory's parent link to move it out of the sandbox.

Updated the related --check "derivation may not be deterministic"
messages to consistently use the real store paths.

Added test for non-root sandbox nix-build --check -K to demonstrate
issue and help prevent regressions.

Copy link
Member

@LnL7 LnL7 left a comment

Choose a reason for hiding this comment

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

I tested something similar to this which fixed the test from #2689.

src/libstore/build.cc Show resolved Hide resolved
@tollb
Copy link
Contributor Author

tollb commented Apr 10, 2020

@LnL7 Thanks for the quick follow-up. That's good news. I'll update and force push the rebased version after local testing.

Temporarily add user-write permission to build directory so that it
can be moved out of the sandbox to the store with a .check suffix.

This is necessary because the build directory has already had its
permissions set read-only, but write permission is required
to update the directory's parent link to move it out of the sandbox.

Updated the related --check "derivation may not be deterministic"
messages to consistently use the real store paths.

Added test for non-root sandbox nix-build --check -K to demonstrate
issue and help prevent regressions.
@tollb tollb force-pushed the fix/build_check_keep_failed_sandbox_perms branch from f654466 to 8132d0a Compare April 10, 2020 21:26
@tollb
Copy link
Contributor Author

tollb commented Apr 10, 2020

@LnL I force-pushed a rebased update. I will open a trivial PR to re-enable the disabled check for Darwin in PR #2689, once this gets merged. Let me know if there is anything else I should do.

@domenkozar
Copy link
Member

@tollb you could re-enable the disabled check here to be sure it's now working.

A test case for correct handling of temporary directory deletion that
was added to check.sh as part of PR NixOS#2689 was initially disabled for
Darwin because of a directory permission issue in PR NixOS#2688.

Now that the issue in PR NixOS#2688 is fixed, this commit enables the test
case for Darwin.
@tollb
Copy link
Contributor Author

tollb commented Apr 10, 2020

@domenkozar Thanks for following-up. I re-enabled the disabled Darwin check (in a second commit) and the automated tests now pass. Please let me know if I need to do anything else.
CC @LnL7

@domenkozar
Copy link
Member

cc @edolstra I'm going to merge this, feel free to revert if you have objections.

@domenkozar domenkozar merged commit ea2148f into NixOS:master Apr 11, 2020
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.

None yet

3 participants