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:
> 
> Is there a way for those users to specify the version that they want?

Indirectly, yes. There's the build_ext --compiler argument which then
interfaces to ccompiler.new_compiler. In essence, you can specify
the compiler type.

The compiler is then looked up in the ccompiler.compiler_class
registry.

Adding new compiler classes is possible through monkey patching
(since the modules implementing them must be in the distutils package).

Not a nice design... exposing a proper registry API for the compiler
classes would be better, even if it's just via something like a
compilerclass dictionary in the setup() call.

On Unix, it is possible to specify the compiler using the CC env var.

> 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.

We completely gave up on the automatic detection of the installation
and only rely on the vcvars style batch files to set up things
correctly for each MS compiler version and then have distutils find it.

> 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.

Something we've needed is a standard way to extract the
lib and include paths used by the compiler:

def get_msvc_paths():

    """ Return a tuple (libpath, inclpath) defining the search
        paths for library files and include files that the MS VC++
        compiler uses per default.

        Both entries are lists of directories.

        Only available on Windows platforms with installed compiler.

    """

and the only monkey-patching we do is to be able to modify
the default compiler switches used:

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):

        apply(old_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
class or associated module.

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:
> 
> 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.

I see that you're basically reading the include and lib
dirs from the vc_env which essentially reads the vcvars
batch files, so that's no different than what we're
already doing.

It may be useful to add an official helper to the module to
run this extraction outside the compiler class, e.g.

def get_vc_var(var):
    return _get_vc_env(plat_spec).get(var)

> 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).

Hmm, to hook into those, we'd have to find every single use of those
methods and override them. Doesn't sound like a good way to do this,
if you want a few options extra with every single compile call.

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:
> 
> 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.

I just had a look at our code and I think we can try a
different approach which doesn't require monkey-patching: we're
using a CompilerSupportMixin for the build commands which has
a .prepare_compiler() method to set up the .compiler attribute
which is then used for compiling things.

We'll have to experiment with that a bit, since I'm not sure whether
this will catch all cases where distutils uses a compiler.

Alternatively, we could use monkey-patching (again :-() to
override ccompiler.new_compiler() and then apply the
fixes there.

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
which is then imported by msvccompiler.py at the end (much like this was done for msvc9compiler.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:
> 
> Steve Dower added the comment:
> 
> 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?

In Python 2, distutils could be easily be used on earlier Python
versions, simplifying the amount of work you'd need to do in your
setup.py to address differences between the various Python versions
you wanted to support.

I don't know what the deal is for Python 3.

Apart from that, it may be necessary to compile some parts of
C extensions with older compilers. Probably a rare case, but
then again: what do we gain by removing the old code ?

Note that VC9 was handled in the same way: it was added to the
set rather than replacing it. That was done in the Python 2
series, though, where the above guarantee was used as basis,
so things may be different for Python 3.

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:
> 
> 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.

The distutils interface isn't documented in all details,
so the rule of thumb by which everybody operates is that any
non-private symbol is part of the public API.

FWIW: I don't see a problem with keeping implementations
around for older MS VC versions. It's well possible that
someone might want to use them for creating a Python version
compiled with an older version of MS VC, e.g. in an
embedding scenario. And you can still have your new cleaned up
version override the default msvccompiler class.

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:
> 
> 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.

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
because we can, even if we don't need to.

In this case, I don't see a need to break things to add support for
a new compiler version, which is why I don't follow Steve's arguments.

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':
Issue #23970: Adds distutils._msvccompiler for new Visual Studio versions.
https://hg.python.org/cpython/rev/b2ee6206fa5e

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':
Issue #23970: Fixes bdist_wininst not working on non-Windows platform.
https://hg.python.org/cpython/rev/32e6123f9f8c

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.