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

Add infrastructure for disabling submodules easily #44077

Closed
alexcrichton opened this issue Aug 24, 2017 · 12 comments · Fixed by #43628
Closed

Add infrastructure for disabling submodules easily #44077

alexcrichton opened this issue Aug 24, 2017 · 12 comments · Fixed by #43628
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

We'd like to expand the number of submodules and tools that we build as part of the main Rust distribution, including the RLS, Clippy, and Rustfmt. In doing so however these tools all depend on nightly features in the compiler which means that they're highly susceptible to breakage. I believe that historically, at least, Clippy has broken quite regularly due to changes in the compiler.

In order to ease the rust-lang/rust contributor developer experience, we're thinking that we'll want a way to easily, and temporarily, disable building/testing the tool in CI. This means that if a contributors accidentally breaks the submodule there's no required pressure for them to do a submodule dance and update things, but rather it can happen after the PR has landed. This in turn means that there will be nights where not all tools are built, but the hope is that this is only periodically and only for nightlies, not for stable/beta releases.

The requirements of this infrastructure would look like:

  • There's a file in the repository with a list of tools that are "known to fail".
  • All tools are always built unconditionally.
    • If the tool fails to build and it's in the "known to fail list", then the build continues
    • If the tool fails to build and it's not in the "known to fail list", then the built halts with an error. The build also prints out a diagnostic indicating the presence of the "known to fail" list and suggests adding the tool to that list.
    • If the tool succeeds to build and is in the "known to fail list" then the build halts with an error. The build also prints out how to correct this by updating the file in question
    • If the tool succeeds to build and is not in the "known to fail list", the build continues.
  • Tools which fail to build are not packaged up. We don't build tarballs. They're listed as available = false in the manifest.

The thinking is that this file would also have contributors and maintainers to cc whenever a tool is added to the file, so they get a heads up that breakage is happening. As soon as there's a new nightly then the tool can get updated, fix its own CI, ensure tests pass, and then finally send a PR to update the submodule in rust-lang/rust and remove the tool from the "known to fail list' as well.

cc @rust-lang/dev-tools

@alexcrichton alexcrichton added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. labels Aug 24, 2017
@alexcrichton
Copy link
Member Author

Oh it's also worth mentioning that this assumes that rustup can handle missing components on nightlies. AFAIK there's support for that, but there's been bugs in it historically and it's not well tested, so we may have some bumps to work out.

@Mark-Simulacrum
Copy link
Member

I think we'd also want support in rustup to not update to a new nightly if some tools aren't built for it.


As an aside, I'm not sure I'm happy with the approach we're taking here. It's concerning that we're willing to ship incomplete toolchains for nightlies; I worry that we'll have to also implement (even if on a human basis) some form of lock a week or so in advance of beta so that we guarantee that beta ships with all tools built. To be clear, I'm fine with doing this -- but I think we should be careful to avoid delays in beta or incomplete betas (and hence, stable). I'm not sure I have any real alternative suggestions though.

@alexcrichton
Copy link
Member Author

I believe rustup has at least some support for this in the sense that manifests can have available = false in them. That code path has been only very lightly tested and buggy in teh past s well though!

You're right though that we'll need to be vigilant around releases and otherwise see how this pans out. Tools like the RLS and rustfmt have empirically broken very rarely so far, but it's expected that tools like Clippy break quite often, so I'm not quite sure how this'd pan out.

That being said, this came out of a lot of discussions, so it's unclear how we'd do better...

@Mark-Simulacrum
Copy link
Member

I agree that there doesn't seem to be a better alternative--I just wanted to voice my worry about this approach.

@arielb1
Copy link
Contributor

arielb1 commented Aug 29, 2017

Shouldn't we instead make the "lockstep upgrade" experience easier? I think the PR author is often in the best place to fix Rust breakage. How big of a concern is this in practice?

@Manishearth
Copy link
Member

I think the PR author is often in the best place to fix Rust breakage.

Agreed. For clippy at least, most breakages are of the kind where the PR author has actually already made similar fixes to librustc_* so fixing clippy is ~no effort. The first option should be fixing clippy, but if it's nontrivial the option to disable it should be there.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 30, 2017

the option to disable it should be there.

While there's no one file to rule them all right now, it would be as easy as flipping this boolean in order to disable it (right now it is disabled).

We could move that associated constant to be a trait method, and offer a simple way to query the central "known to fail" file.

@Mark-Simulacrum
Copy link
Member

The constant is supplemented by the default_condition method on the ShouldRun struct.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 30, 2017

That is only called if the constant is false (and a path was passed to x.py) (see https://github.com/rust-lang/rust/blob/master/src/bootstrap/builder.rs#L71-L74)

@Mark-Simulacrum
Copy link
Member

No? The should_run is called for all Steps and information is stored; the information is read here when no paths are passed; if DEFAULT && default_condition then we run the step by-default.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 30, 2017

Oh. I totally misread those docs then. Thanks.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 1, 2017

I mostly implemented this in #43628.

If the tool succeeds to build and is in the "known to fail list" then the build halts with an error. The build also prints out how to correct this by updating the file in question

I did not implement that. I don't think anyone will be accidentally fixing tools.

bors added a commit that referenced this issue Sep 18, 2017
Run the miri test suite on the aux builder and travis

Reopen of #38350

see #43340 (comment) for earlier discussion

Rationale for running miri's test suite in rustc's CI is that miri currently contains many features that we want in const eval in the future, and these features would break if the test suite is not run.

fixes #44077

r? @nikomatsakis

cc @eddyb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants