-
Notifications
You must be signed in to change notification settings - Fork 110
chore: update dependencies #318
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
base: master
Are you sure you want to change the base?
chore: update dependencies #318
Conversation
2fab041
to
9f3d063
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good. Thanks. I left some comments.
profile: minimal | ||
toolchain: ${{ env.toolchain_msrv }} | ||
override: true | ||
- uses: Swatinem/rust-cache@v2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove rust-cache?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't, it's built-in 😉
https://github.com/actions-rust-lang/setup-rust-toolchain#inputs
Name | Description | Default |
---|---|---|
cache |
Automatically configure Rust cache (using [Swatinem/rust-cache ]) |
true |
toolchain_style: stable | ||
toolchain_msrv: 1.70.0 | ||
toolchain_h3_quinn_msrv: 1.71.0 | ||
toolchain_msrv: 1.74.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only h3_quinn msrv increase should be necessary for quinn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you check for yourself?
My result was always 1.74.1.
- package-ecosystem: cargo | ||
directories: | ||
- / | ||
- fuzz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if there is a benefit for us with dependabot because as libary users can use newer versions without the need to update them in the libary.
@seanmonstar what do you think?
Maybe we can keep fuzz
and add examples
? Or can we just activate for dev-dependencies?
If deciding to use it for all then the subcrates (h3_quinn,h3_datagram and h3_webtransport) should be added. Or is it enough to add root directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't found dependabot to be sufficiently useful when developing libraries, mostly just annoying. 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better than having libraries with outdated dependencies 😄
I added all crates, /
adds all members, others like fuzz are not included in the workspace.
You can limit the amount of PRs, group and ignore as much as you like if you think that it makes it easier for you merging updates.
https://docs.github.com/en/code-security/dependabot/working-with-dependabot/dependabot-options-reference#ignore--
I wonder how much of a problem is expected if it never has been used here...
But no, your users can only update transitive dependencies within compatible ranges as you can see in my changes.
For example, rcgen = "0.14"
allows them to update to 0.14.5 if my work would be merged.
Until then 0.13.3 is the allowed maximum:
https://crates.io/crates/rcgen/versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Libraries rarely have outdated dependencies, since lockfiles are ignored and semver ranges allow users to upgrade. The only time it's true is when a dependency releases a semver-breaking change, which we typically need to handle manually anyways.
I still think it's better to drop dependabot here.
/// An accepted incoming bidirectional stream. | ||
/// | ||
/// Since | ||
#[allow(clippy::large_enum_variant)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe adding // Todo
comment in order to measure if Box
is useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO added
pub(crate) mod internal_error; | ||
|
||
#[allow(clippy::module_inception)] | ||
mod error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Todo better module names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO added.
Keep in mind that maintainers have all permissions, you can change any line yourself in pull requests.
c8ddc2d
to
373a99c
Compare
Chores