-
Notifications
You must be signed in to change notification settings - Fork 2.9k
feat(rust-version): Implement rust-release-channel versioning scheme
#39859
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: main
Are you sure you want to change the base?
Conversation
0e72d7f to
6ba630a
Compare
6ba630a to
eb8b18b
Compare
zharinov
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.
LGTM as far as unit tests reflect the desired behavior (it looks like)
| ${'1.83.0-beta.5'} | ${false} | ||
| ${'1.83.0-beta'} | ${false} | ||
| ${'nightly-2025-11-24'} | ${false} | ||
| ${'stable'} | ${false} |
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 irony 😆
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.
hehe, yeah, but from what I understand isStable() expects a version string, not a random input string, so everything that is not a stable version is rejected by the implementation.
| ${'nightly-2025-11-23'} | ${'nightly-2025-11-24'} | ${false} | ||
| ${'nightly-2024-11-24'} | ${'nightly-2025-11-24'} | ${false} | ||
| ${'nightly-2025-10-24'} | ${'nightly-2025-11-24'} | ${false} | ||
| ${'1.82.0'} | ${'nightly-2025-11-24'} | ${false} |
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.
How is this decided?
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 always treat nightlies as greater than the other versions to achieve a stable sort order. isCompatible() is implemented to treat nightlies as incompatible to the other versions, so this will not actually matter much in practice.
| const parsedVersion = parse(version); | ||
| const parsedCurrent = parse(current); | ||
| if (!parsedVersion || !parsedCurrent) { | ||
| return true; |
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.
true? If we cannot parse versions shouldn't we return false.
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 figured that since the default impl for isCompatible() returns true in all cases then we might want to do the same, but I don't have a strong opinion on this. I'll change it to false instead.
|
|
||
| // Sort and return the highest (last in sorted array) | ||
| matching.sort((a, b) => this.sortVersions(a, b)); | ||
| return matching[matching.length - 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.
| return matching[matching.length - 1]; | |
| return matching.slice(-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.
did you mean matching.slice(-1)[0]? .slice() returns an array, but getSatisfyingVersion() is supposed to return a single string
| return null; | ||
| } | ||
|
|
||
| sortVersions(version: string, other: string): number { |
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 should be called compareVersions
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.
are you sure? https://github.com/renovatebot/renovate/blob/42.42.2/lib/modules/versioning/types.ts#L114-L118 seems to disagree 🤔
eb8b18b to
83320f1
Compare
83320f1 to
78db16a
Compare
Changes
This PR adds a new
rust-release-channelversioning scheme, based on the "Toolchain specification" section of https://rust-lang.github.io/rustup/concepts/toolchains.html.This was extracted from #39529 so that it can be reviewed and merged separately from the corresponding data source.
Context
Please select one of the following:
I did not find an open issue for this at first glance, but there are a couple of related issues/PRs/discussions:
rust-toolchainmanager #38554rust-versiondata source #39529AI assistance disclosure
Did you use AI tools to create any part of this pull request?
Please select one option and, if yes, briefly describe how AI was used (e.g., code, tests, docs) and which tool(s) you used.
I did use Claude Code for the initial draft, but reviewed, adjusted and tested everything manually afterwards.
Documentation (please check one with an [x])
How I've tested my work (please select one)
I have verified these changes via:
The public repository: https://github.com/Turbo87/renovate-rust-test/pulls?q=is%3Apr