Fix issues raised in code review

2 files changed, 72 insertions, 34 deletions

diff --git a/src/engine/SCons/Tool/msvs.py b/src/engine/SCons/Tool/msvs.py
index e92a226..7dd1dc7 100644
--- a/src/engine/SCons/Tool/msvs.py
+++ b/src/engine/SCons/Tool/msvs.py
@@ -47,7 +47,6 @@ import sys
 import SCons.Builder
 import SCons.Node.FS
 import SCons.Platform.win32
-import SCons.Script
 import SCons.Script.SConscript
 import SCons.PathList
 import SCons.Util
@@ -71,14 +70,14 @@ def xmlify(s):
     s = s.replace('\n', '
')
     return s
 
-# Process a CPPPATH list in includes, given the env, target and source.
-# Returns a tuple of nodes.
 def processIncludes(includes, env, target, source):
-    return SCons.PathList.PathList(includes).subst_path(env, target, source)
-
-# Convert a file to its absolute path.
-def processSource(file):
-    return SCons.Script.File(file).abspath
+    """
+    Process a CPPPATH list in includes, given the env, target and source.
+    Returns a list of directory paths. These paths are absolute so we avoid
+    putting pound-prefixed paths in a Visual Studio project file.
+    """
+    return [env.Dir(i).abspath for i in
+            SCons.PathList.PathList(includes).subst_path(env, target, source)]
 
 
 external_makefile_guid = '{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}'
@@ -493,9 +492,7 @@ class _DSPGenerator(object):
 
         cmdargs = GetKeyFromEnv(env, 'cmdargs', variants)
         cppdefines = GetKeyFromEnv(env, 'cppdefines', variants)
-
-        dirpathfunc = lambda x: x.abspath if hasattr(x, 'abspath') else x
-        cpppaths = [dirpathfunc(path) for path in GetKeyFromEnv(env, 'cpppaths', variants)]
+        cpppaths = GetKeyFromEnv(env, 'cpppaths', variants)
 
         self.env = env
 
@@ -538,15 +535,17 @@ class _DSPGenerator(object):
         for n in sourcenames:
             self.sources[n].sort(key=lambda a: a.lower())
 
-        def AddConfig(self, variant, buildtarget, outdir, runfile, cmdargs, cppdefines=[], cpppaths=[], dspfile=dspfile):
+        def AddConfig(self, variant, buildtarget, outdir, runfile, cmdargs, cppdefines, cpppaths, dspfile=dspfile, env=env):
             config = Config()
             config.buildtarget = buildtarget
             config.outdir = outdir
             config.cmdargs = cmdargs
             config.cppdefines = cppdefines
-            config.cpppaths = cpppaths
             config.runfile = runfile
 
+            # Dir objects can't be pickled, so we need an absolute path here.
+            config.cpppaths = processIncludes(cpppaths, env, None, None)
+
             match = re.match(r'(.*)\|(.*)', variant)
             if match:
                 config.variant = match.group(1)
@@ -929,8 +928,7 @@ class _GenerateV7DSP(_DSPGenerator, _GenerateV7User):
             # so they could vary depending on the command being generated.  This code
             # assumes they don't.
             preprocdefs = xmlify(';'.join(processDefines(cppdefines)))
-            includepath_Dirs = processIncludes(cpppaths, self.env, None, None)
-            includepath = xmlify(';'.join([str(x) for x in includepath_Dirs]))
+            includepath = xmlify(';'.join(processIncludes(cpppaths, self.env, None, None)))
 
             if not env_has_buildtarget:
                 del self.env['MSVSBUILDTARGET']
@@ -1229,6 +1227,8 @@ class _GenerateV10DSP(_DSPGenerator, _GenerateV10User):
             buildtarget = self.configs[kind].buildtarget
             runfile     = self.configs[kind].runfile
             cmdargs = self.configs[kind].cmdargs
+            cpppaths = self.configs[kind].cpppaths
+            cppdefines = self.configs[kind].cppdefines
 
             env_has_buildtarget = 'MSVSBUILDTARGET' in self.env
             if not env_has_buildtarget:
@@ -1246,9 +1246,8 @@ class _GenerateV10DSP(_DSPGenerator, _GenerateV10User):
             # This isn't perfect; CPPDEFINES and CPPPATH can contain $TARGET and $SOURCE,
             # so they could vary depending on the command being generated.  This code
             # assumes they don't.
-            preprocdefs = xmlify(';'.join(processDefines(self.env.get('CPPDEFINES', []))))
-            includepath_Dirs = processIncludes(self.env.get('CPPPATH', []), self.env, None, None)
-            includepath = xmlify(';'.join([str(x) for x in includepath_Dirs]))
+            preprocdefs = xmlify(';'.join(processDefines(cppdefines)))
+            includepath = xmlify(';'.join(processIncludes(cpppaths, self.env, None, None)))
 
             if not env_has_buildtarget:
                 del self.env['MSVSBUILDTARGET']
@@ -1317,7 +1316,7 @@ class _GenerateV10DSP(_DSPGenerator, _GenerateV10User):
                 file = value
                 if commonprefix:
                     file = os.path.join(commonprefix, value)
-                file = processSource(file)
+                file = os.path.normpath(file)
 
                 self.file.write('\t\t<%s Include="%s" />\n' % (keywords[kind], file))
                 self.filters_file.write('\t\t<%s Include="%s">\n'
@@ -1828,8 +1827,7 @@ def projectEmitter(target, source, env):
 
         # Project file depends on CPPDEFINES and CPPPATH
         preprocdefs = xmlify(';'.join(processDefines(env.get('CPPDEFINES', []))))
-        includepath_Dirs = processIncludes(env.get('CPPPATH', []), env, None, None)
-        includepath = xmlify(';'.join([str(x) for x in includepath_Dirs]))
+        includepath = xmlify(';'.join(processIncludes(env.get('CPPPATH', []), env, None, None)))
         source = source + "; ppdefs:%s incpath:%s"%(preprocdefs, includepath)
 
         if 'buildtarget' in env and env['buildtarget'] is not None:
diff --git a/src/engine/SCons/Tool/msvsTests.py b/src/engine/SCons/Tool/msvsTests.py
index c9d6b13..3573287 100644
--- a/src/engine/SCons/Tool/msvsTests.py
+++ b/src/engine/SCons/Tool/msvsTests.py
@@ -32,7 +32,6 @@ import copy
 import TestCmd
 import TestUnit
 
-from SCons.Script import Dir
 from SCons.Tool.msvs import *
 from SCons.Tool.MSCommon.vs import SupportedVSList
 import SCons.Util
@@ -422,6 +421,12 @@ class DummyEnv(object):
         else:
             return value
 
+    def Dir(self, name):
+        # Depend upon SCons.Script.Dir so we can create a Directory object
+        # that doesn't actually exist on disk without problems or side effects.
+        return SCons.Script.Dir(name)
+        
+
 class RegKey(object):
     """key class for storing an 'open' registry key"""
     def __init__(self,key):
@@ -644,13 +649,17 @@ class msvsTestCase(unittest.TestCase):
         version_num, suite = msvs_parse_version(self.highest_version)
         if version_num >= 10.0:
             function_test = _GenerateV10DSP
-            dspfile = 'test.vcxproj'
+            suffix = '.vcxproj'
         elif version_num >= 7.0:
             function_test = _GenerateV7DSP
-            dspfile = 'test.dsp'
+            suffix = '.dsp'
         else:
             function_test = _GenerateV6DSP
-            dspfile = 'test.dsp'
+            suffix = '.dsp'
+
+        # Avoid any race conditions between the test cases when we test
+        # actually writing the files.
+        dspfile = 'test%s%s' % (hash(self), suffix)
             
         str_function_test = str(function_test.__init__)
         source = 'test.cpp'
@@ -662,17 +671,43 @@ class msvsTestCase(unittest.TestCase):
                         'debug=False target_arch=32',
                         'debug=True target_arch=x64', 
                         'debug=False target_arch=x64']
-        list_cppdefines = ['_A', '_B', 'C', None]
-        list_cpppaths = [r'C:\test1', r'C:\test1;C:\test2', Dir('subdir'), None]
+        list_cppdefines = [['_A', '_B', 'C'], ['_B', '_C_'], ['D'], []]
+        list_cpppaths = [[r'C:\test1'], [r'C:\test1;C:\test2'],
+                         [DummyEnv().Dir('subdir')], []]
 
         def TestParamsFromList(test_variant, test_list):
-            # Tuple list: (parameter, dictionary of expected result per variant)
-            dirpathfunc = lambda x: x.abspath if hasattr(x, 'abspath') else x
+            """
+            Generates test data based on the parameters passed in.
+
+            Returns tuple list:
+                1. Parameter.
+                2. Dictionary of expected result per variant.
+            """
+            def normalizeParam(param):
+                """
+                Converts the raw data based into the AddConfig function of
+                msvs.py to the expected result.
+
+                Expects the following behavior:
+                    1. A value of None will be converted to an empty list.
+                    2. A File or Directory object will be converted to an
+                       absolute path (because those objects can't be pickled).
+                    3. Otherwise, the parameter will be used.
+                """
+                if param is None:
+                    return []
+                elif isinstance(param, list):
+                    return [normalizeParam(p) for p in param]
+                elif hasattr(param, 'abspath'):
+                    return param.abspath
+                else:
+                    return param
+
             return [
                 (None, dict.fromkeys(test_variant, '')),
                 ('', dict.fromkeys(test_variant, '')),
-                (test_list[0], dict.fromkeys(test_variant, dirpathfunc(test_list[0]))),
-                (test_list, dict(list(zip(test_variant, [dirpathfunc(x) for x in test_list]))))
+                (test_list[0], dict.fromkeys(test_variant, normalizeParam(test_list[0]))),
+                (test_list, dict(list(zip(test_variant, [normalizeParam(x) for x in test_list]))))
             ]
 
         tests_cmdargs = TestParamsFromList(list_variant, list_cmdargs)
@@ -700,9 +735,9 @@ class msvsTestCase(unittest.TestCase):
                             # Create expected dictionary result for this variant_platform
                             expected_configs[variant_platform] = {
                                 'variant': variant,
-                                'platform': platform,
+                                'platform': platform, 
                                 'runfile': runfile,
-                                'buildtarget': buildtarget,
+                                'buildtarget': buildtarget, 
                                 'outdir': outdir,
                                 'cmdargs': expected_cmdargs[variant_platform],
                                 'cppdefines': expected_cppdefines[variant_platform],
@@ -737,7 +772,12 @@ class msvsTestCase(unittest.TestCase):
             genDSP.Build()
 
             # Delete the resulting file so we don't leave anything behind.
-            os.remove(os.path.realpath(dspfile))
+            for file in [dspfile, dspfile + '.filters']:
+                path = os.path.realpath(file)
+                try:
+                    os.remove(path)
+                except OSError:
+                    pass
 
 class msvs6aTestCase(msvsTestCase):
     """Test MSVS 6 Registry"""