Fixed build_ext assumption that cross-compilation on Windows always uses MSVCCompiler#399
Fixed build_ext assumption that cross-compilation on Windows always uses MSVCCompiler#399Avasam wants to merge 2 commits into
Conversation
| # late initialization of compiler even if they shouldn't...) | ||
| self.plat_name != get_platform() | ||
| # Initialization is a MSVCCompiler specific concept | ||
| and hasattr(self.compiler, "initialize") |
There was a problem hiding this comment.
I would've done
| and hasattr(self.compiler, "initialize") | |
| and isinstance(MSVCCompiler, self.compiler) |
But who knows if someone downstream added a custom non-MSVCCompiler with .initialize that relies on being called.
Or it could be considered so internal that it's fine to do the more "proper" check that also helps static checkers.
There was a problem hiding this comment.
I wonder if we shouldn't instead just have the base Compiler provide a degenerate initialize() method, allowing any Compiler to potentially implement it.
But what's really annoying is this other factor "people may rely on late initialization of compiler even if they shouldn't". That really deserves a better explanation. Looks like it was made 18 years ago (Avasam@f39783d#diff-08754ed5a9d7fff430bd6aa04a88d70fd4ef8972aecab7b07373c86271a7c07c) or maybe even earlier, as that's a bundle that seems to trace back to issue 2513 aka python/cpython#46765.
There was a problem hiding this comment.
I'm not fully convinced I'd add a dummy def initialize to base Compiler. Unless there's a demand for it. As it would also subtly affect downstream hasattr(self.compiler, "initialize") checks.
I think it's fine for the concept to be MSVC specific.
Sidenote: Funny how this comes full circle, mhammond being the author of that issue you linked, and I'm opening this PR for MinGW support in his pywin32 package
There was a problem hiding this comment.
I guess we don't need to deal with the 'cross-compiling' concern here. Let's implement the degenerate method to eliminate the one conditional.
There was a problem hiding this comment.
As it would also subtly affect downstream
hasattr(self.compiler, "initialize")checks
I checked and Setuptools doesn't rely on this interface. And if there are checks further downstream, they shouldn't be relying on the presence of class attributes for such behavior.
…or non-MSVC compilers build_ext.run() unconditionally called compiler.initialize() when cross-compiling on Windows, assuming an MSVCCompiler. With any other compiler (e.g. MinGW/msys2) this raised AttributeError. Rather than guard the call with hasattr(), give the base Compiler a no-op initialize() that non-MSVC compilers inherit; MSVCCompiler continues to override it. This also makes the method visible to static type checkers. The redundant os.name == 'nt' guard is dropped since initialize() is now safe on every compiler. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fixes the following error if providing
--plat-namewhen using msys2-mingw on Windows:This could've been caught with static type checking, but
attr-definedis currently disabled in mypy