Skip to content
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

Turn off rustc-dev-guide toolstate for now #71731

Merged
merged 2 commits into from
May 7, 2020

Conversation

mark-i-m
Copy link
Member

@mark-i-m mark-i-m commented May 1, 2020

cc @rust-lang/wg-rustc-dev-guide @rust-lang/infra @ehuss

When we first added toolstate, the intent was to use toolstate to linkcheck PRs so that we would know which PRs break links in the guide (e.g. by moving some definition). However, these days, we are mostly getting 429 errors (too many requests) from github (not sure when this changed), and every day, there seems to be a spurious failure of some other sort. This is all despite efforts to filter out spurious failures.

Getting spurious gh pings is annoying, and we're not actually getting a lot out of this linkcheck beyond what we are getting with our CI on the guide's repo, so I'm proposing to disable this until we can figure out what might be a better path forward.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 1, 2020
@mark-i-m
Copy link
Member Author

mark-i-m commented May 1, 2020

r? @spastorino

@spastorino
Copy link
Member

spastorino commented May 2, 2020

@mark-i-m great. I'm not sure what exactly we need to remove to avoid running toolstate on rustc-dev-guide, but quickly checking the code was wondering about

rust/src/bootstrap/test.rs

Lines 1532 to 1558 in fd61d06

#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub struct RustcGuide;
impl Step for RustcGuide {
type Output = ();
const DEFAULT: bool = false;
const ONLY_HOSTS: bool = true;
fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
run.path("src/doc/rustc-dev-guide")
}
fn make_run(run: RunConfig<'_>) {
run.builder.ensure(RustcGuide);
}
fn run(self, builder: &Builder<'_>) {
let src = builder.src.join("src/doc/rustc-dev-guide");
let mut rustbook_cmd = builder.tool_cmd(Tool::Rustbook);
let toolstate = if try_run(builder, rustbook_cmd.arg("linkcheck").arg(&src)) {
ToolState::TestPass
} else {
ToolState::TestFail
};
builder.save_toolstate("rustc-dev-guide", toolstate);
}
}
and
test::RustcGuide,

@kennytm kennytm changed the title Turn of rustc-dev-guide toolstate for now Turn off rustc-dev-guide toolstate for now May 2, 2020
@Mark-Simulacrum
Copy link
Member

I am unsure if this PR as-is works well enough, let's r? @pietroalbini and cc @kennytm who've done toolstate repo work recently. We'll want to be cautious and try to avoid the problem we hit last time where changes led to no pull request being able to land until we fixed toolstate...

@mark-i-m
Copy link
Member Author

mark-i-m commented May 2, 2020

@spastorino, that code is eventually called from the line I deleted. IIUC this will just leave toolstate in it's current state

@pietroalbini
Copy link
Member

I'd prefer to r? @kennytm if they're not busy, I'm not that familiar with toolstate.

@rust-highfive rust-highfive assigned kennytm and unassigned pietroalbini May 4, 2020
@kennytm
Copy link
Member

kennytm commented May 5, 2020

@bors r+

since we may turn it back on later, i'm ok with leaving the code mentioned in #71731 (comment) around.

@bors
Copy link
Contributor

bors commented May 5, 2020

📌 Commit 9e43b00 has been approved by kennytm

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 5, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 5, 2020
…w, r=kennytm

Turn off rustc-dev-guide toolstate for now

cc @rust-lang/wg-rustc-dev-guide @rust-lang/infra @ehuss

When we first added toolstate, the intent was to use toolstate to linkcheck PRs so that we would know which PRs break links in the guide (e.g. by moving some definition). However, these days, we are mostly getting 429 errors (too many requests) from github (not sure when this changed), and every day, there seems to be a spurious failure of some other sort. This is all despite efforts to filter out spurious failures.

Getting spurious gh pings is annoying, and we're not actually getting a lot out of this linkcheck beyond what we are getting with our CI on the guide's repo, so I'm proposing to disable this until we can figure out what might be a better path forward.
@pietroalbini
Copy link
Member

@bors r-

This failed in #71920 with:

error: Tool `rustc-dev-guide` was not recorded in tool state.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 5, 2020
@kennytm
Copy link
Member

kennytm commented May 5, 2020

Okay, need to comment out rustc-dev-guide from the NIGHTLY_TOOLS in src/bootstrap/toolstate.rs as well.

@kennytm
Copy link
Member

kennytm commented May 6, 2020

@bors r+

@bors
Copy link
Contributor

bors commented May 6, 2020

📌 Commit 837c16b has been approved by kennytm

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 6, 2020
@spastorino
Copy link
Member

I was wondering, is there a reason to not remove everything and in case we want to add this back we can just revert the PR?.

@mark-i-m
Copy link
Member Author

mark-i-m commented May 6, 2020

@spastorino Is there something in particular you are concerned about? Why remove everything if we think we might add something back later?

bors added a commit to rust-lang-ci/rust that referenced this pull request May 6, 2020
Rollup of 5 pull requests

Successful merges:

 - rust-lang#70908 (Provide suggestions for type parameters missing bounds for associated types)
 - rust-lang#71731 (Turn off rustc-dev-guide toolstate for now)
 - rust-lang#71888 (refactor suggest_traits_to_import)
 - rust-lang#71918 (Rename methods section)
 - rust-lang#71950 (Miri validation error handling cleanup)

Failed merges:

r? @ghost
@bors bors merged commit 7fc579f into rust-lang:master May 7, 2020
@mark-i-m mark-i-m deleted the guide-toolstate-off-for-now branch May 7, 2020 01:22
@spastorino
Copy link
Member

@spastorino Is there something in particular you are concerned about? Why remove everything if we think we might add something back later?

Nothing in particular, don't worry :).

@RalfJung
Copy link
Member

rustc-dev-guide is still listed in https://github.com/rust-lang-nursery/rust-toolstate/blob/master/_data/latest.json. Looks like the toolstate was not removed completely?

@ehuss
Copy link
Contributor

ehuss commented May 25, 2020

@RalfJung, I think it needs to be removed manually. Essentially someone needs to make a PR equivalent to rust-lang-nursery/rust-toolstate#17 for that entry.

@RalfJung
Copy link
Member

@rust-lang/infra how does one go about removing a toolstate? Is manually editing the JSON really the right way?

@mark-i-m
Copy link
Member Author

@RalfJung Is this causing problems somehow? I'm personally not 100% we want to completely get rid of some sort of guide toolstate...

@RalfJung
Copy link
Member

It means that the information at https://rust-lang-nursery.github.io/rust-toolstate/ is misleading / outdated / confusing.

@mark-i-m
Copy link
Member Author

I guess my question is "confusing to whom?" I understood that nobody else cared about guide toolstate (as it has been outdated/wrong for a long time).

@RalfJung
Copy link
Member

RalfJung commented May 26, 2020

Confusing to anyone who looks at that table and sees every non-"pass" entry as basically a bug that needs fixing (like me). I mean, that's the entire point of even having that table, if you ask me.

@mark-i-m
Copy link
Member Author

@jyn514 jyn514 added the A-testsuite Area: The testsuite used to check the correctness of rustc label Nov 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.