feat(build): centralize LLVM_VERSION by reneleonhardt · Pull Request #142786 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Features
- Centralized the diverged
LLVM_VERSIONin docker / non-docker builds - Folded the higher number from non-docker 20.1.3 (latest is 20.1.7)
- Prepared the renovate comment if updates would be allowed in
.github/renovate.json5
Working example
https://docs.renovatebot.com/modules/manager/regex/#regular-expression-capture-groups
{ "customManagers": [ { "description": "Update _VERSION in ci scripts", "customType": "regex", "managerFilePatterns": ["src/ci/**/.sh"], "matchStrings": [ "# renovate: datasource=(?.?) depName=(?\S+)( versioning=(?\S+))?( extractVersion=(?\S+))?[^\n]*\nexport .+_VERSION=(?\S+)", ], "versioningTemplate": "{{#if versioning}}{{{versioning}}}{{else}}semver-coerced{{/if}}", }, ], }
r? @marcoieni
rustbot has assigned @marcoieni.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
Area: The testsuite used to check the correctness of rustc
Status: Awaiting review from the assignee but also interested parties.
Relevant to the infrastructure team, which will review and decide on the PR/issue.
labels
This comment has been minimized.
| # To keep docker / non-docker builds in sync |
| # renovate: datasource=github-releases depName=llvm/llvm-project versioning=semver-coerced extractVersion=^llvmorg-(?\d+\.\d+\.\d+(?:.*)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be automatically updated I think. LLVM updates are more risky than regular dependency updates and frequently need changes on the rust side.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renovate doesn't merge automatically, maintainers would still have to do that manually like for dependabot.
For what it's worth, you saw that I didn't enable the regex customManager for these lines in renovate.json5...
| # To keep docker / non-docker builds in sync |
|---|
| # renovate: datasource=github-releases depName=llvm/llvm-project versioning=semver-coerced extractVersion=^llvmorg-(?\d+\.\d+\.\d+(?:.*)) |
| export LLVM_VERSION=20.1.3 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe put this in shared.sh instead?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be possible of course.
I didn't know if all those functions would be unwanted in the non-docker builds, that's why I separated the shared versions from shared functions...
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These versions should not be shared. There is little value in keeping them exactly synchronized, and the process for updating them is quite different. One of them is just built in CI, while the other requires uploading new artifacts via ci-mirrors.
These versions should not be shared. There is little value in keeping them exactly synchronized, and the process for updating them is quite different. One of them is just built in CI, while the other requires uploading new artifacts via ci-mirrors.
There was no comment explaining why non-docker would need the outdated 20.1.0-rc2 instead of 20.1.0-rc3 or a stable release like in docker builds (20.1.3).
There were these comments in both files as you can see in the diff:
# Try to keep the LLVM version here in sync with src/ci/scripts/install-clang.sh
# Try to keep this in sync with src/ci/docker/scripts/build-clang.sh
If both comments have been wrong, please clarify them and upgrade both scripts separately.
You would still be able to enable renovate to update both separately and benefit from automatic CI builds for docker and non-docker to see if a new release is compatible respectively brings no regression for one of them.
The comment isn't wrong per-se, it just maybe could be clarified a bit. Normally we would say something like "WARNING: KEEP IN SYNC WITH X" if it was really required. Here it's more like "when you bump the Linux version, try to think of also updating the Windows/MacOS version sometime in the near-ish future, just to make sure that these versions don't diverge by 5 years.. again :D".
Happy to review clarification of the comments. We don't need renovatebot here though.
In other words, I can't find any release candidate there (or current version for that matter), so the original comments were wrong and syncing docker / non-docker builds isn't wanted, the versions should always be different?
I could write renovate rules to update both separately from each other, then team red and green can see for themselves when a new version passes CI and merge it.
As I explained here, the versions might be different, but in general we try to make them be close to each other in reasonable a timeframe (which can be weeks/months). So I think that this is not a good candidate for automatic syncing.
Labels
Area: Our Github Actions CI
Area: The testsuite used to check the correctness of rustc
Status: Awaiting review from the assignee but also interested parties.
Relevant to the infrastructure team, which will review and decide on the PR/issue.