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

detect timedout builds #206

Merged
merged 3 commits into from
Aug 7, 2018
Merged

detect timedout builds #206

merged 3 commits into from
Aug 7, 2018

Conversation

LnL7
Copy link
Member

@LnL7 LnL7 commented Aug 5, 2018

This should change the error messages for timed out builds to something like.

Timed out, unknown build status on x86_64-linux

I think this is fully backwards compatible, allowing builders to continue sending build results in the old format, while newer versions send more information using the new format. This could be cleaned up once all the infrastructure is updated.

@LnL7 LnL7 requested a review from grahamc August 5, 2018 19:57
LnL7 added 3 commits August 5, 2018 21:57

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Verified

This commit was signed with the committer’s verified signature.
apoelstra Andrew Poelstra
This allows the commentposter to handle other build statuses like a
timeout.  The BuildResult struct still includes success and the
commentposter can still handle messages in the old format for backwards
compatibility.
@LnL7 LnL7 force-pushed the detect-timedout branch from 5acf6e1 to 9d85f62 Compare August 5, 2018 19:57
assert_eq!(lines, vec!["stdout", "stderr", "stdout2", "stderr2"]);
let ret = spawned.wait().unwrap().unwrap().unwrap().success();
Copy link
Member

Choose a reason for hiding this comment

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

😍

},
},
e => BuildStatus::UnexpectedError {
err: format!("failed on interior command {:?}", e),
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it's a problem in practice, but this could potentially expose more information about the builder then before.

self.waiter.join()
.map_err(|_err| io::Error::new(io::ErrorKind::Other, "Couldn't join thread."))
.and_then(|opt| opt.ok_or(io::Error::new(io::ErrorKind::Other, "Thread didn't return an exit status.")))
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not entirely sure these messages are correct.

@grahamc grahamc merged commit 67406a7 into NixOS:released Aug 7, 2018
@LnL7 LnL7 deleted the detect-timedout branch August 8, 2018 16:40
@grahamc grahamc mentioned this pull request Aug 9, 2018
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

2 participants