GH-148047: Check early whether tail-calling is possible for MSVC builds on Windows#148036
GH-148047: Check early whether tail-calling is possible for MSVC builds on Windows#148036itamaro merged 5 commits intopython:mainfrom
Conversation
|
TSan https://github.com/python/cpython/actions/runs/23948631142/job/69850491565?pr=148036 failure for sure unreleated: this is a pure Windows change and would result in a build error on onsupported (Windows) hosts when using I've checked manually that I get all the expected errors for unsupported configurations. |
|
Please open a separate issue for this, rather than reusing the same closed issue. Thanks! |
itamaro
left a comment
There was a problem hiding this comment.
Overall looks good, thank you for this!
Misc/NEWS.d/next/Build/2026-04-03-20-09-46.gh-issue-148047.HE6iGK.rst
Outdated
Show resolved
Hide resolved
|
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 |
|
| <Error Text="MSVC supports tail-calling only for x64." | ||
| Condition="$(Platform) != 'x64'" /> | ||
| <Error Text="Platform toolset >= v145 is required for tail-calling." | ||
| Condition="$(PlatformToolset.Replace('v', '0')) < '145'" /> |
There was a problem hiding this comment.
We can't do this check - PlatformToolset is an arbitrary string, you can't assume that it's comparable to a number in any meaningful way.
| <Error Text="Platform toolset >= v145 is required for tail-calling." | ||
| Condition="$(PlatformToolset.Replace('v', '0')) < '145'" /> | ||
| <Error Text="MSVC requires optimization to be enabled for tail-calling." | ||
| Condition="$(Configuration) == 'Debug'" /> |
There was a problem hiding this comment.
Why would we do this? It makes it annoyingly hard to set tail calling enabled in my environment and then choose between debug/release builds.
A warning is fine, if you really want, but a debug build is inherently without optimisations, so there's no reason to remind users that that's what they're getting.
|
|
||
| <Target Name="_CheckTailCalling" BeforeTargets="PrepareForBuild" Condition="'$(UseTailCallInterp)' == 'true' and $(PlatformToolset) != 'ClangCL'"> | ||
| <Error Text="MSVC supports tail-calling only for x64." | ||
| Condition="$(Platform) != 'x64'" /> |
There was a problem hiding this comment.
Not convinced this needs to be an error - if/when MSVC gets support, they may want to test it on CPython themselves before shipping, and we're just going to block them.
As with Debug, make it a warning if you really want, but ultimately it's up to the user to decide whether they're enabling a feature or not. We shouldn't prevent users from doing what they want to do.
|
Really sorry @chris-eibl, but I'm going to revert this change. I think users can just let the compiler fail to build if it's not available. |
… for MSVC builds on Windows (python#148036)" This reverts commit cbd81d5.
|
Revert up at #148558 |
No worries, totally fine with me. That's why we have discussions and reviews. Sorry for the churn. |
|
No problem, nothing to apologise for! |
Rather than failing late when compiling e.g. a debug configuration
with hundreds of
lets fail early with an explicit error message for configurations that are not supported by MSVC.
This is a follow-up on #140513 / #140548.