New lint and cargotest

sanxiyn 2016-04-25 11:03:40 UTC #1

I improved a lint to warn unused trait imports. This failed to merge, because the lint triggered for unused trait imports in cargo as designed, which causes cargotest to fail because of #[deny(unused)]. As far as I can tell there is no way to tell cargo to pass --cap-lints to rustc. So I am not sure how to proceed.

As is, this means new lint can't be merged until it is fixed in all cargotest crates, as well as existing requirement that it is fixed in all of rustc and rustdoc.

brson 2016-04-26 18:43:20 UTC #2

This might sound insensitive, but this is exactly why cargotest exists! If this feature is important you'll need to convince reviewers that it's worth breaking cargo for, then fix cargo and update cargotest. Sorry not sorry

ker 2016-04-27 14:38:24 UTC #3

Improving lints is not considered "breaking" as per rfc1193, so technically the fact that cargotest has #[deny(...)] attributes is the issue.

sanxiyn 2016-05-03 10:07:09 UTC #4

Okay, changing the requirement from "fix all of rustc and rustdoc" to "fix all of rustc and rustdoc and cargo" is not that a big deal. Still, it is a noticable change, because Cargo is in the different repository.

But cargotest includes projects other than Cargo, which is just Iron at the moment. As I understand, more will be added in the future. Is the intention that landing new lint should be blocked on fixing Iron and additional crates to be determined in the future?

ker 2016-05-03 11:50:40 UTC #5

I think crates in the cargotest list should not have deny or forbid attributes. Instead their CI should set that flag through RUSTFLAGS or similar

sanxiyn 2016-05-03 12:17:30 UTC #6

I think crates in the cargotest list should not have deny or forbid attributes. Instead their CI should set that flag through RUSTFLAGS or similar.

I am not sure how breaking CI is any better than breaking build. @alexcrichton expressed the same opinion at https://github.com/rust-lang/cargo/pull/2622, which also suggested "deny in CI".

brson 2016-05-06 17:08:53 UTC #7

It's mine yeah. Adding crates to cargotest shouldn't be done lightly.