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

Did you mean to block nightlies on clippy? #51122

Merged
merged 5 commits into from
Jul 2, 2018
Merged

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented May 28, 2018

@rust-highfive
Copy link
Collaborator

warning Warning warning

  • These commits modify submodules.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 28, 2018
@kennytm
Copy link
Member

kennytm commented May 28, 2018

Did you really mean to block nightlies on clippy? 😏

  • If you want clippy to block nightlies, you need to submit a PR to rust-central-station later (like rust-lang/rust-central-station@913a3b3).
  • If you want clippy to block betas and stables, you need to update src/ci/docker/x86_64-gnu-tools/checktools.sh.

@kennytm kennytm added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label May 28, 2018
@oli-obk
Copy link
Contributor Author

oli-obk commented May 28, 2018

If you want clippy to block betas and stables

We'll first see how this is going

@@ -40,6 +40,8 @@ pub fn pkgname(builder: &Builder, component: &str) -> String {
if component == "cargo" {
format!("{}-{}", component, builder.cargo_package_vers())
} else if component == "rls" {
format!("{}-{}", component, builder.clippy_package_vers())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You swapped rls and clippy here

Copy link
Contributor Author

@oli-obk oli-obk May 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Copy paste at its worst

@bors
Copy link
Contributor

bors commented May 28, 2018

☔ The latest upstream changes (presumably #51118) made this pull request unmergeable. Please resolve the merge conflicts.

@nrc
Copy link
Member

nrc commented May 29, 2018

To clarify, I meant that I think we should do this in a few weeks once we have collected some data about Clippy tests failing and had a discussion with the dev-tools and core teams.

@oli-obk
Copy link
Contributor Author

oli-obk commented May 29, 2018

Oh... I understood that there had been this discussion and we should do this change in order to collect said data.

Apropos, this PR does not actually block nightlies on clippy, it just ships the clippy-driver binary. With a tiny change I can make this PR not produce a component. If we then merge this PR we'll have much easier time in adding clippy later.

So... how do we collect that data? Do I just give you a histogram of number of hours from clippy breaking to clippy releasing a new version? Or do you want the opposite? A histogram for how long clippy actually works? Because we have this data in some form iin the clippy repo.

@oli-obk
Copy link
Contributor Author

oli-obk commented May 29, 2018

I just talked with @Manishearth about this. Here's what I got out of that discussion:

  1. The data we have is that usually clippy breaks around once every two weeks. Sometimes though we get 3-4 days where it breaks every day, and then there's a period where it works for a while.

  2. We don't often fix clippy ahead of time (before a nightly hits that actually breaks clippy), because there's little use in doing so. If nightlies were blocked on clippy, we'd fix clippy and update the submodule immediately when it's broken (the fixes in the last few months were trivial renamings and some restructurings, nothing where clippy has to figure out a new way to do something)

  3. cc @rust-lang/infra would it make sense to speed up merging submodule-only changes by taking the binaries of the previously merged PR and just testing the changed submodules?

@Manishearth
Copy link
Member

Manishearth commented May 29, 2018 via email

@oli-obk
Copy link
Contributor Author

oli-obk commented May 29, 2018

I generated a diagram from our CHANGELOG.md:

grafik

X is the number of days between clippy versions. The same diagram for just 2018 is

grafik

So it seems that the majority of no breakage happening is in the 3-5 day range

Note that this data is obviously wrong, because if we don't release a new version quickly, the number of days between releases goes up, even if the breakage happens after fewer days.
Another reason this data is wrong is due to the fact that as long as rls is broken, we're not getting new nightlies, and thus clippy isn't breaking.

@kennytm
Copy link
Member

kennytm commented May 29, 2018

@oli-obk

I'm not sure if it's worth it. The main difficulty is determine what is meant by "submodule-only changes", since often you also need to update Cargo.lock, and that may change the behavior of the rest of rustc.

@alexcrichton
Copy link
Member

r? @nrc

@rust-highfive rust-highfive assigned nrc and unassigned alexcrichton May 29, 2018
@kennytm kennytm added the T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. label May 29, 2018
@shepmaster
Copy link
Member

Ping from triage, @nrc !

@nrc nrc added the I-nominated label Jun 4, 2018
@nrc
Copy link
Member

nrc commented Jun 4, 2018

Nominating for discussion in this weeks meeting

@Manishearth
Copy link
Member

Manishearth commented Jun 5, 2018

We discussed this today's meeting, we'd like to merge this, without blocking nightlies (just adding a component). Hopefully we can move to a component based model for clippy from here, and eventually block nightlies. The Rustup support for this exists but may not be perfectly tested, so we may have some stuff to fix here.

One thing we also discussed was the data. It seems like one of the problems is that if clippy breaks nightly, it takes long enough to fix it that other breakages end up entering the queue. For this, we should:

r? @kennytm on the toolchain code?

@rust-highfive rust-highfive assigned kennytm and unassigned nrc Jun 5, 2018
Copy link
Member

@kennytm kennytm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except one item, but cc @Mark-Simulacrum for double-checking.

let doc = image.join("share/doc/clippy");
builder.install(&src.join("README.md"), &doc, 0o644);
builder.install(&src.join("LICENSE-MIT"), &doc, 0o644);
builder.install(&src.join("LICENSE-APACHE"), &doc, 0o644);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clippy doesn't have LICENSE-MIT + LICENSE-APACHE, it only has a LICENSE file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still needs to be fixed.

@o01eg
Copy link
Contributor

o01eg commented Jun 6, 2018

Could you mention in config.toml.example that clippy is available in build.tools array?

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 7, 2018

r? @Mark-Simulacrum

@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 Jul 2, 2018
@bors
Copy link
Contributor

bors commented Jul 2, 2018

⌛ Testing commit 6d11439 with merge 83608dbab1d5e1eab6f59a8486d8c4e3fe4e4274...

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 2, 2018

@bors r-

I only updated the LICENSE file mess for clippy docs, not clippy itself

@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 Jul 2, 2018
@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 2, 2018

@bors r+

passes locally now

@bors
Copy link
Contributor

bors commented Jul 2, 2018

📌 Commit 78adefd has been approved by oli-obk

@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 Jul 2, 2018
@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 2, 2018

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Jul 2, 2018

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Jul 2, 2018

📌 Commit 78adefd has been approved by Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Jul 2, 2018

⌛ Testing commit 78adefd with merge 4faaf7e...

bors added a commit that referenced this pull request Jul 2, 2018
@bors
Copy link
Contributor

bors commented Jul 2, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: Mark-Simulacrum
Pushing 4faaf7e to master...

@bors bors merged commit 78adefd into rust-lang:master Jul 2, 2018
@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 2, 2018

No trumpets? Clippy appears still broken, it worked locally :/ Maybe another change jumped in

@o01eg
Copy link
Contributor

o01eg commented Jul 17, 2018

Was it forgotten to add clippy to Kind::Install in builder.rs?

kennytm added a commit to kennytm/rust that referenced this pull request Jul 18, 2018
Allow clippy to be installed with make install

After rust-lang#51122 clippy is available as a component but doesn't install when building from source.

This PR allows to install clippy with extended tools.
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Sep 14, 2018
Version 1.29.0 (2018-09-13)
==========================

Compiler
--------
- [Bumped minimum LLVM version to 5.0.][51899]
- [Added `powerpc64le-unknown-linux-musl` target.][51619]
- [Added `aarch64-unknown-hermit` and `x86_64-unknown-hermit` targets.][52861]

Libraries
---------
- [`Once::call_once` now no longer requires `Once` to be `'static`.][52239]
- [`BuildHasherDefault` now implements `PartialEq` and `Eq`.][52402]
- [`Box<CStr>`, `Box<OsStr>`, and `Box<Path>` now implement `Clone`.][51912]
- [Implemented `PartialEq<&str>` for `OsString` and `PartialEq<OsString>`
  for `&str`.][51178]
- [`Cell<T>` now allows `T` to be unsized.][50494]
- [`SocketAddr` is now stable on Redox.][52656]

Stabilized APIs
---------------
- [`Arc::downcast`]
- [`Iterator::flatten`]
- [`Rc::downcast`]

Cargo
-----
- [Cargo can silently fix some bad lockfiles ][cargo/5831] You can use
  `--locked` to disable this behaviour.
- [`cargo-install` will now allow you to cross compile an install
  using `--target`][cargo/5614]
- [Added the `cargo-fix` subcommand to automatically move project code from
  2015 edition to 2018.][cargo/5723]

Misc
----
- [`rustdoc` now has the `--cap-lints` option which demotes all lints above
  the specified level to that level.][52354] For example `--cap-lints warn`
  will demote `deny` and `forbid` lints to `warn`.
- [`rustc` and `rustdoc` will now have the exit code of `1` if compilation
  fails, and `101` if there is a panic.][52197]
- [A preview of clippy has been made available through rustup.][51122]
  You can install the preview with `rustup component add clippy-preview`

Compatibility Notes
-------------------
- [`str::{slice_unchecked, slice_unchecked_mut}` are now deprecated.][51807]
  Use `str::get_unchecked(begin..end)` instead.
- [`std::env::home_dir` is now deprecated for its unintuitive behaviour.][51656]
  Consider using the `home_dir` function from
  https://crates.io/crates/dirs instead.
- [`rustc` will no longer silently ignore invalid data in target spec.][52330]

[52861]: rust-lang/rust#52861
[52656]: rust-lang/rust#52656
[52239]: rust-lang/rust#52239
[52330]: rust-lang/rust#52330
[52354]: rust-lang/rust#52354
[52402]: rust-lang/rust#52402
[52103]: rust-lang/rust#52103
[52197]: rust-lang/rust#52197
[51807]: rust-lang/rust#51807
[51899]: rust-lang/rust#51899
[51912]: rust-lang/rust#51912
[51511]: rust-lang/rust#51511
[51619]: rust-lang/rust#51619
[51656]: rust-lang/rust#51656
[51178]: rust-lang/rust#51178
[51122]: rust-lang/rust#51122
[50494]: rust-lang/rust#50494
[cargo/5614]: rust-lang/cargo#5614
[cargo/5723]: rust-lang/cargo#5723
[cargo/5831]: rust-lang/cargo#5831
[`Arc::downcast`]: https://doc.rust-lang.org/std/sync/struct.Arc.html#method.downcast
[`Iterator::flatten`]: https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.flatten
[`Rc::downcast`]: https://doc.rust-lang.org/std/rc/struct.Rc.html#method.downcast
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.