Stability tracking - Clippy

[nrc]

Rust distribution:

• move to nursery
• move tests back to ui tests
• Wait for RLS/rustfmt distro issues to be worked out Stability tracking - Clippy #18 (comment)
• Add to Rust repo submodule
  • Add building miri to Rust CI
  • Add miri's tests to Rust CI
• Add to Rust distro for nightly only
• Start recommending the distro version
• Make cargo clippy require rustup component add clippy, clean up cargo clippy's impl
• Clippy 1.0, if it's not happened already (probably will have)
• Turn it on for stable too

[oli-obk]

What are the preconditions for the nursery move?

[nrc]

What are the preconditions for the nursery move?

None. We can do it asap.

[oli-obk]

Can we do an intermediate step and move clippy to the rust repository, but keep publishing it in the current manner? There have been complaints and a lot of "me too" github votes on nightly breakage that isn't fixed within a day or two.

[Manishearth]

Note that the "move clippy to the rust repo" plan is probably going to be via submoduling (perhaps submoduling a fork/branch to make it easier to do in-place fixes without waiting for clippy review). So the repo stays here, but it also goes in tree and gets CId.

I'm open to doing that step now (adding a submodule and possibly setting up CI, but no distribution). Is there anything we should be worried about before this?

I think the logical progression here would be:

• Add to Rust repo submodule
• Add to Rust CI
• Wait for RLS/rustfmt distro issues to be worked out
• Add to Rust distro for nightly only
• Start recommending the distro version
• Make cargo clippy require rustup component add clippy, clean up cargo clippy's impl
• Clippy 1.0, if it's not happened already (probably will have)
• Turn it on for stable too

[oli-obk]

I edited the OP to reflect these steps.

Is there anything we should be worried about before this?

I should probably move our tests back to ui tests so they integrate better with rustc's test suite.

[nrc]

Add to Rust repo submodule

I think this should be ok to do, I'll need to check with some of the core team

Add to Rust CI
Wait for RLS/rustfmt distro issues to be worked out

We're probably going to need to do these in the opposite order since once you're in the CI, you block PRs landing which mean all issues with the updating procedure, etc. need to be worked out.

Add to Rust distro for nightly only
...
Turn it on for stable too

Although we did this for the RLS, there is a strong feeling that we shouldn't do this for Rustfmt and Clippy. That is, once it hits the distro, it should be ready to ride the trains. We can use a clippy-beta name rather than just clippy to indicate that it is not 1.0 and there will be infrastruture to automatically rename to just clippy when we're ready

I should probably move our tests back to ui tests so they integrate better with rustc's test suite

I think tests would be run separately from the compiler's (this is what we do with Cargo and RLS), so I don't think this is necessary (can still do it if you want, of course).

[oli-obk]

We're probably going to need to do these in the opposite order since once you're in the CI, you block PRs landing which mean all issues with the updating procedure, etc. need to be worked out.

is there a tracking issue for that or some other central place where the issues are discussed?

[nrc]

is there a tracking issue for that or some other central place where the issues are discussed?

Not really sorry. There's been a lot of ad hoc discussion on this topic - some on i.r-l.o and some in meetings, etc.

Some issues are tracked here: #15

This doc discusses some things: https://github.com/nrc/dev-tools-team/blob/master/stability/README.md

And there are some 'unknown unknowns' that we'll discover as we push on the RLS distribution

[Manishearth]

We're probably going to need to do these in the opposite order since once you're in the CI, you block PRs landing which mean all issues with the updating procedure, etc. need to be worked out.

I don't agree. The distro issues are a completely different set of issues from the CI issues, and tying them together seems like an unnecessary delay.

(To be clear, when I said "distro issues", I was talking about the whole deal of "do we choose to have partial nightlies?", "how does this interact with the dist build", "how does rustup component add work with this?", "installed by default?".)

I would like to add to CI early perhaps with the option for PR authors to turn it off to get stuff landed, as we work out the details of the process. IMO this will at least give us an early warning system, and by encouraging rustc devs to fix it on the clippy side too we can gauge how easy the process is and work towards improving it, all the while having an escape hatch for rustc devs when the process gets too hard.

In general having a submodule which PR authors are allowed to redirect temporarily to a fork or a branch is a pretty smooth process, with a bit of burden on clippy devs to resync to master afterwards (which is fine).

I don't really think this must be figured out for RLS and rustfmt first either, especially because it seems like these discussions for rustfmt and RLS haven't really gotten anywhere yet -- I'm willing to coordinate/push whatever decision making is necessary for clippy to move forward here.

That is, once it hits the distro, it should be ready to ride the trains.

I don't really see a reason to do this, but I see a major reason to not. Specifically, cargo-clippy does really weird things now so that cargo install clippy is possible, and this can be greatly simplified once the plugin dylib is in the distro, but this is a nontrivial amount of work requiring both rustc and clippy changes and is best done after clippy is in the nightly distro since it's easier to do incrementally.

It would also make life much easier for clippy users without requiring us to fix all the problems (which we can continue to work on incrementally); since then it won't break every two nightlies.

[nrc]

I don't agree. The distro issues are a completely different set of issues from the CI issues, and tying them together seems like an unnecessary delay.

I'm not talking about the distro issues, I'm talking about issues (both technical and political) of having tools CI blocking Rust PRs. There has been a lot of discussion and a lot of ideas floated, and in that situation we need to get actual experience with the processes before over-committing.

perhaps with the option for PR authors to turn it off to get stuff landed

sure, but this needs to be implemented, documented, and communicated to other teams.

In general having a submodule which PR authors are allowed to redirect temporarily to a fork or a branch is a pretty smooth process, with a bit of burden on clippy devs to resync to master afterwards (which is fine).

This is what we do with the RLS, but it is a bit more complicated than this. One constraint is that we don't want to point a submodule at a branch that might disappear, which rules out branches on individual's repos. That means one of the RLS/Clippy maintainers has to push the branch to upstream (and make sure it never gets deleted). This has proven to be enough of a speed bump that nobody has done this except me (a couple of times we've merged PRs to the RLS or Rustfmt that can solve a problem backwards compatibly).

I don't really think this must be figured out for RLS and rustfmt first either, especially because it seems like these discussions for rustfmt and RLS haven't really gotten anywhere yet

This is not correct - the RLS is in tree and part of the CLI and we're getting useful information from working with it.

I don't really see a reason to do this

This is an implementation issue - we simply don't have anything in the infrastructure to have something part of nightly and not ride the trains. For the RLS, it has been manually removed with every release, but that is not something that can be generalised (in fact people are pretty unhappy about doing it for the RLS). Core team feeling when we discussed tool stability was that there was little benefit from the nightly-only situation. However, it sounds like for Clippy specifically it might be useful to ease implementation - if we landed at the start of a cycle, would 6 weeks be enough time to do that implementation?

[Manishearth]

I'm not talking about the distro issues

I meant that I was 😄 . The "figure out CI plan" was a part of "add to CI" (not "Wait for RLS/rustfmt distro issues to be worked out"), I didn't explicitly mention it, my bad.

That means one of the RLS/Clippy maintainers has to push the branch to upstream

I'd have a separate repo/fork on rust-lang which all rust reviewers have push for, and clippy/devtools devs keep the two in sync.

Also, submodules pin to commits, not branches, and IIRC github doesn't delete commits after the first few minutes of their existence. We'd have to check this.

Even then, all that is sufficient to make this work is to merge submodule contributions via git merge never git rebase, because then that same hash will be in the clippy tree forever (as it is reachable from master). This lets us delete branches if we want.

This is not correct - the RLS is in tree and part of the CLI and we're getting useful information from working with it.

Fair; I wasn't aware of this.

if we landed at the start of a cycle, would 6 weeks be enough time to do that implementation?

maaaybe.

I guess a simple hack is to continue to have cargo-clippy (which so far we're keeping out of distro; though I'm open to this changing) enforce nightly. The current version has nightly-only code by necessity, and it will continue to do so during most of the process of refactoring implementation. Once most of that is torn out we can move to a stable-compat cargo-clippy, but we can also just keep a dummy #[feature(rustc_private)] in there until we're ready for it to ride the trains. In the meantime, the distributed clippy will be available for download on beta, but utterly useless there.

The nice thing is that this process is something totally compatible with the "maaaybe" I gave you -- we can try to clean things up in 6 weeks. If done in time we remove the feature flag; if not, not much harm done, we build a useless dylib on a couple platforms that can be fetched via rustup but does nothing once fetched.

[oli-obk]

New issue: @eddyb mentions that it is desirable to get rid of all plugins. Since clippy is currently a plugin, I see two ways forward:

1. link clippy to the compiler
2. require cargo clippy to stay as it is by being its own driver and injecting the clippy lints (that would suggest moving cargo clippy to rustc, too though)

I don't see an issue with 1. since clippy will be part of the distribution anyhow

[eddyb]

IMO custom drivers are the way forward, and the API should change to be more ergonomic.

[Manishearth]

That sounds ok to me if the API is becoming more ergonomic.

I think as a first step keeping it a regular plugin would help make transitioning easy.

[nrc]

+1 for custom drivers. More ergonomics there would of course be great. I'm not really sure what Clippy's needs are, but happy to review code/design suggestions (the current API is a bit ad hoc and has basically evolved for the RLS and whatever crazy tool idea I was experimenting with at the time).

To be clear about what 'plugin' means, we're talking about the pluggable lints facility?