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

mirror-branch: Always exit with 1 on nix-instantiate error #31

Merged
merged 1 commit into from Jan 3, 2020

Conversation

@samueldr
Copy link
Member

@samueldr samueldr commented Jan 3, 2020

Resolving the values and being fancy is harder than the actual benefits
would give out.

Return all the info we have, and let the pager'd person deal with the
data.

@@ -81,8 +81,9 @@ sub fetch {
exit 127;
}
if ($? > 0) {
warn("error while executing nix-instantiate ($?).\n");
exit $?;
my $code = $? >> 8;

This comment has been minimized.

@gustavderdrache

gustavderdrache Jan 3, 2020

Looks like it's a little bit more complicated - we'll need ($? & 127) ? 127 : ($? >> 8) (parens because I don't remember operator precedence). Processes that are killed via signal leave their signal number in $? & 127, but scripts that exit leave the return code in $? >> 8 - so my suggestion is to use a synthetic 127 exit code for signal-related exits, and propagate the error code otherwise.

At this point, I think I'd suggest just simplifying this block entirely:

$! = 0; # Clear errno to avoid reporting non-fork/exec-related issues
my $d = `NIX_PATH= nix-instantiate --eval -E "builtins.compareVersions (builtins.parseDrvName \\"$curRelease\\").version (builtins.parseDrvName \\"$releaseName\\").version"`;
if ($? != 0) {
  warn "Could not execute nix-instantiate: exit $?; errno $!\n";
  exit 127;
}

This comment has been minimized.

@grahamc

grahamc Jan 3, 2020
Member

+1 on making the exit code static, though probably 1 is better than 127, since 127 feels very specific and somebody seeing exit 1 may correctly assume less thought went in to the code.

This comment has been minimized.

@samueldr

samueldr Jan 3, 2020
Author Member

Updated (including with @grahamc's comment).

@samueldr samueldr force-pushed the samueldr:fix/2020-01-02-mirror-issue-2 branch from 902e490 to 5591858 Jan 3, 2020
@samueldr samueldr changed the title mirror-branch: Exit with actual exit status mirror-branch: Always exit with 127 on nix-instantiate error Jan 3, 2020
@samueldr samueldr force-pushed the samueldr:fix/2020-01-02-mirror-issue-2 branch from 5591858 to 5693ad5 Jan 3, 2020
@samueldr samueldr changed the title mirror-branch: Always exit with 127 on nix-instantiate error mirror-branch: Always exit with 1 on nix-instantiate error Jan 3, 2020
Copy link

@gustavderdrache gustavderdrache left a comment

👍

Resolving the values and being fancy is harder than the actual benefits
would give out.

Return all the info we have, and let the pager'd person deal with the
data.
@samueldr samueldr force-pushed the samueldr:fix/2020-01-02-mirror-issue-2 branch from 5693ad5 to c4a61df Jan 3, 2020
@grahamc grahamc merged commit 579cbfc into NixOS:master Jan 3, 2020
@samueldr samueldr deleted the samueldr:fix/2020-01-02-mirror-issue-2 branch Jan 3, 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.