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

Jump to bottom

Fix build --repeat with -K to use real paths #2687

Closed
wants to merge 1 commit into from
Closed

Fix build --repeat with -K to use real paths #2687

wants to merge 1 commit into from

Conversation

tollb
Copy link
Contributor

@tollb tollb commented Feb 19, 2019 •
edited

Use toRealPath() to map store paths correctly for builds with the
--keep-failed (-K) and --repeat options. This fixes an issue where
the rename of a build output with a .check suffix fails due to
having the wrong store paths.

Added test of build --repeat with --keep-failed to demonstrate
issue and prevent regressions.

Fixes #4617

@stale
Copy link

stale bot commented Feb 13, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the stale label Feb 13, 2021
@tollb
Fix build --repeat with -K to use real paths
86173c6 
Use toRealPath() to map store paths correctly for builds with the
--keep-failed (-K) and --repeat options. This fixes an issue where
the rename of a build output with a .check suffix fails due to
having the wrong store paths.

Added test of build --repeat with --keep-failed to demonstrate
issue and prevent regressions.
@tollb tollb force-pushed the fix/build_repeat_use_real_store_paths branch from ee6e736 to 86173c6 Compare February 28, 2021 21:51
@stale stale bot removed the stale label Feb 28, 2021
@tollb
Copy link
Contributor Author

tollb commented Feb 28, 2021

I believe this bug still exists and I have updated the pull request accordingly.

While most users are unlikely to encounter the issue, it seems potentially serious as there is an attempt to delete and rename files at the wrong locations.

cc: @domenkozar @edolstra

@domenkozar
Copy link
Member 

$ nix-build --repeat 2 -K -E 'with (import <nixpkgs> {}); runCommand "ok" {} "echo ${toString builtins.currentTime} > $out"'
these derivations will be built:
  /nix/store/dymlqs923p47aznnqid5v942qlgrj94p-ok.drv
building '/nix/store/dymlqs923p47aznnqid5v942qlgrj94p-ok.drv' (round 1/3)...
building '/nix/store/dymlqs923p47aznnqid5v942qlgrj94p-ok.drv' (round 2/3)...
building '/nix/store/dymlqs923p47aznnqid5v942qlgrj94p-ok.drv' (round 3/3)...
/nix/store/z09dssxzn6y1kj408hzxpbpvw9mmxfc3-ok

@tollb
Copy link
Contributor Author

tollb commented Mar 1, 2021 •
edited

Thanks for following-up. Your example works. The issue surfaces in the environment of tests/linux-sandbox.sh. There, a sandbox/chroot is forced by setting an absolute path in NIX_REMOTE. So, if you create a temporary directory /tmp/my_remote_store and run your example with the NIX_REMOTE environment variable set you should hopefully reproduce the issue, e.g.:

$ mkdir /tmp/my_remote_store
$ NIX_REMOTE=/tmp/my_remote_store nix-build --repeat 2 -K -E 'with (import {}); runCommand "ok" {} "echo ${toString builtins.currentTime} > $out"'

@tollb tollb mentioned this pull request Mar 8, 2021
Open
@stale
Copy link

stale bot commented Sep 19, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the stale label Sep 19, 2021
@Ericson2314
Copy link
Member

In #7099 --repeat was just removed, do we still need this?

@stale stale bot removed the stale label Jan 13, 2023
@thufschmitt
Copy link
Member

In #7099 --repeat was just removed, do we still need this?

I guess no, let's close. Feel free to reopen if there's a need

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-01-20-nix-team-meeting-minutes-25/25432/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
Nix team
Archived in project
Milestone
No milestone
5 participants
@tollb @domenkozar @Ericson2314 @thufschmitt @nixos-discourse