-
Notifications
You must be signed in to change notification settings - Fork 361
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
Add support for fetching & parsing the Tendermint version of a chain #2302
Conversation
// Remove the pre-release version to ensure we don't give special treatment to pre-releases. | ||
version.pre = semver::Prerelease::EMPTY; |
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 could be problematic, since the -pre versions semantically precede the release without the pre suffix. So this is lossily bumping the version to a later one than what the module author intended.
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.
We added this trimming of Prerelease
version due to this issue: informalsystems/ibc-rs#1337 (comment).
The issue was that networks were using an RC instead of the stable release, e.g.
SDK module at version '0.42.0-rc0' does not meet compatibility requirements >=0.41.3, <=0.42.6
Which was fixed by trimming the Prerelease
element. Not sure if you're hinting at a different issue than the one we noticed in the production signaled in #1337.
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.
The issue was that networks were using an RC instead of the stable release, e.g.
SDK module at version '0.42.0-rc0' does not meet compatibility requirements >=0.41.3, <=0.42.6
This semver check behavior is stricter than what I assumed. I expected that 0.42.0-rc0 does succeed 0.41.3 and precede 0.42.6, but would not meet this requirement: >= 0.42.0. Do pre-release suffixes make versions completely incomparable by the comparison logic?
Which was fixed by trimming the
Prerelease
element. Not sure if you're hinting at a different issue than the one we noticed in the production signaled in #1337.
Thanks for the exposition on this. A proper solution, I think, would be for the compatibility requirements to explicitly include any pre-releases they want to allow, but perhaps practicalities dictate otherwise.
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.
Do pre-release suffixes make versions completely incomparable by the comparison logic?
We could give this a try in a small experiment. Not sure what you want to try exactly, but I managed to reproduce the original issue here: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=c31232aaa272967f2979abf71e83f258
A proper solution, I think, would be for the compatibility requirements to explicitly include any pre-releases they want to allow, but perhaps practicalities dictate otherwise.
Indeed, it's not feasible for us to track all pre-releases and test with each of them individually. We assume that pre-releases have the same API as their corresponding release.
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.
Thanks for the playground; it appears that the semver
crate does treat non-empty pre-release segment as not matching any comparisons with regular versions:
thread 'main' panicked at 'version 0.42.0-rc0 matching req >0.41.5', src/main.rs:14:5
The logic of VersionReq::match
is poorly documented in semver docs, even though the Version
type as such implements a total ordering that is documented to work in the way I described.
Then there is this:
Where the various tools differ in their interpretation or implementation of the spec, this crate follows the implementation choices made by Cargo. If you are operating on version numbers from some other package ecosystem, you will want to use a different semver library which is appropriate to that ecosystem.
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.
Good to go, with reservations I have filed in #2309. These, however, apply to more than the code introduced in this PR, and can be fixed later.
Excellent, thanks for filing 2309 and the great feedback here Mikhail, appreciate the level of details we went into! |
* Add codespace information in unknown SDK error (informalsystems#2268) * ICS20 API improvements (informalsystems#2280) * Remove `Debug` and `'static` requirements on Module trait * Manually implement Debug for `MockRouter` * Remove `Ics20Reader` supertrait `PortReader` * Use primitive_types::U256 instead of uint::construct_uint!() * Impl serde for Amount * Add .changelog entries * Fix relayer Dockerfile for M1 Mac (informalsystems#2286) Otherwise library not found error will be encountered Ref: nodejs/help#3239 * Bump uuid from 1.1.1 to 1.1.2 (informalsystems#2289) Bumps [uuid](https://github.com/uuid-rs/uuid) from 1.1.1 to 1.1.2. - [Release notes](https://github.com/uuid-rs/uuid/releases) - [Commits](uuid-rs/uuid@1.1.1...1.1.2) --- updated-dependencies: - dependency-name: uuid dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump hdpath from 0.6.0 to 0.6.1 (informalsystems#2292) Bumps [hdpath](https://github.com/emeraldpay/hdpath-rs) from 0.6.0 to 0.6.1. - [Release notes](https://github.com/emeraldpay/hdpath-rs/releases) - [Commits](https://github.com/emeraldpay/hdpath-rs/commits) --- updated-dependencies: - dependency-name: hdpath dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump clap from 3.1.18 to 3.2.1 (informalsystems#2291) * Bump semver from 1.0.9 to 1.0.10 (informalsystems#2295) * Bump http from 0.2.7 to 0.2.8 (informalsystems#2296) * Bump tracing from 0.1.34 to 0.1.35 (informalsystems#2290) * Hs/2210 - Fixed the variable TM to point to GAIAD_BINARY (informalsystems#2297) * Fixed variable TM * change log entry and lib-gm version updated to v0.1.3 Co-authored-by: Harveen Singh <[email protected]> * Bump clap_complete from 3.1.4 to 3.2.1 (informalsystems#2288) * Bump clap_complete from 3.1.4 to 3.2.1 Bumps [clap_complete](https://github.com/clap-rs/clap) from 3.1.4 to 3.2.1. - [Release notes](https://github.com/clap-rs/clap/releases) - [Changelog](https://github.com/clap-rs/clap/blob/master/CHANGELOG.md) - [Commits](clap-rs/clap@clap_complete-v3.1.4...clap_complete-v3.2.1) --- updated-dependencies: - dependency-name: clap_complete dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> * Bump clap dependency to 3.2.4 Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Mikhail Zabaluev <[email protected]> * Fix recv packet handler incorrectly using dest port/chan to get receipt/next_seq_recv (informalsystems#2294) * Use packet destination port/channel in recv_packet handler where appropriate * Add .changelog entry * Address review feedback * Use enum for recv-packet results * Improve RecvPacketResult * Ignore acc seq mismath when expected < got (informalsystems#2298) * Ignore acc seq mismath when expected < got * Address review comments * Changelog entry * Add support for fetching & parsing the Tendermint version of a chain (informalsystems#2302) * Fix for informalsystems#2301 * changelog * changelog broken link * KV pairs in log h/t Mikhail * update dependencies to master branch * update tendermint dependency Co-authored-by: Soares Chen <[email protected]> Co-authored-by: Shoaib Ahmed <[email protected]> Co-authored-by: PikachuEXE <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: harveenSingh <[email protected]> Co-authored-by: Harveen Singh <[email protected]> Co-authored-by: Mikhail Zabaluev <[email protected]> Co-authored-by: Anca Zamfir <[email protected]> Co-authored-by: Adi Seredinschi <[email protected]>
…nformalsystems#2302) * Fix for informalsystems#2301 * changelog * changelog broken link * KV pairs in log h/t Mikhail
Closes: #2301
Description
Adds version detection support for the Tendermint dependency. In Hermes logs, this appears as:
PR author checklist:
unclog
.Added tests: integration (for Hermes) or unit/mock tests (for modules).docs/
).Reviewer checklist:
Files changed
in the GitHub PR explorer.