-
-
Notifications
You must be signed in to change notification settings - Fork 31
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
Conversation
mirror-nixos-branch.pl
Outdated
@@ -81,8 +81,9 @@ sub fetch { | |||
exit 127; | |||
} | |||
if ($? > 0) { | |||
warn("error while executing nix-instantiate ($?).\n"); | |||
exit $?; | |||
my $code = $? >> 8; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated (including with @grahamc's comment).
902e490
to
5591858
Compare
5591858
to
5693ad5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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.
5693ad5
to
c4a61df
Compare
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.