| msg67387 - (view) |
Author: Jim Kleckner (jkleckner) |
Date: 2008-05-26 19:56 |
| I tracked down a testing failure for Cython tests on the Windows platform to be the result of how the Visual C environment variables are being detected. In the function, query_vcvarsall, the .bat file: "C:\Program Files\Microsoft Visual Studio 9.0\VC\vcvarsall.bat" is being run with the arguments: x86 & set This in turn calls the platform-specific file vcvars32.bat (or other). In this file, the PATH, INCLUDE, and LIBPATH environment variables are simply being appended/prepended with their values. initialize() then just overrides the os.environ values with these monotonically growing variables. It seems that duplicate names should be filtered out from these paths to prevent overflow. The attached patch seems to fix this issue, if a bit brute force. Please review as a candidate. ===== @set PATH=%DevEnvDir%;%VCINSTALLDIR%\BIN;%VSINSTALLDIR%\Common7\Tools;%VSINSTALLDIR%\Common7\Tools\bin;%FrameworkDir%\%Framework35Version%;%FrameworkDir%\%Framework35Version%\Microsoft .NET Framework 3.5 (Pre-Release Version);%FrameworkDir%\%FrameworkVersion%;%VCINSTALLDIR%\VCPackages;%PATH% @set INCLUDE=%VCINSTALLDIR%\ATLMFC\INCLUDE;%VCINSTALLDIR%\INCLUDE;%INCLUDE% @set LIB=%VCINSTALLDIR%\ATLMFC\LIB;%VCINSTALLDIR%\LIB;%LIB% @set LIBPATH=%FrameworkDir%\%Framework35Version%;%FrameworkDir%\%FrameworkVersion%;%VCINSTALLDIR%\ATLMFC\LIB;%VCINSTALLDIR%\LIB;%LIBPATH% ===== |
|
|
| msg67449 - (view) |
Author: Scott Dial (scott.dial) |
Date: 2008-05-28 14:52 |
| I don't believe this is a valid bug. Can you provide a case where it does in fact grow? This issue has previously been addressed in #1685563, and was carried over to the new code as well. Some lines after the path is appended to, there is a call to normalize_and_reduce_paths(...), which removes duplicates almost exactly like you wrote it. |
|
|
| msg67467 - (view) |
Author: Jim Kleckner (jkleckner) |
Date: 2008-05-29 02:06 |
| Talk about tunnel vision... The code is right next to it! It is the include paths that are growing without bound (and presumably the LIBPATH as well). The test case is the Cython unit tests. They run until the include variable generates a "line too long" error. The normalize_and_reduce_paths() function needs to be performed on INCLUDE and LIBPATH in addition to the exec path. i.e. os.environ['lib'] and os.environ['include']. |
|
|
| msg67479 - (view) |
Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) *  |
Date: 2008-05-29 08:01 |
| I don't understand where the problem comes from: query_vcvarsall() is called only once, when the distutils.msvc9compiler module is imported. Or is the module somehow reloaded or removed from sys.modules? |
|
|
| msg67490 - (view) |
Author: Scott Dial (scott.dial) |
Date: 2008-05-29 13:49 |
| The path gets changed everytime a MSVCCompiler is instantiated. I've seen the same problem with PATH before with PyPy. I agree this is a bug, but I don't see how it can be fixed. The problem exists inside of vcvarsall.bat if I understand this correctly. Furthermore, I don't understand how an exception could actually be thrown; the vc_env values are sourced from the environment after running vcvarsall.bat, so how could they be too long? |
|
|
| msg67513 - (view) |
Author: Jim Kleckner (jkleckner) |
Date: 2008-05-29 21:43 |
| In this file: http://svn.python.org/view/python/trunk/Lib/distutils/msvc9compiler.py?rev=62636&view=auto Notice the line containing: os.environ['include'] = vc_env['include'] This causes the process environment variables to acquire the value returned by vcvars.bat. Since vcvars.bat appends it's values to the environment variable, it grows each time called. Note that the submitted patch does make this problem go away. A better patch could combine the two functions or and treat them consistently. |
|
|
| msg67514 - (view) |
Author: Jim Kleckner (jkleckner) |
Date: 2008-05-29 21:47 |
| Actually, now that I think about it a little more, it seems like "very bad practice" to change the process environment variables as a side effect. A better solution would be to keep a variable for the required ones that is modified as necessary. |
|
|
| msg70182 - (view) |
Author: Jim Kleckner (jkleckner) |
Date: 2008-07-23 22:55 |
| Any new thoughts on this? I had to patch my local copy again after a reinstall. It would be nice to fix it upstream. |
|
|
| msg70183 - (view) |
Author: Jim Kleckner (jkleckner) |
Date: 2008-07-23 23:15 |
| Sorry, posted too quickly. Actually, the problem was a little different. There was an environment variable with '=' characters in it. Here is a patch to deal with that: --- msvc9compiler.py.orig 2008-07-23 16:13:25.248438400 -0700 +++ msvc9compiler.py 2008-07-23 16:13:54.059867200 -0700 @@ -252,7 +252,9 @@ if '=' not in line: continue line = line.strip() - key, value = line.split('=') + i = line.index('=') + key = line[0:i] + value = line[i:] key = key.lower() if key in interesting: if value.endswith(os.pathsep): |
|
|
| msg72340 - (view) |
Author: Jim Kleckner (jkleckner) |
Date: 2008-09-02 14:25 |
| Here is the equals sign fix as a separate patch file. |
|
|
| msg72341 - (view) |
Author: Jim Kleckner (jkleckner) |
Date: 2008-09-02 14:28 |
| Any hope for these two patches being applied? |
|
|
| msg72370 - (view) |
Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * |
Date: 2008-09-02 23:20 |
| Applied both patches (a bit differently for the equal sign) as r66171. |
|
|
| msg72373 - (view) |
Author: Scott Dial (scott.dial) |
Date: 2008-09-03 00:02 |
| This patch shouldn't have been applied as it is. The definition of "removeDuplicates" is both poorly-named, not exactly correct, and redundant (as there is already a "normalize_and_reduce_paths") for performing this fix on PATH. Jim, you acknowledged already that: """The normalize_and_reduce_paths() function needs to be performed on INCLUDE and LIBPATH in addition to the exec path. i.e. os.environ['lib'] and os.environ['include'].""" So, I don't understand how that got missed. I've attached a patch that removes the extra code. |
|
|
| msg72374 - (view) |
Author: Jim Kleckner (jkleckner) |
Date: 2008-09-03 02:39 |
| Yes, much better. I should have read into the interior of the discussion rather than just the edges... Thanks! |
|
|
| msg161242 - (view) |
Author: Scott Dial (scott.dial) |
Date: 2012-05-21 01:20 |
| I was looking through old issues I had commented on and saw that my patch was never applied. The current tip of the codebase still has the redundant "removeDuplicates" function. Not a big deal, just a little extra noise in the code. Probably not worth opening a new ticket for it, but I'd thought I'd ping the nosies (Amaury). |
|
|