gh-138451: Support custom LLVM installation path #138452
Conversation
Add support for explicitly defined LLVM installation location. This is necessary as MSVC now comes with its own LLVM installation and activating MSVC via vcvars.bat will put LLVM tools on the `PATH` before the local ones.
xhochy
requested review from
brandtbucher,
savannahostrowski and
diegorusso
as code owners
|
All commit authors signed the Contributor License Agreement. |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Tools/jit/_llvm.py
Outdated
| async def _find_tool(tool: str, *, echo: bool = False) -> str | None: | ||
| # Explicitly defined LLVM installation location | ||
| if (llvm_root := os.getenv("LLVM_ROOT")) is not None: | ||
| path = os.path.join(llvm_root, "bin", tool) |
There was a problem hiding this comment.
Are all LLVM installation folders guaranteed to have the tool located in the $LLVM_ROOT/bin directory? I think it's somewhat risky to assume that.
There was a problem hiding this comment.
Actually there is a LLVM_TOOLS_INSTALL_DIR in the CMakeLists to customize this: https://github.com/llvm/llvm-project/blob/74ec38fad0a1289f936e5388fa8bbe74653c55d9/llvm/CMakeLists.txt#L494
I should probably use that instead?
There was a problem hiding this comment.
Is LLVM_TOOLS_INSTALL_DIR (or LLVM_ROOT for that matter) documented anywhere?
It seems like an env var used for building/installing LLVM itself, not necessarily something meant to be used after-the-fact by people trying to find LLVM. Or am I misunderstanding?
Tools/jit/_llvm.py
Outdated
| @_async_cache | ||
| async def _find_tool(tool: str, *, echo: bool = False) -> str | None: | ||
| # Explicitly defined LLVM installation location | ||
| if (llvm_tools_dir := os.getenv("LLVM_TOOLS_INSTALL_DIR")) is not None: |
There was a problem hiding this comment.
Should we document this somewhere? If it gets a NEWS entry, then we probably have (or need) a list of environment variables that may need to be set when building.
I'd expect the list to be in the devguide, really, which is a different repo. But do we have one in the main repo?
There was a problem hiding this comment.
Also:
| if (llvm_tools_dir := os.getenv("LLVM_TOOLS_INSTALL_DIR")) is not None: | |
| if llvm_tools_dir := os.getenv("LLVM_TOOLS_INSTALL_DIR")): |
We shouldn't join to an empty value either (since that's interchangeable with "not set"). And possibly we should make sure it's an absolute path as well, though that may not matter so much.
There was a problem hiding this comment.
Considering the JIT is still experimental, I don't think we should document this in the CPython docs. I agree that it should be in the devguide.
There was a problem hiding this comment.
As I did not know about the devguide before this review: Where should I add something? On first sight there was no section obvious to me.
There was a problem hiding this comment.
I assume it's somewhere on this page, but to be honest I didn't read the whole thing: https://github.com/python/devguide/blob/main/getting-started/setup-building.rst
Possibly we need a new section here anyway for building the JIT? @savannahostrowski @brandtbucher are there JIT-specific build docs somewhere to document a relevant environment variable?
There was a problem hiding this comment.
We have docs for building in https://github.com/python/cpython/blob/main/Tools/jit/README.md#building. We'd probably want to update this for now.
Co-authored-by: Steve Dower <steve.dower@microsoft.com>
Fixes #138451
Add support for explicitly defined LLVM installation location.
This is necessary as MSVC now comes with its own LLVM installation and activating MSVC via vcvars.bat will put LLVM tools on the
PATHbefore the local ones.You can see this in usage in conda-forge's latest Python 3.13 build: conda-forge/python-feedstock#807