Upgrade and improve Visual Studio solution/project generation code

This change improves the Visual Studio solution and project generation code in the following ways:

1. Adds support for Visual Studio 2019 and 2017 project files. In this part, I went a different direction than the existing VS2015 code by doing all of this in the V10 class. I have found this to be the easiest way to continue to add support for new versions of Visual Studio; for example, VS2019 support was a 4-line change after the initial changes.
2. Adds support for consumers to specify C++ include paths and C++ preprocessor definitions to be included in the .vcxproj file. This helps Intellisense function better. (Tests included for this part as well, including one to cover an issue pickling Dir() objects when writing miscellaneous CPPPATH info to the .vcxproj file)
3. Adds <VCProjectUpgraderObjectName> to the project file so we are not prompted to upgrade. This helps the case where a developer has a new version of Visual Studio installed and generates projects for that, while the underlying SCons build uses an older toolset.
4. Excludes .filters files from dspfile processing.
5. Adds a test to cover VS2015 support.

2 files changed, 174 insertions, 56 deletions

diff --git a/src/engine/SCons/Tool/msvs.py b/src/engine/SCons/Tool/msvs.py
index 297c083..e92a226 100644
--- a/src/engine/SCons/Tool/msvs.py
+++ b/src/engine/SCons/Tool/msvs.py
@@ -47,6 +47,7 @@ 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
@@ -75,6 +76,10 @@ def xmlify(s):
 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
+
 
 external_makefile_guid = '{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}'
 
@@ -348,10 +353,20 @@ V10DebugSettings = {
 }
 
 class _GenerateV10User(_UserGenerator):
-    """Generates a Project'user file for MSVS 2010"""
+    """Generates a Project'user file for MSVS 2010 or later"""
 
     def __init__(self, dspfile, source, env):
-        self.versionstr = '4.0'
+        version_num, suite = msvs_parse_version(env['MSVS_VERSION'])
+        if version_num >= 14.2:
+            # Visual Studio 2019 is considered to be version 16.
+            self.versionstr = '16.0'
+        elif version_num >= 14.1:
+            # Visual Studio 2017 is considered to be version 15.
+            self.versionstr = '15.0'
+        elif version_num == 14.0:
+            self.versionstr = '14.0'
+        else:
+            self.versionstr = '4.0'
         self.usrhead = V10UserHeader
         self.usrconf = V10UserConfiguration
         self.usrdebg = V10DebugSettings
@@ -462,15 +477,25 @@ class _DSPGenerator(object):
 
         self.sconscript = env['MSVSSCONSCRIPT']
 
-        if 'cmdargs' not in env or env['cmdargs'] is None:
-            cmdargs = [''] * len(variants)
-        elif SCons.Util.is_String(env['cmdargs']):
-            cmdargs = [env['cmdargs']] * len(variants)
-        elif SCons.Util.is_List(env['cmdargs']):
-            if len(env['cmdargs']) != len(variants):
-                raise SCons.Errors.InternalError("Sizes of 'cmdargs' and 'variant' lists must be the same.")
+        def GetKeyFromEnv(env, key, variants):
+            if key not in env or env[key] is None:
+                return [''] * len(variants)
+            elif SCons.Util.is_String(env[key]):
+                return [env[key]] * len(variants)
+            elif SCons.Util.is_List(env[key]):
+                if len(env[key]) != len(variants):
+                    raise SCons.Errors.InternalError("Sizes of '%s' and 'variant' lists must be the same." % key)
+                else:
+                    return env[key]
             else:
-                cmdargs = env['cmdargs']
+                raise SCons.Errors.InternalError("Unsupported type for key '%s' in environment: %s" %
+                                                 (key, type(env[key])))
+
+        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)]
 
         self.env = env
 
@@ -513,11 +538,13 @@ class _DSPGenerator(object):
         for n in sourcenames:
             self.sources[n].sort(key=lambda a: a.lower())
 
-        def AddConfig(self, variant, buildtarget, outdir, runfile, cmdargs, dspfile=dspfile):
+        def AddConfig(self, variant, buildtarget, outdir, runfile, cmdargs, cppdefines=[], cpppaths=[], dspfile=dspfile):
             config = Config()
             config.buildtarget = buildtarget
             config.outdir = outdir
             config.cmdargs = cmdargs
+            config.cppdefines = cppdefines
+            config.cpppaths = cpppaths
             config.runfile = runfile
 
             match = re.match(r'(.*)\|(.*)', variant)
@@ -532,7 +559,7 @@ class _DSPGenerator(object):
             print("Adding '" + self.name + ' - ' + config.variant + '|' + config.platform + "' to '" + str(dspfile) + "'")
 
         for i in range(len(variants)):
-            AddConfig(self, variants[i], buildtarget[i], outdir[i], runfile[i], cmdargs[i])
+            AddConfig(self, variants[i], buildtarget[i], outdir[i], runfile[i], cmdargs[i], cppdefines[i], cpppaths[i])
 
         self.platforms = []
         for key in list(self.configs.keys()):
@@ -882,6 +909,8 @@ class _GenerateV7DSP(_DSPGenerator, _GenerateV7User):
             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:
@@ -899,8 +928,8 @@ class _GenerateV7DSP(_DSPGenerator, _GenerateV7User):
             # 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)
+            preprocdefs = xmlify(';'.join(processDefines(cppdefines)))
+            includepath_Dirs = processIncludes(cpppaths, self.env, None, None)
             includepath = xmlify(';'.join([str(x) for x in includepath_Dirs]))
 
             if not env_has_buildtarget:
@@ -1060,7 +1089,7 @@ class _GenerateV7DSP(_DSPGenerator, _GenerateV7User):
 
 V10DSPHeader = """\
 <?xml version="1.0" encoding="%(encoding)s"?>
-<Project DefaultTargets="Build" ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+<Project DefaultTargets="Build" ToolsVersion="%(versionstr)s" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
 """
 
 V10DSPProjectConfiguration = """\
@@ -1075,6 +1104,7 @@ V10DSPGlobals = """\
 \t\t<ProjectGuid>%(project_guid)s</ProjectGuid>
 %(scc_attrs)s\t\t<RootNamespace>%(name)s</RootNamespace>
 \t\t<Keyword>MakeFileProj</Keyword>
+\t\t<VCProjectUpgraderObjectName>NoUpgrade</VCProjectUpgraderObjectName>
 \t</PropertyGroup>
 """
 
@@ -1112,9 +1142,9 @@ V15DSPHeader = """\
 class _GenerateV10DSP(_DSPGenerator, _GenerateV10User):
     """Generates a Project file for MSVS 2010"""
 
-    def __init__(self, dspfile, header, source, env):
+    def __init__(self, dspfile, source, env):
         _DSPGenerator.__init__(self, dspfile, source, env)
-        self.dspheader = header
+        self.dspheader = V10DSPHeader
         self.dspconfiguration = V10DSPProjectConfiguration
         self.dspglobals = V10DSPGlobals
 
@@ -1123,6 +1153,7 @@ class _GenerateV10DSP(_DSPGenerator, _GenerateV10User):
     def PrintHeader(self):
         env = self.env
         name = self.name
+        versionstr = self.versionstr
         encoding = env.subst('$MSVSENCODING')
         project_guid = env.get('MSVS_PROJECT_GUID', '')
         scc_provider = env.get('MSVS_SCC_PROVIDER', '')
@@ -1234,7 +1265,8 @@ class _GenerateV10DSP(_DSPGenerator, _GenerateV10User):
             raise SCons.Errors.InternalError('Unable to open "' + self.filtersabs + '" for writing:' + str(detail))
 
         self.filters_file.write('<?xml version="1.0" encoding="utf-8"?>\n'
-                                '<Project ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">\n')
+                                '<Project ToolsVersion="%s" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">\n' %
+                                self.versionstr)
 
         self.PrintSourceFiles()
 
@@ -1285,7 +1317,7 @@ class _GenerateV10DSP(_DSPGenerator, _GenerateV10User):
                 file = value
                 if commonprefix:
                     file = os.path.join(commonprefix, value)
-                file = os.path.normpath(file)
+                file = processSource(file)
 
                 self.file.write('\t\t<%s Include="%s" />\n' % (keywords[kind], file))
                 self.filters_file.write('\t\t<%s Include="%s">\n'
@@ -1466,6 +1498,9 @@ class _GenerateV7DSW(_DSWGenerator):
             for dspfile in self.dspfiles:
                 dsp_folder_path, name = os.path.split(dspfile)
                 dsp_folder_path = os.path.abspath(dsp_folder_path)
+                if SCons.Util.splitext(name)[1] == '.filters':
+                    # Ignore .filters project files
+                    continue
                 dsp_relative_folder_path = os.path.relpath(dsp_folder_path, self.dsw_folder_path)
                 if dsp_relative_folder_path == os.curdir:
                     dsp_relative_file_path = name
@@ -1515,7 +1550,11 @@ class _GenerateV7DSW(_DSWGenerator):
     def PrintSolution(self):
         """Writes a solution file"""
         self.file.write('Microsoft Visual Studio Solution File, Format Version %s\n' % self.versionstr)
-        if self.version_num > 14.0:
+        if self.version_num >= 14.2:
+            # Visual Studio 2019 is considered to be version 16.
+            self.file.write('# Visual Studio 16\n')
+        elif self.version_num > 14.0:
+            # Visual Studio 2015 and 2017 are both considered to be version 15.
             self.file.write('# Visual Studio 15\n')
         elif self.version_num >= 12.0:
             self.file.write('# Visual Studio 14\n')
@@ -1695,11 +1734,8 @@ def GenerateDSP(dspfile, source, env):
     version_num = 6.0
     if 'MSVS_VERSION' in env:
         version_num, suite = msvs_parse_version(env['MSVS_VERSION'])
-    if version_num > 14.0:
-        g = _GenerateV10DSP(dspfile, V15DSPHeader, source, env)
-        g.Build()
-    elif version_num >= 10.0:
-        g = _GenerateV10DSP(dspfile, V10DSPHeader, source, env)
+    if version_num >= 10.0:
+        g = _GenerateV10DSP(dspfile, source, env)
         g.Build()
     elif version_num >= 7.0:
         g = _GenerateV7DSP(dspfile, source, env)
@@ -1973,8 +2009,14 @@ def generate(env):
             default_MSVS_SConscript = env.File('SConstruct')
         env['MSVSSCONSCRIPT'] = default_MSVS_SConscript
 
-    env['MSVSSCONS'] = '"%s" -c "%s"' % (python_executable, getExecScriptMain(env))
-    env['MSVSSCONSFLAGS'] = '-C "${MSVSSCONSCRIPT.dir.get_abspath()}" -f ${MSVSSCONSCRIPT.name}'
+    # Allow consumers to provide their own versions of MSVSSCONS and
+    # MSVSSCONSFLAGS. This helps support consumers who use wrapper scripts to
+    # invoke scons.
+    if 'MSVSSCONS' not in env:
+        env['MSVSSCONS'] = '"%s" -c "%s"' % (python_executable, getExecScriptMain(env))
+    if 'MSVSSCONSFLAGS' not in env:
+        env['MSVSSCONSFLAGS'] = '-C "${MSVSSCONSCRIPT.dir.get_abspath()}" -f ${MSVSSCONSCRIPT.name}'
+
     env['MSVSSCONSCOM'] = '$MSVSSCONS $MSVSSCONSFLAGS'
     env['MSVSBUILDCOM'] = '$MSVSSCONSCOM "$MSVSBUILDTARGET"'
     env['MSVSREBUILDCOM'] = '$MSVSSCONSCOM "$MSVSBUILDTARGET"'
diff --git a/src/engine/SCons/Tool/msvsTests.py b/src/engine/SCons/Tool/msvsTests.py
index 477694a..c9d6b13 100644
--- a/src/engine/SCons/Tool/msvsTests.py
+++ b/src/engine/SCons/Tool/msvsTests.py
@@ -32,6 +32,7 @@ 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
@@ -352,6 +353,36 @@ regdata_80 = r'''
 "VCXDCMakeTool"="*.xdc"
 '''.split('\n')
 
+regdata_140 = r'''
+[HKEY_LOCAL_MACHINE\Software\Microsoft\VisualStudio\14.0\Setup\VS]
+"MSMDir"="C:\\Program Files (x86)\\Common Files\\Merge Modules\\"
+"ProductDir"="C:\\Program Files (x86)\\Microsoft Visual Studio 14.0\\"
+"VS7EnvironmentLocation"="C:\\Program Files (x86)\\Microsoft Visual Studio 14.0\\Common7\\IDE\\devenv.exe"
+"EnvironmentPath"="C:\\Program Files (x86)\\Microsoft Visual Studio 14.0\\Common7\\IDE\\devenv.exe"
+"EnvironmentDirectory"="C:\\Program Files (x86)\\Microsoft Visual Studio 14.0\\Common7\\IDE\\"
+"VS7CommonDir"="C:\\Program Files (x86)\\Microsoft Visual Studio 14.0\\Common7\\"
+"VS7CommonBinDir"=""
+[HKEY_LOCAL_MACHINE\Software\Microsoft\VisualStudio\14.0\Setup\VS\BuildNumber]
+"1033"="14.0"
+[HKEY_LOCAL_MACHINE\Software\Microsoft\VisualStudio\14.0\Setup\VS\Community]
+"ProductDir"="C:\\Program Files (x86)\\Microsoft Visual Studio 14.0\\"
+[HKEY_LOCAL_MACHINE\Software\Microsoft\VisualStudio\14.0\Setup\VS\JSLS_MSI]
+"Version"="14.0.25527"
+[HKEY_LOCAL_MACHINE\Software\Microsoft\VisualStudio\14.0\Setup\VS\JSPS_MSI]
+"Version"="14.0.25527"
+[HKEY_LOCAL_MACHINE\Software\Microsoft\VisualStudio\14.0\Setup\VS\Pro]
+"ProductDir"="C:\\Program Files (x86)\\Microsoft Visual Studio 14.0\\"
+[HKEY_LOCAL_MACHINE\Software\Microsoft\VisualStudio\14.0\Setup\VS\professional]
+"IsInstallInProgress"="0"
+"CurrentOperation"="install"
+"SetupFeedUri"="http://go.microsoft.com/fwlink/?LinkID=659004&clcid=0x409"
+"SetupFeedLocalCache"="C:\\ProgramData\\Microsoft\\VisualStudioSecondaryInstaller\\14.0\\LastUsedFeed\\{68432bbb-c9a5-4a7b-bab3-ae5a49b28303}\\Feed.xml"
+"InstallResult"="0"
+[HKEY_LOCAL_MACHINE\Software\Microsoft\VisualStudio\14.0\Setup\VS\SecondaryInstaller]
+[HKEY_LOCAL_MACHINE\Software\Microsoft\VisualStudio\14.0\Setup\VS\SecondaryInstaller\AppInsightsTools]
+"Version"="7.0.20620.1"
+'''.split('\n')
+
 regdata_cv = r'''[HKEY_LOCAL_MACHINE\Software\Microsoft\Windows\CurrentVersion]
 "ProgramFilesDir"="C:\Program Files"
 "CommonFilesDir"="C:\Program Files\Common Files"
@@ -385,6 +416,12 @@ class DummyEnv(object):
     def has_key(self,name):
         return name in self.dict
 
+    def get(self, name, value=None):
+        if self.has_key(name):
+            return self.dict[name]
+        else:
+            return value
+
 class RegKey(object):
     """key class for storing an 'open' registry key"""
     def __init__(self,key):
@@ -607,13 +644,15 @@ class msvsTestCase(unittest.TestCase):
         version_num, suite = msvs_parse_version(self.highest_version)
         if version_num >= 10.0:
             function_test = _GenerateV10DSP
+            dspfile = 'test.vcxproj'
         elif version_num >= 7.0:
             function_test = _GenerateV7DSP
+            dspfile = 'test.dsp'
         else:
             function_test = _GenerateV6DSP
+            dspfile = 'test.dsp'
             
         str_function_test = str(function_test.__init__)
-        dspfile = 'test.dsp'
         source = 'test.cpp'
         
         # Create the cmdargs test list
@@ -623,50 +662,69 @@ class msvsTestCase(unittest.TestCase):
                         'debug=False target_arch=32',
                         'debug=True target_arch=x64', 
                         'debug=False target_arch=x64']
-        
-        # Tuple list :   (parameter,        dictionary of expected result per variant)
-        tests_cmdargs = [(None,            dict.fromkeys(list_variant, '')), 
-                         ('',              dict.fromkeys(list_variant, '')), 
-                         (list_cmdargs[0], dict.fromkeys(list_variant, list_cmdargs[0])),
-                         (list_cmdargs,    dict(list(zip(list_variant, list_cmdargs))))]
-        
+        list_cppdefines = ['_A', '_B', 'C', None]
+        list_cpppaths = [r'C:\test1', r'C:\test1;C:\test2', Dir('subdir'), None]
+
+        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
+            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]))))
+            ]
+
+        tests_cmdargs = TestParamsFromList(list_variant, list_cmdargs)
+        tests_cppdefines = TestParamsFromList(list_variant, list_cppdefines)
+        tests_cpppaths = TestParamsFromList(list_variant, list_cpppaths)
+
         # Run the test for each test case
         for param_cmdargs, expected_cmdargs in tests_cmdargs:
-            debug('Testing %s. with :\n  variant = %s \n  cmdargs = "%s"' % \
-                  (str_function_test, list_variant, param_cmdargs))
-            param_configs = []
-            expected_configs = {}
-            for platform in ['Win32', 'x64']:
-                for variant in ['Debug', 'Release']:
-                    variant_platform = '%s|%s' % (variant, platform)
-                    runfile = '%s\\%s\\test.exe' % (platform, variant)
-                    buildtarget = '%s\\%s\\test.exe' % (platform, variant)
-                    outdir = '%s\\%s' % (platform, variant)
-            
-                    # Create parameter list for this variant_platform
-                    param_configs.append([variant_platform, runfile, buildtarget, outdir])
+            for param_cppdefines, expected_cppdefines in tests_cppdefines:
+                for param_cpppaths, expected_cpppaths in tests_cpppaths:
+                    debug('Testing %s. with :\n  variant = %s \n  cmdargs = "%s" \n  cppdefines = "%s" \n  cpppaths = "%s"' % \
+                          (str_function_test, list_variant, param_cmdargs, param_cppdefines, param_cpppaths))
+                    param_configs = []
+                    expected_configs = {}
+                    for platform in ['Win32', 'x64']:
+                        for variant in ['Debug', 'Release']:
+                            variant_platform = '%s|%s' % (variant, platform)
+                            runfile = '%s\\%s\\test.exe' % (platform, variant)
+                            buildtarget = '%s\\%s\\test.exe' % (platform, variant)
+                            outdir = '%s\\%s' % (platform, variant)
             
-                    # Create expected dictionary result for this variant_platform
-                    expected_configs[variant_platform] = \
-                    {'variant': variant, 'platform': platform, 
-                     'runfile': runfile,
-                     'buildtarget': buildtarget, 
-                     'outdir': outdir,
-                     'cmdargs': expected_cmdargs[variant_platform]}
+                            # Create parameter list for this variant_platform
+                            param_configs.append([variant_platform, runfile, buildtarget, outdir])
             
+                            # Create expected dictionary result for this variant_platform
+                            expected_configs[variant_platform] = {
+                                'variant': variant,
+                                'platform': platform,
+                                'runfile': runfile,
+                                'buildtarget': buildtarget,
+                                'outdir': outdir,
+                                'cmdargs': expected_cmdargs[variant_platform],
+                                'cppdefines': expected_cppdefines[variant_platform],
+                                'cpppaths': expected_cpppaths[variant_platform],
+                            }
+
             # Create parameter environment with final parameter dictionary
             param_dict = dict(list(zip(('variant', 'runfile', 'buildtarget', 'outdir'),
                                   [list(l) for l in zip(*param_configs)])))
             param_dict['cmdargs'] = param_cmdargs
+            param_dict['cppdefines'] = param_cppdefines
+            param_dict['cpppaths'] = param_cpppaths
 
             # Hack to be able to run the test with a 'DummyEnv'
             class _DummyEnv(DummyEnv):
-                def subst(self, string) : 
+                def subst(self, string, *args, **kwargs):
                     return string
             
             env = _DummyEnv(param_dict)
             env['MSVSSCONSCRIPT'] = ''
             env['MSVS_VERSION'] = self.highest_version
+            env['MSVSBUILDTARGET'] = 'target'
            
             # Call function to test
             genDSP = function_test(dspfile, source, env)
@@ -676,6 +734,11 @@ class msvsTestCase(unittest.TestCase):
             for key in list(genDSP.configs.keys()):
                 self.assertDictEqual(genDSP.configs[key].__dict__, expected_configs[key])
 
+            genDSP.Build()
+
+            # Delete the resulting file so we don't leave anything behind.
+            os.remove(os.path.realpath(dspfile))
+
 class msvs6aTestCase(msvsTestCase):
     """Test MSVS 6 Registry"""
     registry = DummyRegistry(regdata_6a + regdata_cv)
@@ -787,6 +850,18 @@ class msvs80TestCase(msvsTestCase):
     }
     default_install_loc = install_locs['8.0']
 
+class msvs140TestCase(msvsTestCase):
+    """Test MSVS 140 Registry"""
+    registry = DummyRegistry(regdata_140 + regdata_cv)
+    default_version = '14.0'
+    highest_version = '14.0'
+    number_of_versions = 2
+    install_locs = {
+        '14.0' : {'VSINSTALLDIR': 'C:\\Program Files\\Microsoft Visual Studio 14.0',
+                  'VCINSTALLDIR': 'C:\\Program Files\\Microsoft Visual Studio 14.0\\VC'},
+    }
+    default_install_loc = install_locs['14.0']
+
 class msvsEmptyTestCase(msvsTestCase):
     """Test Empty Registry"""
     registry = DummyRegistry(regdata_none)
@@ -829,6 +904,7 @@ if __name__ == "__main__":
         msvs71TestCase,
         msvs8ExpTestCase,
         msvs80TestCase,
+        msvs140TestCase,
         msvsEmptyTestCase,
     ]