classification
Title: Extend MSVCCompiler class to respect environment variables
Type: enhancement Stage: resolved
Components: Distutils, Windows Versions: Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: Nosy List: christopher.hogan, dstufft, eric.araujo, paul.moore, r.david.murray, rohitjamuar, steve.dower, tim.golden, zach.ware
Priority: normal Keywords: patch

Created on 2016-04-28 18:05 by rohitjamuar, last changed 2021-02-03 18:21 by steve.dower. This issue is now closed.

Files
File name Uploaded Description Edit
distutils_patch_27.patch rohitjamuar, 2016-05-04 21:15 Patch for CPython-2.7
distutils_patch_master.patch rohitjamuar, 2016-12-13 23:16 Patch for master branch
Messages (14)
msg264439 - (view) Author: Rohit Jamuar (rohitjamuar) * Date: 2016-04-28 18:05
The UnixCompiler class respects flags (CC, LD, AR, CFLAGS and LDFLAGS) set to the environment, whereas MSVCCompiler class does not. This change allows building CPython and any module that invokes distutils to utilize flags and executables that have been set to the environment. Inclusion of this change would ensure MSVCCompiler's behavior to be same as that of UnixCompiler and would also allow using a different set of compiler / linker / archiver, on Windows, without having the necessity for implementing separate compiler classes - using environment variables it should be possible to use a separate set of build executables - for example icl, clang, etc.
msg264448 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2016-04-28 20:25
I can't seem to get the patch to apply, could you please regenerate it against a fresh checkout of https://hg.python.org/cpython#default (or 'master' of github.com/python/cpython)?

This looks fairly straightforward, but I'm far from an expert on distutils :).  I can see issues arising from attempts to use a compiler/linker/etc. that doesn't recognize options that we assume are going to cl/link/etc., but I'm not sure how big a deal that is: I'd be inclined to say that if you're trying to tell distutils where cl is, don't get mad when what you told it doesn't work.

Also, I don't think Lib/distutils/msvccompiler.py is used anymore, but rather Lib/distutils/_msvccompiler.py in 3.5+.

I'm generally in favor of this, but would feel much better about it with buy-in from Steve.

There has also been mention of creating a new ICCCompiler class, but I'm not sure whether that has gotten anywhere.
msg264449 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-04-28 20:55
I'm neutral about applying it to 2.7, though I can see the value in doing so.

For 3.5 it definitely has to modify _msvccompiler - the other two modules are just left behind to distract people apparently :) (but actually in case someone accidentally(?) too a direct dependency on an undocumented class).

One thing I'd like to see is an environment variable to switch it on - probably DISTUTILS_USE_SDK is fine for this. In the default case, we know exactly where we are looking for the tools, and so we know what they are called. If you're intending to override this completely, you probably need to override everything, and DISTUTILS_USE_SDK is the flag for this.
msg264672 - (view) Author: Rohit Jamuar (rohitjamuar) * Date: 2016-05-02 21:28
Just so that I understand it clearly - Inside MSVCCompiler class (in _msvccompiler.py / msvccompiler.py / msvc9compiler.py ), current implementation of find_exe() finds compiler / linker / ... after parsing PATH. Should the changes be so, that if DISTUTILS_USE_SDK is set to the environment, the values set to CC / AR / LD, etc. are used verbatim? Or did you mean to say, that just as CC, LD and AR are being read from the environment, the same way rc.exe, mc.exe and mt.exe should be read as well, in case DISTUTILS_USE_SDK is set to the environment?
msg264755 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-05-03 18:50
I was thinking more like this:

if DISTUTILS_USE_SDK:
    assume PATH is configured
    execute os.getenv("CC", "cl.exe") directly
else:
    find VS in the registry
    execute "cl.exe" etc.

The only change here from the current situation is preferring the CC variable in the former case when it is set. When we look up VS in the registry, we know what executable we are going to find and overriding them via the environment is only going to cause issues/vulnerabilities.
msg264864 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2016-05-04 21:21
(The version field is for the whole issue, not per patch.)
msg266928 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2016-06-02 19:23
The patch looks fine to me, but frankly I'm a bit scared to commit anything to distutils :).  It would be nice to have tests for this.

I'm also not sure if we can backport this to 2.7 and 3.5: this is pretty clearly a new feature, but dips its toes into the grey area of "supporting new platforms".  Anyone else have any thoughts?
msg267033 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-06-03 04:42
I still want the behavior I described, since there's no value in overriding just the executable name but not the rest of the path.

For 2.7 I think this'll help with long term maintainability enough to be the Right Thing. For 3.5 I'm not as sure, but it's probably worth keeping the implementations consistent as long as we can justify it.
msg282253 - (view) Author: Rohit Jamuar (rohitjamuar) * Date: 2016-12-02 19:56
Bump! The changes, that Steve was asking for, have been incorporated. If something was missed, please inform. Otherwise, could this be merged?
msg283036 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2016-12-12 19:03
Steve, does the latest patch look good to you?
msg283065 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-12-13 00:24
I'd rather only take the change to _msvccompiler and not the ones that are there for historical interest (a.k.a. backwards compatibility with people who never expect internal implementation details to change).

If any tools break because they're using the wrong compiler class, please file a bug against them.

I didn't check the 2.7 file, but the one for master is fine.
msg286887 - (view) Author: Rohit Jamuar (rohitjamuar) * Date: 2017-02-03 20:22
Steve, I've limited the changes to _msvccompiler for the master-branch's patch. Is it alright?
msg287010 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2017-02-04 23:54
Taking another look at the patch, I'm not real keen on the C/LDFLAGS section. I think if we want to support setting these, we should fully override the default settings (otherwise you can't specify certain options that are in the defaults), and avoid splitting them (which probably means changing the spawn() call to not quote arguments - right now the .split() logic is just wrong).

I'm sorry this is tough to get together - the distutils compilers are really fragile, which is why we would really rather migrate people off them onto some other build framework. Right now though, that framework doesn't exist. (One potential starting point is my msbuildcompiler at https://github.com/zooba/pyfindvs/tree/master/pyfindvs/msbuildcompiler will generate a .vcxproj file and build it, which allows much more reliable behaviour for options and detecting installs.)
msg386353 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-02-03 18:21
Distutils is now deprecated (see PEP 632) and all tagged issues are being closed. From now until removal, only release blocking issues will be considered for distutils.

If this issue does not relate to distutils, please remove the component and reopen it. If you believe it still requires a fix, most likely the issue should be re-reported at https://github.com/pypa/setuptools
History
Date User Action Args
2021-02-03 18:21:25steve.dowersetstatus: open -> closed
resolution: out of date
messages: + msg386353

stage: patch review -> resolved
2017-02-04 23:54:50steve.dowersetmessages: + msg287010
2017-02-03 20:22:26rohitjamuarsetmessages: + msg286887
2016-12-13 23:16:45rohitjamuarsetfiles: + distutils_patch_master.patch
2016-12-13 23:15:45rohitjamuarsetfiles: - distutils_patch_master.patch
2016-12-13 22:30:50rohitjamuarsetfiles: + distutils_patch_master.patch
2016-12-13 22:30:03rohitjamuarsetfiles: - distutils_patch_master.patch
2016-12-13 00:24:07steve.dowersetmessages: + msg283065
2016-12-12 19:03:00zach.waresetmessages: + msg283036
2016-12-02 19:56:17rohitjamuarsetmessages: + msg282253
2016-06-03 04:42:04steve.dowersetmessages: + msg267033
2016-06-02 19:23:39zach.waresetmessages: + msg266928
2016-05-04 21:21:19zach.waresetmessages: + msg264864
versions: + Python 3.5, Python 3.6
2016-05-04 21:15:54rohitjamuarsetfiles: + distutils_patch_27.patch
versions: + Python 2.7, - Python 3.5, Python 3.6
2016-05-04 21:15:28rohitjamuarsetfiles: + distutils_patch_master.patch
versions: + Python 3.6, - Python 2.7
2016-05-04 21:13:59rohitjamuarsetfiles: - msvc_respect_env_flags.patch
2016-05-03 18:50:29steve.dowersetmessages: + msg264755
2016-05-02 21:28:45rohitjamuarsetmessages: + msg264672
2016-04-28 20:55:25steve.dowersetmessages: + msg264449
2016-04-28 20:26:00zach.waresetnosy: + christopher.hogan
2016-04-28 20:25:25zach.waresetnosy: + paul.moore, tim.golden, steve.dower
messages: + msg264448

components: + Windows
stage: patch review
2016-04-28 18:05:19rohitjamuarcreate