Add clippy as a submodule

[oli-obk]

This builds clippy as part of ./x.py build (locally and in CI).

This allows building clippy with ./x.py build src/tools/clippy

Needs rust-dev-tools/dev-tools-team#18 (comment) to be resolved before it can be merged. Contributers can simply open a PR to clippy and point the submodule at the pull/$pr_number/head branch.

This does not build clippy or test the clippy test suite at all as per rust-dev-tools/dev-tools-team#18 (comment)

r? @nrc

cc @Manishearth @llogiq @mcarton @alexcrichton

[Manishearth]

This seems to block CI on clippy builds, yeah?

It seems like rust-dev-tools/dev-tools-team#18 (comment) said we should do this too, but just want to check with @nrc again on that.

I'm very happy if rustc blocks on clippy builds in CI, but I'm just not sure if that's what nrc was going for.

[oli-obk]

This seems to block CI on clippy builds, yeah?

Yes that's how I read that comment, too. I don't think it's too much of a burden on developers to "just get it compiling again", since they just got the entire rustc compiling again and know the knobs to twist. The runtime effects of such a change can then be left to us, as they traditionally are much less complex to figure out (once you have written a few lints).

The only issue I see is that it requires a stage2 build due to a dependency on serde.

Note that currently this also builds cargo clippy. This can easily be circumvented by making cargo clippy an optional, but by-default feature and having rustc build without default features. I can do that in this PR if desired.

[Manishearth]

since they just got the entire rustc compiling again and know the knobs to twist

We should document the process for fixing this somewhere before landing IMO.

(I can do a writeup if you want)

The only issue I see is that it requires a stage2 build due to a dependency on serde.

Because of toml?

Ideally libclippy.so or the rust-clippy driver won't read from toml at all, that's all cargo clippy's job.

Though passing this info down to rust-clippy without a format will be annoying.

[oli-obk]

We should document the process for fixing this somewhere before landing IMO.

done

Ideally libclippy.so or the rust-clippy driver won't read from toml at all, that's all cargo clippy's job.
Though passing this info down to rust-clippy without a format will be annoying.

We can probably pass these things as CLI flags, which are already parsed.

[Manishearth]

might need a git fetch pulls/blah as well.

[Manishearth]

we should test this workflow on a test repo.

[oli-obk]

I'm doing that right now with the Cargo.lock

[oli-obk]

I'm not sure why this is marked S-wating-on-author, as I don't think there's anything left for me to address right now.

[Manishearth]

probably because it's marked as WIP?

[Manishearth]

How regularly should we be updating this? Only in the event of breakage, or should we keep it in sync every day so that rustc contributors have to do less merge conflict work upgrading?

We probably should maintain a vendored branch on the clippy repo that tracks what is currently vendored in rustc for convenience.

[oli-obk]

We probably should maintain a vendored branch on the clippy repo that tracks what is currently vendored in rustc for convenience.

That makes sense. We can merge the vendored branch into master when the nightly is on travis, and open a PR here to update the submodule to clippy's master branch as often as possible. These could even be part of rollups, since they should only produce conflicts very rarely.

[nrc]

How regularly should we be updating this?

This is up to the Clippy devs. Updating means a PR that has to go through the bors process (though it should be a rollup), so it is not trivial. I tend to update the RLS every week or two, basically when I do a release of the client or there is a significant change.

[nrc]

Yes that's how I read that comment, too.

Sorry, I was not 100% clear in that comment. I meant that building and testing should be possible, but neither should block landing PRs.

[nrc]

This looks good to me (modulo the building blocking PRs thing).

@Mark-Simulacrum - could you r? the build system changes please?

[Mark-Simulacrum]

Build system changes seem fine. Note that this won't dist clippy in the current configuration, but that's presumably intentional.

[oli-obk]

This looks good to me (modulo the building blocking PRs thing).

I'm not sure what the point of running it inside CI is, if noone ever notices that the build breaks. Should I turn it on so it is unconditionally run and tested in dev mode (so if someone builds locally), but not on any CI?

Or is this where we add the exception file to the build system, so if clippy breaks, one adds it to the file and it's not built anymore on CI? This way everbody notices when they break clippy and have a choice between ignoring it and @ mentioning us or fixing it?

[nrc]

I'm not sure what the point of running it inside CI is, if noone ever notices that the build breaks.

It is a more long-term goal that Clippy block CI, the core team are not ready to do that until at least the RLS rides the trains to beta (it is considered too risky to have multiple tools doing new things).

Should I turn it on so it is unconditionally run and tested in dev mode (so if someone builds locally), but not on any CI?

That sounds good to me.

[alexcrichton]

Oh wait I think no, let's please not run tests yet. We are not in a position to do so right now. It wouldn't be a great experience to download broken trees from git "by default" as I suspect the clippy submodule will be broken quite often. Although we can land PRs that'd just hurt developers.

I've commented elsewhere about how we're going to run tests, but that support isn't here yet. I don't personally know why we'd add build support in tree before there's infrastructure to actually run tests for it, but I won't stop it if others would like it.

[oli-obk]

It is a more long-term goal that Clippy block CI, the core team are not ready to do that until at least the RLS rides the trains to beta (it is considered too risky to have multiple tools doing new things).

Makes total sense.

Oh wait I think no, let's please not run tests yet. We are not in a position to do so right now.

I'm very confused now. This PR only builds clippy. It does not run its tests.

It wouldn't be a great experience to download broken trees from git "by default"

broken as in doesn't build or broken as in "that commit doesn't exist, git will complain"? Because it'll only be the former.

I suspect the clippy submodule will be broken quite often.

The goal of this PR is to ensure that it will never be broken again, because it'll only break if the developer changes something that would break it. That developer is at the same time in the unique position to know exactly how to fix it (since they just had to fix it all over rustc).

Although we can land PRs that'd just hurt developers.

Soo... simply as stated above

I turn it on so it is unconditionally run and tested in dev mode (so if someone builds locally), but not on any CI?

[alexcrichton]

The goal of this PR, in that case, is not satisfiable right now. We aren't in a position to ensure that "clippy never breaks again".

[oli-obk]

That's fine, it doesn't have to happen in this PR.

If I made it easy to tune out the clippy building failure, but have it still happen by default on local builds, would that be ok?

I mean it could be as simple as uncommenting an entry in a ignore-tools-breakage.toml.

This way, if your PR breaks clippy, you either fix clippy, or turn building it off for everyone who comes after your build. In the first case, we get a PR at clippy, in the second case, someone should ping us, we create a new PR that reenables clippy together with the fixed clippy.

This means noone ever gets a broken clippy without it being their fault.

[Manishearth]

I don't personally know why we'd add build support in tree before there's infrastructure to actually run tests for it, but I won't stop it if others would like it.

Clippy tests don't break often. Mostly clippy-side changes break them, and we CI Clippy side. If a rustc change breaks clippy that's the concern, and we're fine with having tests broken for a short while as we fix things.

This PR has two goals. It starts giving Clippy authors early warning about breakage, and gives PR authors the ability to fix it. Many clippy breakages are from rustc changes that contain mostly automated fixes to the rest of librustc_* and to fix this clippy authors have to go back and figure this out again. It should really be much less effort overall with authors helping. Also keeps clippy's use case of an API in mind, so if an API needs to be changed in a way that completely removed the functionality clippy needs, this can be discussed at the time.

The second goal is to prototype the eventual contribution model. This way we can work towards a smooth integration and work out all the kinks before all the parts are ready.

[alexcrichton]

@oli-obk

This way, if your PR breaks clippy, you either fix clippy, or turn building it off for everyone who comes after your build. In the first case, we get a PR at clippy, in the second case, someone should ping us, we create a new PR that reenables clippy together with the fixed clippy.

Yes that's what the "planned infrastructure" is currently thought to look roughly like. This doesn't exist today, however, so no, I do not think we should enable anything for local builds.

[oli-obk]

Ok. So I'm kind of lost here now wrt what I should do in this PR. I can create said infrastructure if that is desirable and noone has existing plans or already started working on it. Or should I just disable all the clippy building and require one to run ./x.py build src/tools/clippy?

[alexcrichton]

Or should I just disable all the clippy building and require one to run ./x.py build src/tools/clippy?

Isn't that what this PR does?

I don't mind adding more infrastructure here, although I'm not sure if we've hammered out precisely what we'd like it to look like. The discussion here leads me to believe that at least mine and @nrc's perceptions of these workflows may differ?

[oli-obk]

Isn't that what this PR does?

No, it also builds clippy if you just do ./x.py build.

[oli-obk]

I disabled building clippy by default and edited the OP message to reflect the current state of the PR

[alexcrichton]

@oli-obk I've created a tracking issue for the requirements of what we think the infrastructure for running tests would look like.

[carols10cents]

It sounds like this is back in the reviewers' court. @alexcrichton @nrc what are the next steps on this? (also it looks like travis passed even though it says it's still running)

[alexcrichton]

I believe this needs an r+ from @nrc

[shepmaster]

@rust-lang/dev-tools It has been over 6 days since the last comment from a reviewer. Perhaps someone besides @nrc can step up?

[alexcrichton]

I think this documentation may wish to be updated with the current state of the PR

[nrc]

@bors: r+ (sorry for the delay)

[bors]

📌 Commit 20f1e68 has been approved by nrc

[bors]

⌛️ Testing commit 20f1e68 with merge b3b5b99...

[bors]

💔 Test failed - status-travis

[kennytm]

@bors retry #38618

dist-i586-gnu-i686-musl failed:

[01:23:36] test sync::mpsc::sync_tests::fmt_debug_recv ... ok
[01:23:36] test sync::mpsc::sync_tests::fmt_debug_sender ... ok
[01:23:36] test sync::mpsc::sync_tests::fmt_debug_sync_sender ... ok
[01:23:36] error: An unknown error occurred
[01:23:36] 
[01:23:36] To learn more, run the command again with --verbose.
[01:23:36] 
[01:23:36] 
[01:23:36] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "-j" "4" "--target" "i686-unknown-linux-musl" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "std:0.0.0" "-p" "alloc:0.0.0" "-p" "libc:0.0.0" "-p" "compiler_builtins:0.0.0" "-p" "collections:0.0.0" "-p" "unwind:0.0.0" "-p" "core:0.0.0" "-p" "rand:0.0.0" "-p" "panic_abort:0.0.0" "-p" "alloc_system:0.0.0" "-p" "std_unicode:0.0.0" "--"
[01:23:36] expected success, got: exit code: 101
[01:23:36] 
[01:23:36] 
[01:23:36] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test --target i686-unknown-linux-musl --target i586-unknown-linux-gnu
[01:23:36] Build completed unsuccessfully in 1:21:36

[bors]

⌛️ Testing commit 20f1e68 with merge 204c0a4...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: nrc
Pushing 204c0a4 to master...