gh-146475: Block Apple LLVM 21 from passing JIT tool version check#146478
gh-146475: Block Apple LLVM 21 from passing JIT tool version check#146478ashutoshk314 wants to merge 2 commits intopython:mainfrom
Conversation
ashutoshk314
requested review from
brandtbucher,
diegorusso and
savannahostrowski
as code owners
|
The following commit authors need to sign 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f29015ef58
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) -> bool: | ||
| output = await _run(name, ["--version"], echo=echo) | ||
| _llvm_version_pattern = re.compile(rf"version\s+{llvm_version}\.\d+\.\d+\S*\s+") | ||
| _llvm_version_pattern = re.compile(rf"(?<!Apple )LLVM version\s+{llvm_version}\.\d+\.\d+\S*\s+") |
There was a problem hiding this comment.
_check_tool_version() now only matches outputs containing LLVM version, but the JIT tool lookup also validates clang in _find_tool(), and upstream clang --version reports clang version 21.x.y (not LLVM version). In environments with LLVM 21 clang, this makes the version check fail and run("clang", ...) eventually raise Can't find clang-21!, breaking JIT stencil builds even with a correct compiler installed.
Useful? React with 👍 / 👎.
| @@ -59,7 +59,7 @@ async def _check_tool_version( | |||
| name: str, llvm_version: str, *, echo: bool = False | |||
| ) -> bool: | |||
| output = await _run(name, ["--version"], echo=echo) | |||
| _llvm_version_pattern = re.compile(rf"version\s+{llvm_version}\.\d+\.\d+\S*\s+") | |||
| _llvm_version_pattern = re.compile(rf"(?<!Apple )LLVM version\s+{llvm_version}\.\d+\.\d+\S*\s+") | |||
There was a problem hiding this comment.
On my mac I had to add LLVM and clang to match the string. Can you please check?
| _llvm_version_pattern = re.compile(rf"(?<!Apple )LLVM version\s+{llvm_version}\.\d+\.\d+\S*\s+") | |
| _llvm_version_pattern = re.compile(rf"(?<!Apple )(LLVM|clang) version\s+{llvm_version}\.\d+\.\d+\S*\s+") |
There was a problem hiding this comment.
$ /Library/Developer/CommandLineTools/usr/bin/llvm-objdump --version
Apple LLVM version 21.0.0
Optimized build.
...
$ /Library/Developer/CommandLineTools/usr/bin/clang --version
Apple clang version 21.0.0 (clang-2100.0.123.102)
Target: arm64-apple-darwin25.4.0
...
There was a problem hiding this comment.
Yep, I'm on MacOS as well and checked that we do need clang in the regex.
|
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 |
savannahostrowski
left a comment
savannahostrowski
left a comment
There was a problem hiding this comment.
@ashutoshk314 Can you please sign the CLA?
@Fidget-Spinner I saw the issue but is Apple LLVM 21 incompatible because their fork has diverged/is missing features/etc? What about it makes it incompatible?
| @@ -59,7 +59,7 @@ async def _check_tool_version( | |||
| name: str, llvm_version: str, *, echo: bool = False | |||
| ) -> bool: | |||
| output = await _run(name, ["--version"], echo=echo) | |||
| _llvm_version_pattern = re.compile(rf"version\s+{llvm_version}\.\d+\.\d+\S*\s+") | |||
| _llvm_version_pattern = re.compile(rf"(?<!Apple )LLVM version\s+{llvm_version}\.\d+\.\d+\S*\s+") | |||
There was a problem hiding this comment.
Yep, I'm on MacOS as well and checked that we do need clang in the regex.
| @@ -0,0 +1,2 @@ | |||
| Block Apple LLVM 21 from passing the JIT tool version check, as the JIT is | |||
There was a problem hiding this comment.
We should include at least some details about why it's incompatible IMO.
IIUC, it produces different relocations than normal clang. I can't confirm, as I don't have a macOS AArch64 machine. |
|
@Fidget-Spinner The reason I'm asking is because I was able to successfully build with the JIT enabled and have all the tests pass? I'm a bit confused here on what specifically doesn't work. Maybe @kumaraditya303 can shed more light on why this was an issue? |
During the compilation with apple clang, I was getting the above error. I haven't looked at the root cause of this. |
6 participants
Fixes #146475
The JIT is incompatible with Apple LLVM 21, but the tool version
check was passing it through. This change blocks Apple LLVM 21
from passing the check.