classification
Title: VS8 include dirs grow without bound
Type: behavior Stage:
Components: Distutils Versions: Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: amaury.forgeotdarc, jkleckner, scott.dial
Priority: normal Keywords: patch

Created on 2008-05-26 19:56 by jkleckner, last changed 2012-05-21 01:20 by scott.dial. This issue is now closed.

Files
File name Uploaded Description Edit
dist.patch jkleckner, 2008-05-26 19:56
equalsInEnv.patch jkleckner, 2008-09-02 14:25 Fix situation where an equals sign is in an environment variable.
msvc9compiler_path.diff scott.dial, 2008-09-03 00:02 patch to use "normalize_and_reduce_paths" instead of "removeDuplicates"
Messages (15)
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) * (Python committer) 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) * (Python committer) 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).
History
Date User Action Args
2012-05-21 01:20:29scott.dialsetmessages: + msg161242
2008-09-03 02:39:54jklecknersetmessages: + msg72374
2008-09-03 00:02:37scott.dialsetfiles: + msvc9compiler_path.diff
messages: + msg72373
2008-09-02 23:20:45amaury.forgeotdarcsetstatus: open -> closed
resolution: fixed
messages: + msg72370
2008-09-02 14:28:58jklecknersetmessages: + msg72341
2008-09-02 14:25:29jklecknersetfiles: + equalsInEnv.patch
messages: + msg72340
2008-07-23 23:15:50jklecknersetmessages: + msg70183
2008-07-23 22:55:06jklecknersetmessages: + msg70182
2008-05-29 21:47:13jklecknersetmessages: + msg67514
2008-05-29 21:43:16jklecknersetmessages: + msg67513
2008-05-29 13:49:50scott.dialsetmessages: + msg67490
2008-05-29 08:01:09amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg67479
2008-05-29 02:06:58jklecknersetmessages: + msg67467
2008-05-28 14:52:20scott.dialsetnosy: + scott.dial
messages: + msg67449
2008-05-26 19:56:35jklecknercreate