search

rusticata / x509-parser

X.509 parser written in pure Rust. Fast, zero-copy, safe.
Other
206 stars 67 forks source link

clippy: fix new findings, add cron schedule to CI #155

Closed cpu closed 5 months ago

cpu commented 5 months ago

This branch fixes some clippy findings that are breaking CI for new builds (for example).

It also adds schedule cron triggers to the clippy-checks and rust workflows so that we can catch these things sooner when there isn't active development work pushing new branches on a regular cadence.

cpu commented 5 months ago

I'll try to find some time for some other small CI cleanups in the near future but wanted to get this up to unbreak CI for external contributors.

Some items on my mind for follow-up:

cpu commented 5 months ago

@chifflier Can I bug you for a review? :bow:

chifflier commented 5 months ago

Hi, Thank you for your time, and for this PR! I'm currently reviewing it. I also have hit the warnings from clippy-unstable in some other projects and find this "forward-breakage" quite annoying, but in some way it is the exact reason why I added unstable in the CI :)

I'll try to find some time for some other small CI cleanups in the near future but wanted to get this up to unbreak CI for external contributors.

Some items on my mind for follow-up:

* The way clippy integration is being handled seems to [always fail](https://github.com/cpu/x509-parser/actions/workflows/clippy-check.yml) on forked repos. It would be nice to fix that.

I wasn't aware of that, it is indeed a problem

* There are a lot of "Node.js 16 actions are deprecated" warnings from the actions-rs tooling. Most of the projects I contribute to have moved away from those to [dtolnay/rust-toolchain](https://github.com/dtolnay/rust-toolchain) and I suggest we do the same. The [actions-rs organization](https://github.com/actions-rs) has been archived and the tooling is abandonware :-(

I'm all in favor of a move to a maintained code. Since there are many repositories under the rusticata organization, I would try to apply this to all of them. I don't think we use specific or complex features from this action, so this should not get too complex. Do you want to work on it? That would be awesome!

* I've had good luck in other repos adding [cargo-semver-checks](https://github.com/obi1kenobi/cargo-semver-checks) and [cargo-check-external-types](https://github.com/awslabs/cargo-check-external-types) to help catch issues. Could be worthwhile here too 👍

I use cargo-semver-checks in my local tests before release, but never added it to the CI. That would be nice! I do not use the other tool so I can't tell, but globally speaking I prefer to add as many checks as possible in the CI.

Thank you again for these great ideas and proposals

chifflier commented 5 months ago

Applied, thanks!

cpu commented 5 months ago

Thanks for the feedback :-)

Do you want to work on it? That would be awesome!

Yes, I will try to open some PRs over the next ~week or so.

cpu commented 5 months ago

Yes, I will try to open some PRs over the next ~week or so.

Here's the first: https://github.com/rusticata/rusticata-macros/pull/8 Let's get that one approved and then I will do the other repos once I know the changes look good.