Move deny lint checks to script

[Harshil-Jani]

Closes #1551
Signed-off-by: Harshil Jani harshiljani2002@gmail.com

[apoelstra]

This article which I am coincidentally reading right now suggests just putting RUSTFLAGS=-D warnings across the whole CI script rather than doing one run. (It also has a bunch of other easy to apply suggestions; search "CI Workflow"; but these are unrelated to this PR.)

OTOH I think cargo clippy --all-targets --all-features is likely to accomplish what we want, so I'm happy to ACK this as-is.

[apoelstra]

BTW it looks like CI is having some sort of network hiccup. I'll kick this in a few hours, or another day.

[apoelstra]

ACK 2ad5ba5

[sanket1729]

Amongst all the lints that we are deleting, are we losing anything that the compiler would not warn us? Should we make the deny a warn and the CI can then mark the warnings as errors?

[apoelstra]

Good question. I think missing_docs is not a warning by default, so we should put a warn(missing_docs) back in. But the rest I'm pretty sure are warnings.

[Harshil-Jani]

Thanks @apoelstra for the Article (Added to reading list). I have updated the commit. Does it look good now ?

[apoelstra]

Looks great, thanks!

CI is failing though because we don't have clippy available in all contexts. Could you change test.sh to add the line above the other cargo clippy lines (inside the if "$DO_LINT" block), since we know that clippy is available there?

[apoelstra]

Could you copy the if syntax from bitcoin/contrib/test.sh which gates the existing cargo clippy invocations?

[apoelstra]

This is really interesting .... I'm surprised that we can run the examples in the root like this even though the examples actually live in bitcoin/. I wonder what would happen if multiple crates had examples with the same name.

@Harshil-Jani could you either:

• Add -D warningsto the existing cargo clippy lines and add only a single new cargo clippy --all-targets --all-features line; or
• Do what you've done, but remove the existing cargo clippy block because it's now redundant

[tcharding]

We already have these lines in the bitcoin/contrib/test.sh script, what is the purpose of adding them here?

[apoelstra]

This ones have -D warnings added.

[tcharding]

We have -D warnings in bitcoin/contrib/test.sh also

[Harshil-Jani]

This is really interesting .... I'm surprised that we can run the examples in the root like this even though the examples actually live in bitcoin/. I wonder what would happen if multiple crates had examples with the same name.

@Harshil-Jani could you either:

• Add -D warningsto the existing cargo clippy lines and add only a single new cargo clippy --all-targets --all-features line; or
• Do what you've done, but remove the existing cargo clippy block because it's now redundant

Going ahead with the second implementation.

[apoelstra]

This looks good to me. Interesting that the --all-targets line was already present in bitcoin/contrib.sh. It definitely looks like it belongs in the top-level contrib.sh. Thanks!

[apoelstra]

ACK 98bfd52

[tcharding]

I had a play with this. Looks like the explicit linting of the examples is unneeded, and git blame says I'm the goose that added those in the first place :)

As far as I can tell we need no changes to the CI scripts at all in this PR to achieve the closing of the issue we just remove the "coding convention" attributes as is done. The CI scripts in bitcoin, hashes, and internals already have calls to clippy as: cargo clippy --all-features --all-targets -- -D warnings.

[Harshil-Jani]

Thankyou @tcharding and @apoelstra for clarity and helping out.

I had a play with this. Looks like the explicit linting of the examples is unneeded, and git blame says I'm the goose that added those in the first place :)

As far as I can tell we need no changes to the CI scripts at all in this PR to achieve the closing of the issue we just remove the "coding convention" attributes as is done. The CI scripts in bitcoin, hashes, and internals already have calls to clippy as: cargo clippy --all-features --all-targets -- -D warnings.

[apoelstra]

ACK a1c3082

[Kixunil]

ACK a1c3082