Actually add rustc-guide to toolstate, don't fail builds for the guide #62759
Conversation
rust-highfive
added
the
S-waiting-on-review
| @@ -88,7 +89,6 @@ status_check() { | |||
| # these tools are not required for beta to successfully branch | |||
| check_dispatch $1 nightly miri src/tools/miri | |||
| check_dispatch $1 nightly embedded-book src/doc/embedded-book | |||
| check_dispatch $1 nightly rustc-guide src/doc/rustc-guide | |||
There was a problem hiding this comment.
Put this back. This line only verifies that if you have updated rustc-guide the test must pass, and the nightly argument already excluded it from being checked in the beta branch.
There was a problem hiding this comment.
I feel it would be beneficial to remove it. If I include rustc-guide in an update, but it fails due to some transient error, I would prefer if that didn't fail the build. Generally, I think bad links in rustc-guide should not prevent it from being updated.
There was a problem hiding this comment.
Disagreed.
If it is really a transient failure you could just @bors retry.
If the link is an actual bad link it could simply be updated or removed.
There was a problem hiding this comment.
It takes time to investigate why something failed. It involves loading log files. It involves investigating if it is transient or legitimate. It requires time to remove from a PR, and reporting issues. These are all things I do not want to do. Particularly for rustc-guide, which is not really published from here (I love rustc-guide BTW, don't get me wrong). No other documentation checks external links, and I don't think it is a good idea to gate on it.
If it is enforced, I probably won't include rustc-guide in book updates. Someone else can submit updates for it. I apologize for sounding curt, but I just don't want to spend my time on it.
There was a problem hiding this comment.
If we don't care about whether a link failed why do we add a linkchecker? Ignoring the alarm isn't good.
Besides, not gating on it is even worse for debugging since the maintainer of rustc-guide would now need to dig the log archive to get the error message. While with gating at least the Rust-Log-Analyzer could help you extract the relevant logs. If you don't want to investigate you could just mention the rustc-guide maintainer. Since other books are already gated on doc-tests you do need to investigate the log to find out which book is broken anyway.
Furthermore you seem to be exaggerating the probability of spurious external link failure.
There was a problem hiding this comment.
AFAIK without this line, there's basically no toolstate tracking happening at all, or is there?
The status is saved in the local toolstate file by the call to x.py test at the top of the file (save-toolstates is set in config.toml). The publishing is done at the bottom (commit_toolstate_change).
There was a problem hiding this comment.
So @ehuss @mark-i-m how can we proceed here? Personally I agree with @kennytm and, as @mark-i-m said, "not gating update PRs on this might be less helpful because the status would just remain broken". If a PR updates this submodule and tests still don't pass, surely something is wrong with that PR?
If you have spuriously failing tests, the precedent with RLS is to disable/ignore those tests on your side so that at least the other tests can still do their job.
There was a problem hiding this comment.
As I said, I'm fine adding it back and seeing how it goes. I just don't want to spend much time dealing with failures going forward.
I personally don't think a failure indicates a problem with the PR. It just indicates the problem hasn't been fixed, yet. A PR that updates a broken rustc-guide when it remains broken doesn't make the problem worse, it's already broken. Making me take time to remove it from the PR doesn't magically make it fixed, it just takes my time, with no benefit for anyone.
There was a problem hiding this comment.
They way I see it, allowing tools/doctests to fail at all is a concession to the fact that synchronized updates are hard. But really a failing tool is not an acceptable state. If you are updating a tool and your tests are failing, that should be rejected just like when you are updating rustc and tests are failing.
Making sure tests pass is not a waste of time, it is a basic part of software maintenance.
There was a problem hiding this comment.
As I said, I'm fine adding it back and seeing how it goes. I just don't want to spend much time dealing with failures going forward.
Thank you :) I will add it back. If it fails again, feel free to just drop rustc-guide and I can investigate.
If you have spuriously failing tests, the precedent with RLS is to disable/ignore those tests on your side so that at least the other tests can still do their job.
Yes, this is the approach I have been taking with rust-lang/rustc-dev-guide#388. Also, Michael-F-Bryan re-wrote the linkchecker to take better advantage of caching, and maybe it will have more flexible handling of timeouts and server errors later too, so hopefully spurious failures will be less common in the future too.
|
I opened a PR for this PR at mark-i-m#1, with some comments for stuff I figured out while doing my research for #62266. |
Co-Authored-By: kennytm <kennytm@gmail.com>
Co-Authored-By: Ralf Jung <post@ralfj.de>
Co-Authored-By: Ralf Jung <post@ralfj.de>
|
Ok, I'm now fairly confused about which lines to change in which files, but unfortunately, I don't have time to try to understand how these things work. I'm just going to wait for the discussion to settle and see what the consensus is... |
Co-Authored-By: kennytm <kennytm@gmail.com>
|
Your PR failed (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
It looks like the CI failure is spurious? |
|
I honestly don't know what the state of this is. Is it waiting for me? |
|
I'm waiting for #62759 (review) to be resolved. |
|
@bors r+ |
|
📌 Commit 11a3b74 has been approved by |
bors
added
S-waiting-on-bors
|
@bors retry rolled up. |
Rollup of 4 pull requests Successful merges: - #62550 (Implement RFC 2707 + Parser recovery for range patterns) - #62759 (Actually add rustc-guide to toolstate, don't fail builds for the guide) - #62809 (rustc: Update wasm32 support for LLVM 9) - #62974 (bump crossbeam-epoch dependency) Failed merges: r? @ghost
Rollup of 8 pull requests Successful merges: - #62550 (Implement RFC 2707 + Parser recovery for range patterns) - #62759 (Actually add rustc-guide to toolstate, don't fail builds for the guide) - #62806 (Fix few Clippy warnings) - #62974 (bump crossbeam-epoch dependency) - #63051 (Avoid ICE when referencing desugared local binding in borrow error) - #63061 (In which we constantly improve the Vec(Deque) array PartialEq impls) - #63067 (Add test for issue-50900) - #63071 (Allow rustbot to add `F-*` + `requires-nightly`.) Failed merges: r? @ghost
|
Note: Toolstate status currently isn't working due to an unrelated problem with toolstate publishing, so rustc-guide is currently in test-fail state. I looked at updating it, but it looks like it is still broken. |
cc @ehuss
r? @kennytm