Reduce warning strictness

[epage]

What does this PR try to resolve?

This solves two use cases

• When developing a feature, I might be experimenting with some sloppy, intermediate states and warnings shouldn't prevent me from running tests
• A new dependency being released should not block people from running tests (also solved by the above) or cause CI (for PRs or Bors) to fail, blocking them on something unrelated to their work

How should we test and review this PR?

I didn't see any pertinent history for why cargo conditions deny(warnings) on cfg(test) while other parts of cargo use feature = "deny-warnings", so I just went forward with this consolidation. It does mean some targets that we test without the feature might have warnings slip through though the risk is low.

I tested this against clap master which adds more deprecations.

Additional information

This does raise the risk of deprecations not being addressed but that is a question of contributor culture. I know I would look into it if I saw deprecation warnings go by and I would hope others would too.

[rust-highfive]

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

[weihanglo]

As a person who heeds deprecation warnings, I have no objection to this change. Let's await feedback from others.

[ehuss]

I think making it conditional on deny-warnings is great (I get annoyed when it shows up). I'm not sure why we haven't done that in the past.

But I'm not sure I understand the change to deprecated. Shouldn't allow(deprecated) be added to individual items when they intentionally use deprecated things? I feel uncomfortable unconditionally allowing it.

If the concern is that an update to a dependency deprecated something, then I think it is good that CI fails. That indicates something that should be addressed. CI failures like that are very rare, so I'm not sure if they are particularly painful.

[epage]

I think making it conditional on deny-warnings is great (I get annoyed when it shows up). I'm not sure why we haven't done that in the past.

If the conversation on deprecations takes too long, I'll split these up. I wasn't sure what people's feelings would be on either so I put up both

But I'm not sure I understand the change to deprecated. Shouldn't allow(deprecated) be added to individual items when they intentionally use deprecated things? I feel uncomfortable unconditionally allowing it.

I'm not too sure fully what all was going on but I was still getting deprecation warnings. My interest is for deprecations to be treated as warnings and never error. After dealing with being a clap client, I did similar in the CI for my projects.

If the concern is that an update to a dependency deprecated something, then I think it is good that CI fails. That indicates something that should be addressed. CI failures like that are very rare, so I'm not sure if they are particularly painful.

From my experience, taking an approach like this leads to time lost and frustration, both for contributors and maintainers and I feel its not healthy for projects.

If I wasn't using cargo to test the the next clap release, it would take a developer a couple of hours to update to the next clap release which would could be a period of time before a contributor can get to that and then a period of time to make it through the review cycle. During all of this, any other work on cargo grinds to a halt. Faulty CI causes toil as people assume failures are their fault and investigate and ping us to figure out what is going on. This derails their own work and causes extra task switching for them as well as task switching for us as we respond to them. CI should exclusively be reporting failures related to the code at hand and not for external factors.

In my opinion, more projects should be using deprecations. They provide a compiler-aided approach for working through breaking changes and is a much better experience for upgrading. They also help avoid projects stalling out until their next breaking change (I've observed this in other projects I'm depending on). If I had been more on top of it, I would have proposed a RustConf talk encouraging this.

[weihanglo]

A bold idea: Having a bot to collect warnings and post a comment in the PR to raise the visibility. The bot categorises warnings by deprecations, clippy warnings, or other built-in lint rules. So that either the PR author notices and fixes them, or the reviewer takes it as a reference to request a change, or everyone is happy to ignore it.

[epage]

A bold idea: Having a bot to collect warnings and post a comment in the PR to raise the visibility. The bot categorises warnings by deprecations, clippy warnings, or other built-in lint rules. So that either the PR author notices and fixes them, or the reviewer takes it as a reference to request a change, or everyone is happy to ignore it.

There is a clippy action that makes comments: https://github.com/giraffate/clippy-action

We could also use Serif which tracks alerts across commits and branches

• https://www.reddit.com/r/rust/comments/yz7l2v/github_actions_for_clippy/ixpomjq/?context=3
• https://docs.github.com/en/code-security/code-scanning/integrating-with-code-scanning/sarif-support-for-code-scanning

[weihanglo]

These tools are really cool! I'll vote allowing deprecations on CI with one of those GitHub Actions warning on deprecations.

[bors]

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

[weihanglo]

Since #11699 got landed, is this pull request still useful? Looks like it is something we can close out.

[epage]

@weihanglo the thing that is still covered by this

A new dependency being released should not block people from running tests (also solved by the above) or cause CI (for PRs or Bors) to fail, blocking them on something unrelated to their work

Having a lockfile would also help with this which will hopefully be happening soon?

[epage]

With a Cargo.lock, the value of this is lessened.