-
Notifications
You must be signed in to change notification settings - Fork 76
Update msrv deps #959
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
Update msrv deps #959
Conversation
d6e2242 to
0ac5e73
Compare
Pull Request Test Coverage Report for Build 17276399990Details
💛 - Coveralls |
|
We will also want to update the README to remove the pinned deps instructions. |
09c02d2 to
60d96d8
Compare
|
Check out Cargo's version requirement syntax. X.Y.Z specified in Cargo.toml only holds X.Y true and the lock only locks X.Y.Z. This Is why for most dependencies I think we want to specify X.Y.Z in Cargo.toml and perhaps X.Y. if we absolutely must. This is what inspired my comment above to prefer complete specification if possible. That lets dependencies that combine choose a higher but compatible version. |
0cb6dc8 to
386a205
Compare
|
I have currently pointed |
386a205 to
468401f
Compare
.github/workflows/python.yml
Outdated
| - name: "Install Rust 1.85.0" | ||
| uses: actions-rs/toolchain@v1 | ||
| with: | ||
| toolchain: 1.78.0 | ||
| toolchain: 1.85.0 |
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 realized from working on #876 that this action is obsolete and doesn't have the desired effect of using the specified toolchain (logs indicate it's always using nightly). We should use:
- name: "Install Rust 1.85.0"
uses: dtolnay/[email protected]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.
Actually #986 was the issue. But I still prefer using the actively maintained rust action.
75a9b28 to
6e49fdb
Compare
6e49fdb to
a7420a6
Compare
|
@DanGould would you like this PR to similarly split into seperate commits like in the ohttp relay payjoin/ohttp-relay#66? |
|
It doesn't need to be incredibly granular but it would be nice if the source code changes could be associated with specific commits for just the relevant dependency upgrades |
36339d2 to
07d9799
Compare
|
Note that we are now pointing directly to the main branch on ohttp-relay instead of a release candidate |
|
I can cut an ohttp-relay release |
657190b to
f5dfe48
Compare
This updates the dependencies of payjoin-cli to match the msrv of payjoin bumped to 1.85.0. This commit does not update any dependencies that create any notable new features or api changes that require changes in the source code.
f5dfe48 to
083620d
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.
I'll approve this because it's not a regression, but one status code handler was changed from a wrong implementation to another wrong implementation that I'd like to see addressed in an immediate follow up. I noticed that the defined anyhow version also varied. Seems OK to me.
ACK 083620d
payjoin-directory/Cargo.toml
Outdated
| [dependencies] | ||
| anyhow = "1.0.71" | ||
| bitcoin = { version = "0.32.4", features = ["base64", "rand-std"] } | ||
| anyhow = "1.0.89" |
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.
payjoin-cli was updated to anyhow 1.0.99. In general I usually update workspace dependencies in tandem (rather than by crate) so that they don't get out of sync and require multiple versions in Cargo.lock.
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.
This is a mistake in that commit! i will change it
|
|
||
| async fn push(&self, mailbox_id: &ShortId, channel_type: &str, data: Vec<u8>) -> Result<()> { | ||
| let mut conn = self.client.get_async_connection().await?; | ||
| let mut conn = self.client.get_multiplexed_async_connection().await?; |
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.
Where'd you find this equivalence? https://docs.rs/redis/latest/redis/#connection-pooling?
Would be nice to have this kind of change documented in the commit log.
payjoin/src/core/ohttp.rs
Outdated
| .status( | ||
| m.control() | ||
| .status() | ||
| .map(|code| { | ||
| http::StatusCode::from_u16(code.code()).map_err(|_| bhttp::Error::InvalidStatus) | ||
| }) | ||
| .unwrap_or(Ok(http::StatusCode::INTERNAL_SERVER_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.
I fear the original code is kind of jank, AND the updated code is jank.
The original is jank because it defaults to INTERNAL_SERVER_ERROR instead of throwing an error in this function when things go wrong. The updated code is jank because it keeps this but also maps to an error variant (InvalidStatus) that is never propagated. I think something like the following is more appropriate, where any problems become an InvalidStatus that actually gets returned as a Result.
| .status( | |
| m.control() | |
| .status() | |
| .map(|code| { | |
| http::StatusCode::from_u16(code.code()).map_err(|_| bhttp::Error::InvalidStatus) | |
| }) | |
| .unwrap_or(Ok(http::StatusCode::INTERNAL_SERVER_ERROR))?, | |
| ) | |
| .status({ | |
| let code = m.control().status().ok_or(bhttp::Error::InvalidStatus)?; | |
| http::StatusCode::from_u16(code.code()).map_err(|_| bhttp::Error::InvalidStatus)? | |
| }) |
This commit updates the payjoin directory deps that do not imact any source code changes or significant feature changes.
The bhttp dependency now no longer needs to be pinned to o.5.1 as we are bumping our msrv to 1.85.0.
794dff7 to
9ef3280
Compare
this update bumps the redis dependency to 1.85.0 and changes some db source with the updated api changes. The `get_async_connection()` method for redis Client was deprecated in v0.25.2 and removed in v0.30.0 the method `get_multiplexed_async_connetion()` is the suggested replacement.
This bumps the dependencies in payjoin ffi to match the overall payjoin msrv of 1.85.0
This commit updates the dependencies in payjoin-test-utils to match the msrv of 1.85.0 that does not change any features or any significant api changes.
this commit updates the dependencies in payjoin to match the new 1.85.0 msrv that don't make any significant api changes or feature changes.
With the new msrv of 1.85.0 we no longer need to lock the version of bhttp and can now use standard semver for this dependency. We do need some minor modifications in the source as the api has changed.
This commit updates the tls related dependencies to mathc the new payjoin msrv to 1.85.0> All these crates have these changes together as the crates are all interdependent on each other. As well there are some significant api changes that come with these new dep bumps. payjoin-cli and payjoin-directory both use tokio-rustls which has an internal rustls module accesible so I removed their direct rustls deps in hopes that it could prevent some divergent changes in the future.
9ef3280 to
7eaa50e
Compare
|
Ok, the anyhow error was a mistake I made when splitting these commits up but is now fixed, as well I added a description for the deprecation of get_async_connection for |
DanGould
left a comment
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.
Legend
ACK 7eaa50e
I took the liberty to update the versions on many of the dependencies that don't necessarily need to be updated based solely on our new msrv.
Every existing dep should now be on an msrv of 1.85.0. Our ohttp-relay is now using the new v0.0.11 release with these changes as well.
There were a few api changes that were made in the source but most of those changes are minor and can be seen in separate commits