Skip to content

rustc-guide submodule issues #62739

Closed
Closed
@ehuss

Description

@ehuss
Contributor

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

Activity

added
C-bugCategory: This is a bug.
T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.
T-infraRelevant to the infrastructure team, which will review and decide on the PR/issue.
on Jul 17, 2019
mark-i-m

mark-i-m commented on Jul 17, 2019

@mark-i-m
Member

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.

I can't find anything that actually runs the rustc-guide tests.

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.

Running them locally, they fail with bad links.

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.

Note, this is especially awkward for me to deal with, because it only runs on linux, which is not my native platform.

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.

The rustc-guide entry was never added to toolstate.

Huh? @pietroalbini any ideas? (Sorry, I have pinged you a lot recently)

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.

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

pietroalbini commented on Jul 17, 2019

@pietroalbini
Member

cc @kennytm for toolstate

ehuss

ehuss commented on Jul 17, 2019

@ehuss
ContributorAuthor

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.

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.

Huh? pietroalbini any ideas? (Sorry, I have pinged you a lot recently)

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

mark-i-m commented on Jul 17, 2019

@mark-i-m
Member

@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

mark-i-m commented on Jul 17, 2019

@mark-i-m
Member

@ehuss If it is any comfort, I enabled github restrictions on rustc-guide that prevent merging PRs/pushing to master without green CI.

ehuss

ehuss commented on Jul 17, 2019

@ehuss
ContributorAuthor

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

mark-i-m commented on Jul 17, 2019

@mark-i-m
Member

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.

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

mark-i-m commented on Jul 17, 2019

@mark-i-m
Member
ehuss

ehuss commented on Jul 17, 2019

@ehuss
ContributorAuthor

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?

Oh, if that's the goal, there are a few changes I think:

  • Remove rustc-guide from status_check. It's only purpose there is to fail the build if the submodule is updated and the status is not test-pass.
  • Somehow exclude rustc-guide from the 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

mark-i-m commented on Jul 17, 2019

@mark-i-m
Member

@ehuss Thanks! I opened #62759

Michael-F-Bryan

Michael-F-Bryan commented on Jul 19, 2019

@Michael-F-Bryan

@Michael-F-Bryan would it be possible to get linkcheck to default to reqwest?

@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

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    C-bugCategory: This is a bug.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.T-infraRelevant to the infrastructure team, which will review and decide on the PR/issue.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @ehuss@mati865@jonas-schievink@pietroalbini@mark-i-m

        Issue actions

          rustc-guide submodule issues · Issue #62739 · rust-lang/rust