New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update distutils.msvccompiler for VC14 #68158
Comments
Because of the compiler change, we need to rework the detection of MSVC for Python 3.5. I took the opportunity to clean up the entire module and remove msvc9compiler, and updated the tests. |
Adding Paul Moore as he's essentially the intersection between distutils & Windows |
Hmm, I thought everyone was too scared to change distutils. Brave man :-) More seriously, I'm not sure I can comment usefully, I don't really understand any of this code, and the changes are pretty big. I guess the biggest risk is that someone directly references the changed code in a custom distutils command. Hmm, I just checked the numpy sources. It looks like numpy/distutils/command/config.py references msvc9compiler. And there's lots of references to msvccompiler. I'd suggest you confirm if numpy (and probably scipy) still build with these changes. |
Numpy uses a fork of distutils anyway, and currently nobody builds against 3.5 because of the compiler change. There are all sorts of monkey patches out there against this module and msvc9compiler (I've written some) and those should be unnecessary now, so I want then to break. I also don't see any reason for distutils to continue supporting old compilers, since 3.5 requires VC14, but maybe I'm wrong about that? |
Sounds fair enough to me. I guess you have most of the bases covered, and AFAIK, we've never actually supported building extensions with anything other than the compiler python.org uses, so that seems a reasonable view to take. Apart from the numpy/scipy guys, the only other person I've ever heard of doing major distutils subclassing is Marc-André Lemburg. If you haven't spoken to him yet, it might be worth getting his view. I've added him to the nosy list. For my part, though, I'm happy with this change. |
We do some monkey-patching on MSVCCompiler as well, but mostly because distutils doesn't define a good way to subclass or configure a compiler class. IMO, monkey patching is a really bad idea. distutils should be evolved to provide more proper extension mechanisms like the ones via distclass and cmdclass. That said, we only use the compilers that Python itself is compiled with for each version, so your change should work for us (modulo some fixes we'd have to do to support it). One detail you are not considering with your patch is that distutils can well be used to build other tools and those may need older VC versions. Couldn't you at least keep the old compiler support around in a separate file ? |
Is there a way for those users to specify the version that they want? I can flow that through from somewhere, and honestly the compiler arguments have not changed in a long time. The problem is detecting an installation. I have formalized the DISTUTILS_USE_SDK variable, so setting up the env with that will be more reliable than previously. Also, I'm interested in what other patches you might apply. This is the last time I really want to touch this, so it's the time to get things like that added. |
On 16.04.2015 15:18, Steve Dower wrote:
Indirectly, yes. There's the build_ext --compiler argument which then The compiler is then looked up in the ccompiler.compiler_class Adding new compiler classes is possible through monkey patching Not a nice design... exposing a proper registry API for the compiler On Unix, it is possible to specify the compiler using the CC env var.
We completely gave up on the automatic detection of the installation
Something we've needed is a standard way to extract the def get_msvc_paths():
and the only monkey-patching we do is to be able to modify if python_version < '2.4':
# VC6
MSVC_COMPILER_FLAGS = ['/O2', '/Gf', '/GB', '/GD', '/Ob2']
elif python_version < '2.6':
# VC7.1
MSVC_COMPILER_FLAGS = ['/O2', '/GF', '/GB', '/Ob2']
else:
# VC9
MSVC_COMPILER_FLAGS = ['/O2', '/GF', '/Ob2']
if hasattr(MSVCCompiler, 'initialize'):
# distutils 2.5.0 separates the initialization of the
# .compile_options out into a new method .initialize()
# remember old _initialize
old_MSVCCompiler_initialize = MSVCCompiler.initialize
def mx_msvccompiler_initialize(self, *args, **kws):
# Add our extra options
self.compile_options.extend(MSVC_COMPILER_FLAGS)
# "Install" new initialize
MSVCCompiler.initialize = mx_msvccompiler_initialize Would be great to a standard way to do this in the MSVCCompiler |
So it looks like what I should do is use the include_dirs and library_dirs members from CCompiler so they can be set through other properties. The extra arguments are passed into compile() as extra_preargs and extra_postargs, though I'm not entirely sure where they come from. I'd rather figure out how to use those than invent a new way of specifying them (FWIW, they've always been supported by the various msvccompiler implementations). |
On 17.04.2015 00:06, Steve Dower wrote:
I see that you're basically reading the include and lib It may be useful to add an official helper to the module to def get_vc_var(var):
return _get_vc_env(plat_spec).get(var)
Hmm, to hook into those, we'd have to find every single use of those I guess we'll then just continue to override the .initialize() call. |
Yeah, the basic functionality is no different, since there isn't really a better way to handle future versions of VS automatically. I'd rather not officially expose this stuff though. Maybe you could override the compiler creation code and return an alternative implementation? That will be most portable. |
On 17.04.2015 16:04, Steve Dower wrote:
I just had a look at our code and I think we can try a We'll have to experiment with that a bit, since I'm not sure whether Alternatively, we could use monkey-patching (again :-() to |
Made some updates to the patch to use the existing infrastructure for setting include and library paths, and to fix bdist_wininst. While it may be worth doing more significant restructuring to help people with overriding aspects of build, that's almost certainly something that would be better done in a separate build tool. This patch is really just focused on getting the bare minimum support (distutils) working with the changed compiler and some cleanup to make it easier to maintain. |
Any further comments on this? I'd like to get it in for beta (though I don't believe it's covered by feature freeze). |
Why are you removing the mcvs9compiler.py file when at the same time your are referencing it in the msvccompiler.py doc string ? --- a/Lib/distutils/msvccompiler.py
+++ b/Lib/distutils/msvccompiler.py
@@ -1,201 +1,120 @@
"""distutils.msvccompiler
Contains MSVCCompiler, an implementation of the abstract CCompiler class
-for the Microsoft Visual Studio.
+for Microsoft Visual Studio 2015.
+
+The module is compatible with VS 2015 and later. You can find legacy support
+for older versions in distutils.msvc9compiler and distutils.msvccompiler.
""" IMO, it would be better and more in line with the b/w aspects of distutils, to move the VS15 support into a new msvc14compiler.py That way we can maintain compatibility with existing code which uses stuff from those legacy modules. |
Simply because I didn't update the doc string :) I don't really want to put a version number on this file, since it isn't MSVC 14.0 specific - it's MSVC 14 and all future versions. We don't check the build version anymore, though get_build_version() is still there (hardcoded to 14.0) As you say, the existing modules are legacy, so I wonder what would be useful from them? They refer to compilers that aren't available and won't be useful for Python 3.5 to the extent that I'd really like to discourage using them. The modules themselves are undocumented and were not available on other platforms, so any code using them should be protecting themselves against ImportError already, and they're going to be broken anyway so they may as well break early. But my main question is why would you still need the old versions around, given that compatibility is already broken? |
On 11.05.2015 21:03, Steve Dower wrote:
In Python 2, distutils could be easily be used on earlier Python I don't know what the deal is for Python 3. Apart from that, it may be necessary to compile some parts of Note that VC9 was handled in the same way: it was added to the |
I guess we need a third opinion. For me, the subclasses of CCompiler are undocumented and not a guaranteed interface (people using them directly are consenting adults). They're also an eyesore, so if I can clean them up without breaking the CCompiler interface (or minor version upgrades) then I should. |
On 11.05.2015 23:20, Steve Dower wrote:
The distutils interface isn't documented in all details, FWIW: I don't see a problem with keeping implementations |
I agree with Steve, we shouldn't be constrained to preserve all the undocumented internals of distutils - doing that in the past is what has made distutils essentially unmaintainable. I don't think there's a concrete example of real code that will be broken by this change - if I follow the comments correctly, MAL's code will still work ("I guess we'll then just continue to override the .initialize() call"). Without a specific breakage that can't be fixed to work with the new code, I don't think this patch should be blocked just because people might be using the old internals. |
On 11.05.2015 23:50, Paul Moore wrote:
This is not about our code. We can work around all this. The point is that in general, we don't break things in Python just In this case, I don't see a need to break things to add support for |
Things are already 'broken' for the new compiler version, so Python won't build properly with older versions of VC anymore (there are a few more changes, like removing _PyVerify_fd, that will make this even less likely - the new CRT is for too incompatible with the old one, though it's much more compatible with other OSs). If it's a big deal, I'll add the new compiler class as _msvccompiler.py and leave the old ones there as legacy code. That will save us from this discussion next time and avoid breaking people immediately (though there's almost certainly going to be subtle issues for them...) |
New patch. I've renamed the new class _msvccompiler and changed ccompiler to use it instead of the old ones. The old ones are still there to avoid breaking people who were using them directly, and the old tests are still there too. Personally, I expect subtle breakages in actual use (that we couldn't possible test for reliably) and I don't like deliberately leaving cruft behind, but it seems like the best way to move forward without breaking people. Would adding a deprecation warning to the orphaned modules make sense? They should never be imported in normal use. |
We don't strictly need this for beta 1, but I'd like to get it in. Final call for feedback (I'll probably hold off checking in until tomorrow morning, ~18 hours from now) |
New changeset b2ee6206fa5e by Steve Dower in branch 'default': |
b2ee6206fa5e broke every non-Windows buildbot. http://buildbot.python.org/all/builders/AMD64%20Snow%20Leop%203.x/builds/3229/steps/test/logs/stdio |
New changeset 32e6123f9f8c by Steve Dower in branch 'default': |
I was away the last few days, so just found the changes now. IMO, it's a good idea to use a new module for the new compiler, but don't think it's a good idea to make the whole module private, since this implicitly disallows sub-classing the compiler class going forward, which people will need to do sooner or later. Why not rename the module to msvc14compiler (or some other public name) instead ? |
I understood it only disallowed complaining about breaking changes without a deprecation cycle :) I'm sorry I didn't realize you were away. If you have examples of how subclassing this class (and not just CCompiler) is useful and does something that can't be done through the existing interface, then we'll have something to discuss. From past experiences, I now prefer to default to "disallow" inheritance by default, as it isn't a breaking change to allow it again in the future but you can't go the other way. |
If the name is changed I'd like to see something that doesn't reflect the msvc version, as my understanding is that from now on the old compatibility issues are gone. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: