Allow message patch version #659
Conversation
haixuanTao
force-pushed
the
allow-message-patch-version
branch
from
eabbb02
to
934c84c
Compare
libraries/message/src/lib.rs
Outdated
| let current_version = semver::Version::parse(env!("CARGO_PKG_VERSION")) | ||
| .map_err(|error| format!("failed to parse current version: {error}"))?; |
There was a problem hiding this comment.
This should be how we fill the crate_version argument, so we can just it that.
libraries/message/src/lib.rs
Outdated
| let req = semver::VersionReq::parse(&format!( | ||
| "~{}.{}.0", | ||
| current_version.major, current_version.minor | ||
| )) | ||
| .map_err(|error| format!("failed to set allowing patch version of messages: {error}"))?; |
There was a problem hiding this comment.
Could you clarify why this is needed? What is the issue/error with the current code?
There was a problem hiding this comment.
Ok, so the current code will fail if we send a patched version message. When we bump dora-message to 4.0.1 it will not pass the semver that is below the current version. So 4.0.1 will not pass 4.0.0. See: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c5eaa703cadac747d68985cd10faba7a
With VersionReq::parse("~4.0.0"), we allow for all patch version.
There was a problem hiding this comment.
The thing is we might want to push updates to messages that are unrelated to the node api. For example adding a new message between the cli and the coordinator or what not, and this will allow us to do this.
There was a problem hiding this comment.
Sorry, actually, double checking on this at: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c5eaa703cadac747d68985cd10faba7a
It seems the blocking piece is let matches = req.matches(&specified_version) || specified_dora_req.matches(crate_version);
I'm going to replace this piece.
There was a problem hiding this comment.
It seems the blocking piece is
let matches = req.matches(&specified_version) || specified_dora_req.matches(crate_version);
I'm a bit confused what the issue of that piece is. We compare in both directions and return true when either one matches.
Given that we're using semver conventions for the message crate, caret requirements seemed like the best match. We can also do tillde requirements if you prefer, this would be a bit stricter. For example, 4.1.0 is semver-compatible with 4.0.0, but not tilde-compatible.
There was a problem hiding this comment.
Ok, my bad I confused || with an and sign instead of a or sign.
I guess we can leave is as is in that case
|
Closed as the behviour was actually right. |
Merge after #658
This PR makes dora-message patch version accepted between them, and not fail.