Stop failing on toolstate changes #64977
Conversation
rust-highfive
added
the
S-waiting-on-review
|
To be clear, the "according to @Manishearth" bit is specifically for Clippy. Other tools may disagree? Clippy breaks often enough that being able to land a partial fix is nice to just prevent things from piling up and getting worse. |
|
Very nice. I would also be fine with e.g. only doing this for rollups if that makes some folks happier but that might also be more complex to implement. |
There was a problem hiding this comment.
I think we need a manual FCP from all maintainers listed in https://github.com/rust-lang/rust/blob/master/src/tools/publish_toolstate.py. Or maybe restrict this to clippy only.
|
I will try to put together a manual FCP comment sometime today. |
Previously, toolstate would always error the build if the tool regressed (or didn't improve to test-pass). This isn't great for rollups, in particular, as it means we basically can't viably rollup tool updates. In most cases, when trying to land a submodule (tool) change, you mostly want to just bump the sub-commit, and don't care too much if that actually fully fixes the tool.
The previous commit changed toolstate to never fail the build (excluding beta week and beta/stable branches). This amends that logic to also fail the PR builder "as if" we were on beta, since a PR author changing a submodule likely does want some indication of failure rather than silent acceptance. Ideally, there'd be some way to provide a "warning" but I don't believe that's possible with current set of checks.
Mark-Simulacrum
force-pushed
the
toolstate-no-fail
branch
from
51b90e9
to
dbf6ac3
Compare
| python2.7 "$CHECK_NOT" "$OS" "$TOOLSTATE_FILE" "_data/latest.json" regressed | ||
| fi | ||
| sed -i "1 a\\ | ||
| sed -i "1 a\\ | ||
| $COMMIT\t$(cat "$TOOLSTATE_FILE") | ||
| " "history/$OS.tsv" |
There was a problem hiding this comment.
Why do we not even do the "changed" test any more? That had nothing to do with regressing or not.
If I read this correctly, this would commit a new toolstate to the repo even for commits that do not change any toolstate. Is this intentional? That would be useful for #60301 but I was under the impression that we deliberately reduced traffic for that repo.
There was a problem hiding this comment.
Oh and also, doesn't this remove the check that tools do not regress in the beta week? We only run that check when submodules change now, instead of on all PRs, it seems.
There was a problem hiding this comment.
Hm, yeah, that might be right. To be honest I'm having a really hard time understanding this code - I thought we'd handled those cases up above but maybe not :)
|
Ping from triage Thank you. |
kennytm
added
S-blocked
|
Blocked on #65000. |
|
Yes, I also need to do some work on this PR -- I'm pretty sure that the current PR diff is not actually correct. I've been meaning to put some time into understanding this but have not managed to allocate a continuous block of time yet :) Blocked is an appropriate status for now though -- I suspect that the FCP is essentially good enough, but we can say this is blocked on me for now. We can also close the PR for now? Not sure. |
|
☔ The latest upstream changes (presumably #65943) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Because the FCP looks close to being accepted, I'll move this to inactive-closed as you suggested. Please reopen when you can continue to work on it, thanks! |
hdhoang
added
S-inactive
r? @kennytm
@Centril, @Manishearth, and I were discussing this on Discord today (there's probably some issues to cc, but I can't find them at a cursory search).
Previously, toolstate would always error the build if the tool regressed (or didn't improve to test-pass). This isn't great for rollups, in particular, as it means we basically can't viably rollup tool updates.
In most cases, when trying to land a submodule (tool) change, you mostly want to just bump the sub-commit, and don't care too much if that actually fully fixes the tool (according to @Manishearth).
This amends that logic to also fail the PR builder "as if" we were on beta, since a PR author changing a submodule likely does want some indication of failure rather than silent acceptance. Ideally, there'd be some way to provide a "warning" but I don't believe that's possible with current set of checks.