Closed JarredAllen closed 1 year ago
richardeoin
commented
1 year ago Thanks for the PR! I haven't looked at the 0.9.0 release of smoltcp yet, but of course to would be great to upgrade.
There aren't any special requirements for testing this, but I would ask that you check the examples that use smoltcp. The CI requires that they compile, and before merging the PR I will check some or all of them still function correctly.
JarredAllen
commented
1 year ago Thanks for the PR! I haven't looked at the 0.9.0 release of smoltcp yet, but of course to would be great to upgrade.
There aren't any special requirements for testing this, but I would ask that you check the examples that use smoltcp. The CI requires that they compile, and before merging the PR I will check some or all of them still function correctly.
Good check, the examples don't presently work. I'll fix this up and then get back to you on this.
JarredAllen
commented
1 year ago I think these changes should fix the examples. I don't have the setup to actually run them, but they all (they all = I did a ripgrep for examples that mention smoltcp and only checked those) compile and I think they should work.
Also, the new smoltcp version requires a newer rust version than your CI is running. I didn't touch the MSRV for this crate since it's an optional dependency, but your CI will have to be updated to at least 1.65 for it to run (sorry, I don't know how CI works, so I don't know how to do this myself).
richardeoin
commented
1 year ago I actually suggest that you do bump the MSRV of this crate from 1.62 to 1.65. There's a commit here that shows how to change the MSRV in both the README and the CI.
JarredAllen
commented
1 year ago I actually suggest that you do bump the MSRV of this crate from 1.62 to 1.65. There's a commit here that shows how to change the MSRV in both the README and the CI.
Okay, that's done. I think this should be good to go now.
olback
commented
1 year ago rust-version should probably also be updated in Cargo.toml. Seems like this was missed the last time msrv was bumped.
ryan-summers
commented
1 year ago I'm actually of the opinion of forcing people to upgrade this. They're free to remain on older versions of stm32h7xx-hal, but I don't see the purpose in slowing down development of the HAL to accomodate people not wanting to keep dependencies updated.
Context: I'm currently in the process of updating smoltcp-nal to support 0.9, the changes are non-trivial, but it's not bad IMHO.
richardeoin
commented
1 year ago This needs to be rebased onto master, but is otherwise ready to merge. I don't have permission to modify this branch, so @JarredAllen would you prefer to perform the rebase yourself?
JarredAllen
commented
1 year ago This needs to be rebased onto master, but is otherwise ready to merge. I don't have permission to modify this branch, so @JarredAllen would you prefer to perform the rebase yourself?
Sorry about that, I don't know why it didn't let you rebase it (I thought I enabled the option to let people with access to this repo edit my PR, but maybe I misclicked or something?), but it should be rebased and ready to merge now.
richardeoin
commented
1 year ago No problem. Thanks for the contribution!
bors r+
richardeoin
commented
1 year ago bors r+
bors[bot]
commented
1 year ago Build succeeded:
The smoltcp 0.9.0 release has some features that would be nice to use, but is backwards-incompatible, so I made this PR that updates the dependency and makes the necessary changes to be compatible with the upgrade.
I verified it by checking that my code (which uses this crate and smoltcp extensively) still works, but I'm open to doing other verification to demonstrate that it still works if you want.
Being compatible with the newer version of smoltcp does impose some backwards-incompatible changes on this crate's API, so it'll have to come with a version bump. I hope this is acceptable.