-
Notifications
You must be signed in to change notification settings - Fork 888
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
CI: Enable core dump on Linux #1802
Conversation
I am not sure who would be best to review this, to determine if it's correct and sufficient. @lzutao do you recall who suggested core dumps? |
On discord, |
Are any of @pietroalbini @kennytm or @alexcrichton in a position to review this change to determine if it'd provide the requisite information should travis end up segfaulting one of our tests in the future? |
☔ The latest upstream changes (presumably 7afe6a8) made this pull request unmergeable. Please resolve the merge conflicts. |
Rebased on master. |
I would personally discourage this in the sense that it's make CI configuration very complicated for probably not a lot of reward, does rustup segfault that often on CI? For testing it there's not really any great way to verify visuall that this works so if it's to be merged I'd recommend triggering a segfault in a test and make sure it generates an appropriate log entry. |
@kinnison What do you think about this? Should we keep or close this? |
While I saw a couple of segfaults last week, I've not seen any since, so I think this was a spurious tech failure at Travis and not something we should complicate our CI for right now. Especially since we really shouldn't see any segfaults in rustup code in the general course of PRs. We'll have this to come back to if we decide we need it in the future. Thank you @lzutao for having a go at this in the meantime. |
Based on rust-lang/rust#52333 .
Demo run here: https://travis-ci.com/lzutao/rustup.rs/jobs/195307167#L434