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) * |
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) * |
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) * |
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) * |
Date: 2016-05-04 21:21 |
(The version field is for the whole issue, not per patch.)
|
msg266928 - (view) |
Author: Zachary Ware (zach.ware) * |
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) * |
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) * |
Date: 2016-12-12 19:03 |
Steve, does the latest patch look good to you?
|
msg283065 - (view) |
Author: Steve Dower (steve.dower) * |
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) * |
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) * |
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
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:30 | admin | set | github: 71063 |
2021-02-03 18:21:25 | steve.dower | set | status: open -> closed resolution: out of date messages:
+ msg386353
stage: patch review -> resolved |
2017-02-04 23:54:50 | steve.dower | set | messages:
+ msg287010 |
2017-02-03 20:22:26 | rohitjamuar | set | messages:
+ msg286887 |
2016-12-13 23:16:45 | rohitjamuar | set | files:
+ distutils_patch_master.patch |
2016-12-13 23:15:45 | rohitjamuar | set | files:
- distutils_patch_master.patch |
2016-12-13 22:30:50 | rohitjamuar | set | files:
+ distutils_patch_master.patch |
2016-12-13 22:30:03 | rohitjamuar | set | files:
- distutils_patch_master.patch |
2016-12-13 00:24:07 | steve.dower | set | messages:
+ msg283065 |
2016-12-12 19:03:00 | zach.ware | set | messages:
+ msg283036 |
2016-12-02 19:56:17 | rohitjamuar | set | messages:
+ msg282253 |
2016-06-03 04:42:04 | steve.dower | set | messages:
+ msg267033 |
2016-06-02 19:23:39 | zach.ware | set | messages:
+ msg266928 |
2016-05-04 21:21:19 | zach.ware | set | messages:
+ msg264864 versions:
+ Python 3.5, Python 3.6 |
2016-05-04 21:15:54 | rohitjamuar | set | files:
+ distutils_patch_27.patch versions:
+ Python 2.7, - Python 3.5, Python 3.6 |
2016-05-04 21:15:28 | rohitjamuar | set | files:
+ distutils_patch_master.patch versions:
+ Python 3.6, - Python 2.7 |
2016-05-04 21:13:59 | rohitjamuar | set | files:
- msvc_respect_env_flags.patch |
2016-05-03 18:50:29 | steve.dower | set | messages:
+ msg264755 |
2016-05-02 21:28:45 | rohitjamuar | set | messages:
+ msg264672 |
2016-04-28 20:55:25 | steve.dower | set | messages:
+ msg264449 |
2016-04-28 20:26:00 | zach.ware | set | nosy:
+ christopher.hogan
|
2016-04-28 20:25:25 | zach.ware | set | nosy:
+ paul.moore, tim.golden, steve.dower messages:
+ msg264448
components:
+ Windows stage: patch review |
2016-04-28 18:05:19 | rohitjamuar | create | |