Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

gh-138497: Support LLVM_VERSION configuration via env #138498

wants to merge 10 commits into from
+56 −25

Conversation

danigm
Copy link

@danigm danigm commented Sep 4, 2025 •
edited by bedevere-app bot
Loading

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

@bedevere-app bedevere-app bot mentioned this pull request Sep 4, 2025
Open
@brandtbucher
Copy link
Member

If we do this:

  • We should do it in a manner similar to CFLAGS_JIT, where the env var is captured by the configure script and passed via the command-line to the build script.
  • EXTERNALS_LLVM_TAG should be left alone, I don't think it makes sense for anyone to override that (it's only used for Windows builds and it's literally a tag in a repo we control).

@danigm
Copy link
Author

danigm commented Sep 9, 2025

If we do this:

* We should do it in a manner similar to `CFLAGS_JIT`, where the env var is captured by the `configure` script and passed via the command-line to the build script.

* `EXTERNALS_LLVM_TAG` should be left alone, I don't think it makes sense for anyone to override that (it's only used for Windows builds and it's literally a tag in a repo we control).

I've done that way. Get the env variable in configure and pass to Tools/jit/build.py script as a new argument --llvm-version.

@danigm danigm force-pushed the llvm-version branch 2 times, most recently from b5b9649 to 4189a3e Compare September 9, 2025 08:03
@brandtbucher
Copy link
Member

@savannahostrowski, thoughts on this change?

@emmatyping emmatyping removed their request for review September 11, 2025 23:57
savannahostrowski
savannahostrowski requested changes Sep 15, 2025
View reviewed changes
Copy link
Member

@savannahostrowski savannahostrowski left a 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.

@bedevere-app
Copy link

bedevere-app bot commented Sep 15, 2025

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. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

emmatyping
emmatyping reviewed Sep 16, 2025
View reviewed changes
@danigm
Copy link
Author

danigm commented Sep 16, 2025

I have made the requested changes; please review again

@bedevere-app
Copy link

bedevere-app bot commented Sep 16, 2025

Thanks for making the requested changes!

@savannahostrowski: please review the changes made to this pull request.

@danigm
Copy link
Author

danigm commented Sep 17, 2025

I have made the requested changes; please review again

@bedevere-app
Copy link

bedevere-app bot commented Sep 17, 2025

Thanks for making the requested changes!

@savannahostrowski: please review the changes made to this pull request.

savannahostrowski
savannahostrowski reviewed Sep 17, 2025
View reviewed changes
Copy link
Member

@savannahostrowski savannahostrowski left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@savannahostrowski savannahostrowski savannahostrowski approved these changes

@emmatyping emmatyping emmatyping left review comments

@diegorusso diegorusso diegorusso approved these changes

@brandtbucher brandtbucher Awaiting requested review from brandtbucher brandtbucher is a code owner

Assignees
No one assigned
Labels
awaiting merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@danigm @brandtbucher @savannahostrowski @hugovk @diegorusso @emmatyping