Closed
Description
Recent PR (#62733) failed due to some issues with updating the rustc-guide submodule. It's not clear exactly the extent of what's wrong, but here are some issues I see:
- I can't find anything that actually runs the rustc-guide tests.
- Running them locally, they fail with bad links.
Error: there are broken links
Caused By: "https://github.com/rust-lang/compiler-team/blob/master/experts/MAP.md" returned 404 Not Found
- Note, this is especially awkward for me to deal with, because it only runs on linux, which is not my native platform.
- The rustc-guide entry was never added to toolstate.
I'll also note that I am concerned that running external link checks on CI will likely be unreliable, and I do not want PRs to be blocked because of it.
cc @mark-i-m
Metadata
Metadata
Assignees
Labels
Type
Projects
Milestone
Relationships
Development
No branches or pull requests
Activity
mark-i-m commentedon Jul 17, 2019
Sorry, CI has been have rate-limiting issues recently. There is supposed to be a cron job that checks the links every day, but issues are masked by the rate-limiting.
I will try to get on this today.
Our CI on the rust-lang/rustc-guide repo runs them for each PR and a CI cron job is supposed to run them nightly. Unfortunately, I don't think we can do much better. Links just break sometimes.
This link broke very recently. Unfortunately, since I haven't had time to look into CI failures, I missed this one. It should be fixed now.
In general, most breakage comes from compiler code changes, so I expect the CI to find them quickly.
Sorry about this. @Michael-F-Bryan would it be possible to get linkcheck to default to reqwest? Or expose such a feature? That would help tremendously.
Huh? @pietroalbini any ideas? (Sorry, I have pinged you a lot recently)
This is the first time such a thing has happened. Also, to my knowledge, it ahould not block a PR but trigger link toolstate. Clearly, I have done something wrong.
pietroalbini commentedon Jul 17, 2019
cc @kennytm for toolstate
ehuss commentedon Jul 17, 2019
My point is, there is no reason for rustc-guide to be in toolstate if it never runs. Without being tested, there's no "state" to add to the toolstate. Tools usually run here to generate that state.
When a tool is added, it has to be manually added to the toolstate (like I did here).
But to be clear, I do not want to add rustc-guide to the toolstate as-is. If it is testing external links, it will likely fail too often. I think that will be too disrupting to update PRs.
mark-i-m commentedon Jul 17, 2019
@ehuss Ah, I guess that is why toolstate is not working properly.
I very much want the guided added to toolstate. It's incredibly hard to discover these sorts of breakage with stumbling over it in other PRs to the guide. This makes it harder to keep the guide up to date and it also makes contribution to the guide more painful.
I'll also add that I don't think it will be a regular problem. It was somewhat unlucky that the compiler-team repo broke at the same time as your PR.
I would ask that we do an experiment: enable toolstate for the guide, and if it breaks another PR, we can disable it.
mark-i-m commentedon Jul 17, 2019
@ehuss If it is any comfort, I enabled github restrictions on rustc-guide that prevent merging PRs/pushing to master without green CI.
ehuss commentedon Jul 17, 2019
That's fair, I'm fine with trying for a while and see how it goes. Also, I'm not advocating removing it from the toolstate (sorry I'm not being clear). I meant that changing it so that it is allowed to fail when the submodule is updated at all times (even week before release). That is, changing
checktools.sh
to allow failures of rustc-guide, even when the submodule is updated. But still report status changes on the website and the issue tracker.Can you take care of the two updates I mentioned above (adding it to the json file, and updating
checktools.sh
to actually test it)?mark-i-m commentedon Jul 17, 2019
Yes, that was the original goal, but clearly, I'm not doing that right. Is this the necessary change? Or do I need to explicitly allow it to fail somewhere? https://github.com/rust-lang/rust/compare/master...mark-i-m:rustc-guide-toolstate-check?expand=1
mark-i-m commentedon Jul 17, 2019
Also, I opened: rust-lang-nursery/rust-toolstate#12
ehuss commentedon Jul 17, 2019
Oh, if that's the goal, there are a few changes I think:
rustc-guide
fromstatus_check
. It's only purpose there is to fail the build if the submodule is updated and the status is nottest-pass
.SIX_WEEK_CYCLE
check. That triggers during the last week, and if any PR causes a tool to regress, it is rejected. We probably don't want that for rustc-guide.I would verify with @kennytm exactly what to do there, but that's how I see it.
mark-i-m commentedon Jul 17, 2019
@ehuss Thanks! I opened #62759
Michael-F-Bryan commentedon Jul 19, 2019
@mark-i-m I'm not quite sure what you mean here. Are you looking for the
follow-web-links
option under Configuration?13 remaining items