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

project CI refresh #8

Merged
merged 17 commits into from
Apr 8, 2024
Merged

project CI refresh #8

merged 17 commits into from
Apr 8, 2024

Conversation

cpu
Copy link
Contributor

@cpu cpu commented Apr 7, 2024

  • Switches out deprecated action-rs workflow dependencies
  • Removes extra clippy workflow
  • Updates MSRV to 1.63, adds to Cargo metadata
  • Updates cargo check to deny warnings, fixes findings
  • Updates clippy to use nightly, fixes findings
  • Updates checkout workflow dep to latest version
  • Adds Dependabot config for keeping cargo & github action deps up-to-date
  • Adds cargo-semver-checks
  • Adds cargo-check-external-types, configured to allow nom types
  • Updates generic bounds for consistency in order, location

cpu added 15 commits April 7, 2024 11:11
* Reformats to better support parameters.
* Adds merge_group trigger for supporting queued merges.
* Adds cron scheduled build.
The `actions-rs/clippy-check` action is archived and there isn't a clear
alternative. Since clippy is already being run in `rust.yml` the right
fix seems to be removing this extra workflow.
This better matches what other rusticata crates are using, and Rust 1.44
seems to be having issues fetching deps in CI:

```

Caused by:
  failed to load source for dependency `nom`

Caused by:
  Unable to update registry `https://github.com/rust-lang/crates.io-index`

Caused by:
  failed to fetch `https://github.com/rust-lang/crates.io-index`

Caused by:
  bad packet length; class=Net (12)
```
```
warning: bound is defined in more than one place
  --> src/combinator.rs:76:47
   |
76 | pub fn upgrade_error<I, O, E1: ParseError<I>, E2: ParseError<I>, F>(
   |                                               ^^
...
81 |     E2: From<E1>,
   |     ^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#multiple_bound_locations
   = note: `#[warn(clippy::multiple_bound_locations)]` on by default
```
@cpu
Copy link
Contributor Author

cpu commented Apr 7, 2024

@chifflier This is the first PR following up from our conversation over in x509-parser.

I figured the best approach would be to update one Rusticata project low in the dep tree and then once you approved of the changes, I can start updating all of the other repos. I wanted to avoid doing them all at once and then having to update many different PRs based on the review feedback.

Hopefully these changes are uncontroversial, but I've done them commit-by-commit to make it easy to ammend/drop parts as req'd.

Copy link
Member

@chifflier chifflier left a comment

Choose a reason for hiding this comment

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

Wow, the patch is bigger than I thought!
Thanks a lot for working on this, the patch looks good to me. There are just minor comments, and an open question for the test feature matrix that can be important before copying the CI files to other crates
I also think the PR strategy is good (patching most depended crates first).

.github/workflows/clippy-check.yml Show resolved Hide resolved
.github/workflows/rust.yml Show resolved Hide resolved
.github/workflows/rust.yml Show resolved Hide resolved
.github/workflows/rust.yml Show resolved Hide resolved
src/combinator.rs Outdated Show resolved Hide resolved
cpu added 2 commits April 8, 2024 09:26
Puts generic bounds in the same order as the generic parameters are
defined.
@cpu
Copy link
Contributor Author

cpu commented Apr 8, 2024

@chifflier Thanks for the review! 🚀 I think this is ready for merge when you're happy. I'll start updating other repos as I find the time with the goal to get through all of them in the next week or two.

@chifflier
Copy link
Member

This all looks fine to me, thank you for the PR and for the replies.
Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants