GeezerVison

@mdboom I'm baffled by the error in the CI. By any chance do you know how to fix it? https://github.com/python/cpython/actions/runs/13287244377/job/37098802007?pr=130040

@mdboom I'm baffled by the error in the CI. By any chance do you know how to fix it? https://github.com/python/cpython/actions/runs/13287244377/job/37098802007?pr=130040

I can try poking at this locally a bit later today, but my first guess is that it's because the equals sign is getting swallowed here:

/p:PlatformToolset clangcl

...so clangcl is just seen as another project name.

I don't really understand the quoting rules for PowerShell -- maybe "/p:PlatformToolset\=clangcl" is needed?

I think I ran into this same problem in bench-runner, and I ultimately gave up and used cmd rather than PowerShell, which you can do by putting shell: cmd above the run: ... in the yml file. See here for how I solved a similar problem in bench_runner.

That makes sense. Thanks. Applied the change to use cmd instead!

@chris-eibl I guess we're blocked on getting patches to make clang-cl build CPython. Currently there's a few build errors which I assume you patched out on your branch? Do you mind upstreaming those patches? I can review them.

Nevermind, I will try to patch them into this PR as well, so that we can do an integration test.

I don't really understand the quoting rules for PowerShell -- maybe "/p:PlatformToolset\=clangcl" is needed?

Yeah, see this part of build.bat -h:

If the argument contains an '=', the
entire argument must be quoted (e.g. `build.bat "/p:PlatformToolset=v141"`).

Had a quick look at the PR and everything looks fine to me.

The build log of your latest commit 7f91920 shows

  Performing Custom Build Tools
   Assembling: D:\a\cpython\cpython\externals\mpdecimal-4.0.0\libmpdec\vcdiv64.asm
  error: symbol 'AsyncioDebug' is already defined

I have never seen that error before.

For sure not during assembling, must come out of _asynciomodule.c:

GENERATE_DEBUG_SECTION(AsyncioDebug, Py_AsyncioModuleDebugOffsets AsyncioDebug)

That's the only occurence I've found.

Ah - you've found it yourself while I was typing :)

Interestingly: I cannot reproduce locally for the failing commit 7f91920 using:

build.bat --tail-call-interp -p x64 -c Release "/p:PlatformToolset=clangcl" "/p:PreferredToolArchitecture=x64"

with Visual Studio 2022 17.13.0 Preview 5.0, which ships with cl.exe 19.43.34808 and clang 19.1.1.

Let's see whether your latest commit fixes CI - looks promising :)

Yeah, see this part of build.bat -h:

If the argument contains an '=', the
entire argument must be quoted (e.g. `build.bat "/p:PlatformToolset=v141"`).

I'm pretty sure this advice assumes using cmd, not PowerShell (which is the default on Github Actions). Overriding the shell used on Github Actions seems to be the easy solution. The harder solution is figuring out PowerShell's escaping rules...

with Visual Studio 2022 17.13.0 Preview 5.0, which ships with cl.exe 19.43.34808 and clang 19.1.1.

Let's see whether your latest commit fixes CI - looks promising :)

That didn't fix it :(.

Ups - still the same error - very weird. No idea atm - sorry ...

Unfortunately I cannot reproduce locally. Have a different clang version, though. Did also use clang 19.1.6 a lot.

Will fire up a build for this commit ...

Update:
Commit 7f91920 builds for me with clang 19.1.6, too:

build.bat --tail-call-interp -p x64 -c Release "/p:PlatformToolset=clangcl" "/p:LLVMToolsVersion=19.1.6" "/p:LLVMInstallDir=clang+llvm-19.1.6-x86_64-pc-windows-msvc"

Ah.. I think I forgot to add parentheses :) my bad

Nevermind, that didn't fix it. Now that I look at the macros closer, it shouldn't fix it either.

Really strange why that happens on CI. Cannot reproduce with clang 19.1.1 and 19.1.6.
The CI seems to use 19.1.5 - but hard to believe this is the culprit.

Those macros have been touched recently #129223.
Yet, I never had an issue with clang-cl so far (and still have not).

Fixed it on CI!

@zooba gentle ping for a review please. Thanks again for reviewing that other PR on getting PGO working on clang-cl. This should be relatively tame in comparison :).

Looks fine to me.

I hope we can justify (one day) taking options out of build.bat without deprecation periods and warnings. I think we probably can. In any case, this PR isn't the cause of sprawling options, so no harm in merging it.

@zooba: I understand. But doing that options in build.bat is the only way to get it configurable without modifying/hardcoding it in pythoncore.vcxproj. Which I manually had to do when trying computed gotos builds in #130090, and thus always had a "dirty repo".

Would it be okay for you if I create a similar PR for computed gotos?

Or is there a better way?

Environment variables pass through into the build just fine, and are exposed as MSBuild properties (provided we don't immediately overwrite them). I have no problem (well, fewer problems) with enabling a feature by environment variable, it's just the batch file option that worries me.

The main difference is that if we remove the setting and it was only used by environment variable, the build will still succeed. If we remove a build.bat option, builds will break. So arguably a good dividing line is whether builds ought to break if/when we remove the option - most optimisations and experimental features aren't in that category, IMHO.

That would be fine with me, too. IIUC, e.g. just add

<PreprocessorDefinitions Condition="'$(HAVE_COMPUTED_GOTOS)' == 'true'">HAVE_COMPUTED_GOTOS;%(PreprocessorDefinitions)</PreprocessorDefinitions>

and leave it to the user to

• either add "/p:HAVE_COMPUTED_GOTOS=true" when invoking build.bat
• or just set the environment variable HAVE_COMPUTED_GOTOS=true before invoking build.bat

up to their liking.

Either way, build.bat does not have to "learn" all of that things. Might have been better for some of the existing switches, too - but it's too late for that. Maybe not for tailcall, since this is new?

And if it is really important to warn or fail if a variable does not make sense any longer, we could do something like

<Message Text="HAVE_COMPUTED_GOTOS are no longer supported" Condition="$(HAVE_COMPUTED_GOTOS) == "true" Importance="high" />

or

<Error Text="HAVE_COMPUTED_GOTOS are no longer supported" Condition="$(HAVE_COMPUTED_GOTOS) == "true" Code="1" />

Regarding documentation: shall this still be available via build.bat -h (still less clutter) or only via PCbuild/readme.txt?

PS: we even could do some meaningful defaults like enabling tail calling if not given and LLVMToolsVersion indicates clang-cl >= 19.x.x.

Likewise, we could enable cg if HAVE_COMPUTED_GOTOS is not given (or true) but disable it if HAVE_COMPUTED_GOTOS=false, etc ...

Ah, and the "switch" shall maybe be named WITH_COMPUTED_GOTOS in analogy to --with-computed-gotos in the configure script and the other --with things, where build.bat tries to mimic configure.

Naming is always hard :(

IIUC, ...

Yeah, though I'd test for != '' rather than a specific value. And it only really needs mentioning in PCbuild\README.txt, probably with a note that they are subject to change at any time.

I'm okay with meaningful defaults, but do be very careful about the checks. It's very easy to break unrelated builds, because basically everything gets evaluated all the time and there's not much in the way of error handling.