GeezerVison

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 skip news label instead.

Rebased and added documentation to the mentioned README. Is there anything more I can do here to get it merged?

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.

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.

activating MSVC via vcvars.bat will put LLVM tools on the PATH

I think this is the same problem I've faced in #141967 (comment), so thanks for doing this!

I've noticed there, that e.g. running

build.bat --experimental-jit --tail-call-interp "/p:PlatformToolset=ClangCL" "/p:LLVMInstallDir=<my-path-to>\llvm-21.1.4.0" "/p:LLVMToolsVersion=21"

will smuggle in the include path of the clang specified on the command line in front of the environment variable

INCLUDE=<my-path-to>\llvm-21.1.4.0\lib\clang\21\include;<other include paths>

which the hard coded (and maybe different) clang version used in the jit builds doesn't like depending on the version mismatch ...

LLVMInstallDir is the variable documented here https://learn.microsoft.com/en-us/cpp/build/clang-support-msbuild?view=msvc-170 which we already use at various places like

Does that mean that we have to specify the additional LLVM_TOOLS_INSTALL_DIR variable to let the jit build pick up the same clang that is used for building cPython?

Does that mean that we have to specify the additional LLVM_TOOLS_INSTALL_DIR variable to let the jit build pick up the same clang that is used for building cPython?

There's no strict requirement for make/configure variables to match pcbuild.proj variables, but if it's specified as an environment variable then we should probably detect if it's set and copy it over the actual variable. That's one line in python.props. I wouldn't bother updating all the references, just do a read in one place.

IIUC, you mean setting LLVMInstallDir and LLVMToolsVersion based on LLVM_TOOLS_INSTALL_DIR and LLVM_VERSION?

I think it might be "too late", since AFAIR internally this is used by MS when preparing the environment according to "their" variable names, but we'll see.

What's definitely easy: just use the MS variable names here

    <Exec Command='$(PythonForBuild) "$(PySourcePath)Tools\jit\build.py" $(JITArgs) --output-dir "$(GeneratedJitStencilsDir)" --pyconfig-dir "$(PySourcePath)PC" --llvm-version="$(LLVM_VERSION)" --llvm-tools-install-dir="$(LLVM_TOOLS_INSTALL_DIR)"'/>

because, as you said

There's no strict requirement for make/configure variables

and just update the jit readme accordingly if we cannot map the variables easily "early" enough ...

Or we do it in build.bat. Let's see ...

Any file that sets PlatformToolset is loaded early enough to influence all the default settings in MSBuild, so it can certainly be set there. I believe that's only python.props, though it might be part/all of pyproject.props as well, but I think this is a better setting for python.props along with the other properties relating to external dependencies.

@zooba @chris-eibl I'm a bit unsure what I need to do here to get this merged. Is there anything from your side that I should address in my code for windows builds?

First I thought we have these choices:

1. do nothing, i.e. allow people specifying LLVMInstallDir the "regular" way to determine the clang-cl used for compiling Python and LLVM_TOOLS_INSTALL_DIR to specify the clang used for the jit stencils. With the potential drawback, that those two might interfere like I've mentioned above. It doesn't always happen and depends on the "compatibility" of the clang versions, so I thought most probably we should prohibit that (and don't force users to specify the same args twice).
2. use just LLVMInstallDir and LLVMToolsVersion, i.e. different variables than on configure based builds
3. if LLVM_TOOLS_INSTALL_DIR is specified, override LLVMInstallDir. Likewise for LLVM_VERSION / LLVMToolsVersion.
4. if LLVMInstallDir is given, override LLVM_TOOLS_INSTALL_DIR. Likewise for LLVM_VERSION / LLVMToolsVersion.

I've first tried option 4 via adding

<LLVM_TOOLS_INSTALL_DIR Condition="$(LLVMInstallDir) != '' and $(LLVM_TOOLS_INSTALL_DIR) == ''">$(LLVMInstallDir)</LLVM_TOOLS_INSTALL_DIR>
<LLVM_VERSION Condition="$(LLVMToolsVersion) != '' and $(LLVM_VERSION) == ''">$(LLVMToolsVersion)</LLVM_VERSION>

to

This would have been my preferred choice, because then we would not have to "teach something new" and could have the same variables on Windows / Linux.

However, if during installing of Visual Studio the bundled clang-cl is chosen to be installed, it turned out that LLVM_VERSION is always set to this bundled version, even if doing just a regular MSVC build. So option 2 and 4 are out ...

Implementing option 3 is easy, too: just add

<LLVMInstallDir Condition="$(LLVM_TOOLS_INSTALL_DIR) != ''">$(LLVM_TOOLS_INSTALL_DIR)</LLVMInstallDir>
<LLVMToolsVersion Condition="$(LLVM_VERSION) != ''">$(LLVM_VERSION)</LLVMToolsVersion>

after

Now we'd have to document that when overriding the clang used to build the jit stencils will chose that clang for building Python, too, if built with "/p:PlatformToolset=ClangCL".

So option 1 gives more flexibility, but needs (?) a warning and "specifying the same thing twice" might feel like a wart.
Option 3 needs even more documentation?

I am unsure what to do best, but considering the jit is maybe no longer experimental, we should care to find a good solution here?

I am unsure what to do best, but considering the jit is maybe no longer experimental, we should care to find a good solution here?

The JIT is still experimental, but Python build flags and in general build.bat flags are bound by backwards compatibility, so when adding things we have to treat it as non-experimental.

Just checking in here, is there anything else required for this one?

Sorry, seems I've stalled this. IMHO we can handle the "clang version to build python" vs "clang version to build the jit" quirk in a follow-up PR? Especially since the first seems to be niche on Windows?

I think that's reasonable but I'd defer to @zooba on how he feels about that. I also don't want the follow up to get lost in the ether.

I think that's reasonable but I'd defer to @zooba on how he feels about that

I've furthermore tested this PR now locally successfully, too, in addition to my initial review(s).
@xhochy Thanks for this, nice improvement!
@zooba: any objections to merging this?

I also don't want the follow up to get lost in the ether.

I've created #146210.

IMHO we can handle the "clang version to build python" vs "clang version to build the jit" quirk in a follow-up PR?

I'm not totally up on all the nuance here, but provided it works to regen explicitly with one version and then build without it automatically regenerating, this should be a non-issue (perhaps a bit annoying for someone who wants to work on the JIT on Windows and also compile with a different version of Clang, having to switch environment variables, but irrelevant for someone compiling with the same version or with MSVC even if they're regenerating the JIT stencils)?

The change itself looks fine to me. It just uses the environment variables directly (or overridden as /p: options to MSBuild), which is the "natural" way.