All commit authors signed the Contributor License Agreement.

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply theskip 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 phraseI 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 herehttps://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 additionalLLVM_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 additionalLLVM_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 inpython.props. I wouldn't bother updating all the references, just do a read in one place.

IIUC, you mean settingLLVMInstallDir andLLVMToolsVersion based onLLVM_TOOLS_INSTALL_DIR andLLVM_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 inbuild.bat. Let's see ...

Any file that setsPlatformToolset is loaded early enough to influence all the default settings in MSBuild, so it can certainly be set there. I believe that's onlypython.props, though it might be part/all ofpyproject.props as well, but I think this is a better setting forpython.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 specifyingLLVMInstallDir the "regular" way to determine the clang-cl used for compiling Python andLLVM_TOOLS_INSTALL_DIR to specify the clang used for the jit stencils. With the potential drawback, that those two might interfere like I'vementioned 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 justLLVMInstallDir andLLVMToolsVersion, i.e. different variables than on configure based builds
3. ifLLVM_TOOLS_INSTALL_DIR is specified, overrideLLVMInstallDir. Likewise forLLVM_VERSION /LLVMToolsVersion.
4. ifLLVMInstallDir is given, overrideLLVM_TOOLS_INSTALL_DIR. Likewise forLLVM_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 thatLLVM_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.