-
-
Notifications
You must be signed in to change notification settings - Fork 33k
gh-138497: Support LLVM_VERSION configuration via env #138498
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
If we do this:
|
I've done that way. Get the env variable in |
b5b9649
to
4189a3e
Compare
@savannahostrowski, thoughts on this change? |
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 this! I think this change is fair, so long as we only advertise that a single LLVM version is officially supported.
Misc/NEWS.d/next/Build/2025-09-04-12-16-31.gh-issue-138497.Y_5YXh.rst
Outdated
Show resolved
Hide resolved
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
I have made the requested changes; please review again |
Thanks for making the requested changes! @savannahostrowski: please review the changes made to this pull request. |
I have made the requested changes; please review again |
Thanks for making the requested changes! @savannahostrowski: please review the changes made to this pull request. |
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 making the updates - I have a couple of other small wording suggestions but I think otherwise this looks good.
Would you also mind updating the Tools/jit/README.md
to reflect this change? Right now, the README says that LLVM 19 is required. With this change, we should call out that LLVM 19 is the officially supported version, but that you can modify it if need be, using the env var.
Misc/NEWS.d/next/Build/2025-09-04-12-16-31.gh-issue-138497.Y_5YXh.rst
Outdated
Show resolved
Hide resolved
eb2a37a
to
18f96e9
Compare
I have made the requested changes; please review again |
Thanks for making the requested changes! @savannahostrowski: please review the changes made to this pull request. |
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.
One minor tweak, but otherwise, LGTM! Thanks for doing this!
This patch makes possible to override the llvm version used during build using "LLVM_VERSION" and "EXTERNALS_LLVM_TAG" environment variables. This is useful to test the build with other LLVM versions without changing the sources, for example to build with llvm-20.
The LLVM_VERSION env variable is passed at configure time to the Tools/jit/build.py script as --llvm-version parameter.
…YXh.rst Rewording news entry Co-authored-by: Savannah Bailey <[email protected]>
a98b4a4
to
a03d668
Compare
I've just rebased on top of main and solved the conflict in configure and configure.ac. |
@danigm Apologies for the delay here - mind resolving the conflicts? Then, we should be good to go. |
Done, I've just merged main branch and solved the conflicts in configure files. |
Yep, this looks good to me. Just the one small grammatical change. As a note, the build step is happening twice so the warning message is printing twice right now. This is not a result of this PR (I introduced this 😅 - resolving in #139241) |
Co-authored-by: Savannah Ostrowski <[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.
it looks OK to me. I'll leave to Savannah to merge it.
Yep - I think we just need to resolve the merge conflicts now. Once that's done, this is ready to go |
This patch makes possible to override the llvm version used during build using "LLVM_VERSION" and "EXTERNALS_LLVM_TAG" environment variables.
This is useful to test the build with other LLVM versions without changing the sources, for example to build with llvm-20.
Related issue: #136895