Issue #24798: _msvccompiler.py doesn't properly support manifests

2 files changed, 50 insertions, 238 deletions

diff --git a/Lib/distutils/_msvccompiler.py b/Lib/distutils/_msvccompiler.py
index 4e2eed7..b344616 100644
--- a/Lib/distutils/_msvccompiler.py
+++ b/Lib/distutils/_msvccompiler.py
@@ -15,7 +15,6 @@ for older versions in distutils.msvc9compiler and distutils.msvccompiler.
 
 import os
 import subprocess
-import re
 
 from distutils.errors import DistutilsExecError, DistutilsPlatformError, \
                              CompileError, LibError, LinkError
@@ -182,7 +181,8 @@ class MSVCCompiler(CCompiler) :
             raise DistutilsPlatformError("Unable to find a compatible "
                 "Visual Studio installation.")
 
-        paths = vc_env.get('path', '').split(os.pathsep)
+        self._paths = vc_env.get('path', '')
+        paths = self._paths.split(os.pathsep)
         self.cc = _find_exe("cl.exe", paths)
         self.linker = _find_exe("link.exe", paths)
         self.lib = _find_exe("lib.exe", paths)
@@ -199,23 +199,41 @@ class MSVCCompiler(CCompiler) :
                 self.add_library_dir(dir)
 
         self.preprocess_options = None
+        # Use /MT[d] to build statically, then switch from libucrt[d].lib to ucrt[d].lib
+        # later to dynamically link to ucrtbase but not vcruntime.
         self.compile_options = [
-            '/nologo', '/Ox', '/MD', '/W3', '/GL', '/DNDEBUG'
+            '/nologo', '/Ox', '/MT', '/W3', '/GL', '/DNDEBUG'
         ]
         self.compile_options_debug = [
-            '/nologo', '/Od', '/MDd', '/Zi', '/W3', '/D_DEBUG'
+            '/nologo', '/Od', '/MTd', '/Zi', '/W3', '/D_DEBUG'
         ]
 
-        self.ldflags_shared = [
-            '/nologo', '/DLL', '/INCREMENTAL:NO', '/LTCG', '/nodefaultlib:libucrt.lib', 'ucrt.lib'
+        ldflags = [
+            '/nologo', '/INCREMENTAL:NO', '/LTCG', '/nodefaultlib:libucrt.lib', 'ucrt.lib',
         ]
-        self.ldflags_shared_debug = [
-            '/nologo', '/DLL', '/INCREMENTAL:no', '/LTCG', '/DEBUG:FULL', '/nodefaultlib:libucrtd.lib', 'ucrtd.lib'
-        ]
-        self.ldflags_static = [
-            '/nologo'
+        ldflags_debug = [
+            '/nologo', '/INCREMENTAL:NO', '/LTCG', '/DEBUG:FULL', '/nodefaultlib:libucrtd.lib', 'ucrtd.lib',
         ]
 
+        self.ldflags_exe = [*ldflags, '/MANIFEST:EMBED,ID=1']
+        self.ldflags_exe_debug = [*ldflags_debug, '/MANIFEST:EMBED,ID=1']
+        self.ldflags_shared = [*ldflags, '/DLL', '/MANIFEST:EMBED,ID=2', '/MANIFESTUAC:NO']
+        self.ldflags_shared_debug = [*ldflags_debug, '/DLL', '/MANIFEST:EMBED,ID=2', '/MANIFESTUAC:NO']
+        self.ldflags_static = [*ldflags]
+        self.ldflags_static_debug = [*ldflags_debug]
+
+        self._ldflags = {
+            (CCompiler.EXECUTABLE, None): self.ldflags_exe,
+            (CCompiler.EXECUTABLE, False): self.ldflags_exe,
+            (CCompiler.EXECUTABLE, True): self.ldflags_exe_debug,
+            (CCompiler.SHARED_OBJECT, None): self.ldflags_shared,
+            (CCompiler.SHARED_OBJECT, False): self.ldflags_shared,
+            (CCompiler.SHARED_OBJECT, True): self.ldflags_shared_debug,
+            (CCompiler.SHARED_LIBRARY, None): self.ldflags_static,
+            (CCompiler.SHARED_LIBRARY, False): self.ldflags_static,
+            (CCompiler.SHARED_LIBRARY, True): self.ldflags_static_debug,
+        }
+
         self.initialized = True
 
     # -- Worker methods ------------------------------------------------
@@ -224,9 +242,12 @@ class MSVCCompiler(CCompiler) :
                          source_filenames,
                          strip_dir=0,
                          output_dir=''):
-        ext_map = {ext: self.obj_extension for ext in self.src_extensions}
-        ext_map.update((ext, self.res_extension)
-                for ext in self._rc_extensions + self._mc_extensions)
+        ext_map = {
+            **{ext: self.obj_extension for ext in self.src_extensions},
+            **{ext: self.res_extension for ext in self._rc_extensions + self._mc_extensions},
+        }
+
+        output_dir = output_dir or ''
 
         def make_out_path(p):
             base, ext = os.path.splitext(p)
@@ -237,18 +258,17 @@ class MSVCCompiler(CCompiler) :
                 if base.startswith((os.path.sep, os.path.altsep)):
                     base = base[1:]
             try:
-                return base + ext_map[ext]
+                # XXX: This may produce absurdly long paths. We should check
+                # the length of the result and trim base until we fit within
+                # 260 characters.
+                return os.path.join(output_dir, base + ext_map[ext])
             except LookupError:
                 # Better to raise an exception instead of silently continuing
                 # and later complain about sources and targets having
                 # different lengths
                 raise CompileError("Don't know how to compile {}".format(p))
 
-        output_dir = output_dir or ''
-        return [
-            os.path.join(output_dir, make_out_path(src_name))
-            for src_name in source_filenames
-        ]
+        return list(map(make_out_path, source_filenames))
 
 
     def compile(self, sources,
@@ -359,6 +379,7 @@ class MSVCCompiler(CCompiler) :
             if debug:
                 pass # XXX what goes here?
             try:
+                log.debug('Executing "%s" %s', self.lib, ' '.join(lib_args))
                 self.spawn([self.lib] + lib_args)
             except DistutilsExecError as msg:
                 raise LibError(msg)
@@ -399,14 +420,9 @@ class MSVCCompiler(CCompiler) :
             output_filename = os.path.join(output_dir, output_filename)
 
         if self._need_link(objects, output_filename):
-            ldflags = (self.ldflags_shared_debug if debug
-                       else self.ldflags_shared)
-            if target_desc == CCompiler.EXECUTABLE:
-                ldflags = ldflags[1:]
+            ldflags = self._ldflags[target_desc, debug]
 
-            export_opts = []
-            for sym in (export_symbols or []):
-                export_opts.append("/EXPORT:" + sym)
+            export_opts = ["/EXPORT:" + sym for sym in (export_symbols or [])]
 
             ld_args = (ldflags + lib_opts + export_opts +
                        objects + ['/OUT:' + output_filename])
@@ -425,8 +441,6 @@ class MSVCCompiler(CCompiler) :
                     self.library_filename(dll_name))
                 ld_args.append ('/IMPLIB:' + implib_file)
 
-            self.manifest_setup_ldargs(output_filename, build_temp, ld_args)
-
             if extra_preargs:
                 ld_args[:0] = extra_preargs
             if extra_postargs:
@@ -434,101 +448,20 @@ class MSVCCompiler(CCompiler) :
 
             self.mkpath(os.path.dirname(output_filename))
             try:
+                log.debug('Executing "%s" %s', self.linker, ' '.join(ld_args))
                 self.spawn([self.linker] + ld_args)
             except DistutilsExecError as msg:
                 raise LinkError(msg)
-
-            # embed the manifest
-            # XXX - this is somewhat fragile - if mt.exe fails, distutils
-            # will still consider the DLL up-to-date, but it will not have a
-            # manifest.  Maybe we should link to a temp file?  OTOH, that
-            # implies a build environment error that shouldn't go undetected.
-            mfinfo = self.manifest_get_embed_info(target_desc, ld_args)
-            if mfinfo is not None:
-                mffilename, mfid = mfinfo
-                out_arg = '-outputresource:{};{}'.format(output_filename, mfid)
-                try:
-                    self.spawn([self.mt, '-nologo', '-manifest',
-                                mffilename, out_arg])
-                except DistutilsExecError as msg:
-                    raise LinkError(msg)
         else:
             log.debug("skipping %s (up-to-date)", output_filename)
 
-    def manifest_setup_ldargs(self, output_filename, build_temp, ld_args):
-        # If we need a manifest at all, an embedded manifest is recommended.
-        # See MSDN article titled
-        # "How to: Embed a Manifest Inside a C/C++ Application"
-        # (currently at http://msdn2.microsoft.com/en-us/library/ms235591(VS.80).aspx)
-        # Ask the linker to generate the manifest in the temp dir, so
-        # we can check it, and possibly embed it, later.
-        temp_manifest = os.path.join(
-                build_temp,
-                os.path.basename(output_filename) + ".manifest")
-        ld_args.append('/MANIFESTFILE:' + temp_manifest)
-
-    def manifest_get_embed_info(self, target_desc, ld_args):
-        # If a manifest should be embedded, return a tuple of
-        # (manifest_filename, resource_id).  Returns None if no manifest
-        # should be embedded.  See http://bugs.python.org/issue7833 for why
-        # we want to avoid any manifest for extension modules if we can)
-        for arg in ld_args:
-            if arg.startswith("/MANIFESTFILE:"):
-                temp_manifest = arg.split(":", 1)[1]
-                break
-        else:
-            # no /MANIFESTFILE so nothing to do.
-            return None
-        if target_desc == CCompiler.EXECUTABLE:
-            # by default, executables always get the manifest with the
-            # CRT referenced.
-            mfid = 1
-        else:
-            # Extension modules try and avoid any manifest if possible.
-            mfid = 2
-            temp_manifest = self._remove_visual_c_ref(temp_manifest)
-        if temp_manifest is None:
-            return None
-        return temp_manifest, mfid
-
-    def _remove_visual_c_ref(self, manifest_file):
+    def spawn(self, cmd):
+        old_path = os.getenv('path')
         try:
-            # Remove references to the Visual C runtime, so they will
-            # fall through to the Visual C dependency of Python.exe.
-            # This way, when installed for a restricted user (e.g.
-            # runtimes are not in WinSxS folder, but in Python's own
-            # folder), the runtimes do not need to be in every folder
-            # with .pyd's.
-            # Returns either the filename of the modified manifest or
-            # None if no manifest should be embedded.
-            manifest_f = open(manifest_file)
-            try:
-                manifest_buf = manifest_f.read()
-            finally:
-                manifest_f.close()
-            pattern = re.compile(
-                r"""<assemblyIdentity.*?name=("|')Microsoft\."""\
-                r"""VC\d{2}\.CRT("|').*?(/>|</assemblyIdentity>)""",
-                re.DOTALL)
-            manifest_buf = re.sub(pattern, "", manifest_buf)
-            pattern = "<dependentAssembly>\s*</dependentAssembly>"
-            manifest_buf = re.sub(pattern, "", manifest_buf)
-            # Now see if any other assemblies are referenced - if not, we
-            # don't want a manifest embedded.
-            pattern = re.compile(
-                r"""<assemblyIdentity.*?name=(?:"|')(.+?)(?:"|')"""
-                r""".*?(?:/>|</assemblyIdentity>)""", re.DOTALL)
-            if re.search(pattern, manifest_buf) is None:
-                return None
-
-            manifest_f = open(manifest_file, 'w')
-            try:
-                manifest_f.write(manifest_buf)
-                return manifest_file
-            finally:
-                manifest_f.close()
-        except OSError:
-            pass
+            os.environ['path'] = self._paths
+            return super().spawn(cmd)
+        finally:
+            os.environ['path'] = old_path
 
     # -- Miscellaneous methods -----------------------------------------
     # These are all used by the 'gen_lib_options() function, in
diff --git a/Lib/distutils/tests/test_msvccompiler.py b/Lib/distutils/tests/test_msvccompiler.py
index b440754..1f88907 100644
--- a/Lib/distutils/tests/test_msvccompiler.py
+++ b/Lib/distutils/tests/test_msvccompiler.py
@@ -7,88 +7,6 @@ from distutils.errors import DistutilsPlatformError
 from distutils.tests import support
 from test.support import run_unittest
 
-# A manifest with the only assembly reference being the msvcrt assembly, so
-# should have the assembly completely stripped.  Note that although the
-# assembly has a <security> reference the assembly is removed - that is
-# currently a "feature", not a bug :)
-_MANIFEST_WITH_ONLY_MSVC_REFERENCE = """\
-<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
-<assembly xmlns="urn:schemas-microsoft-com:asm.v1"
-          manifestVersion="1.0">
-  <trustInfo xmlns="urn:schemas-microsoft-com:asm.v3">
-    <security>
-      <requestedPrivileges>
-        <requestedExecutionLevel level="asInvoker" uiAccess="false">
-        </requestedExecutionLevel>
-      </requestedPrivileges>
-    </security>
-  </trustInfo>
-  <dependency>
-    <dependentAssembly>
-      <assemblyIdentity type="win32" name="Microsoft.VC90.CRT"
-         version="9.0.21022.8" processorArchitecture="x86"
-         publicKeyToken="XXXX">
-      </assemblyIdentity>
-    </dependentAssembly>
-  </dependency>
-</assembly>
-"""
-
-# A manifest with references to assemblies other than msvcrt.  When processed,
-# this assembly should be returned with just the msvcrt part removed.
-_MANIFEST_WITH_MULTIPLE_REFERENCES = """\
-<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
-<assembly xmlns="urn:schemas-microsoft-com:asm.v1"
-          manifestVersion="1.0">
-  <trustInfo xmlns="urn:schemas-microsoft-com:asm.v3">
-    <security>
-      <requestedPrivileges>
-        <requestedExecutionLevel level="asInvoker" uiAccess="false">
-        </requestedExecutionLevel>
-      </requestedPrivileges>
-    </security>
-  </trustInfo>
-  <dependency>
-    <dependentAssembly>
-      <assemblyIdentity type="win32" name="Microsoft.VC90.CRT"
-         version="9.0.21022.8" processorArchitecture="x86"
-         publicKeyToken="XXXX">
-      </assemblyIdentity>
-    </dependentAssembly>
-  </dependency>
-  <dependency>
-    <dependentAssembly>
-      <assemblyIdentity type="win32" name="Microsoft.VC90.MFC"
-        version="9.0.21022.8" processorArchitecture="x86"
-        publicKeyToken="XXXX"></assemblyIdentity>
-    </dependentAssembly>
-  </dependency>
-</assembly>
-"""
-
-_CLEANED_MANIFEST = """\
-<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
-<assembly xmlns="urn:schemas-microsoft-com:asm.v1"
-          manifestVersion="1.0">
-  <trustInfo xmlns="urn:schemas-microsoft-com:asm.v3">
-    <security>
-      <requestedPrivileges>
-        <requestedExecutionLevel level="asInvoker" uiAccess="false">
-        </requestedExecutionLevel>
-      </requestedPrivileges>
-    </security>
-  </trustInfo>
-  <dependency>
-
-  </dependency>
-  <dependency>
-    <dependentAssembly>
-      <assemblyIdentity type="win32" name="Microsoft.VC90.MFC"
-        version="9.0.21022.8" processorArchitecture="x86"
-        publicKeyToken="XXXX"></assemblyIdentity>
-    </dependentAssembly>
-  </dependency>
-</assembly>"""
 
 SKIP_MESSAGE = (None if sys.platform == "win32" else
                 "These tests are only for win32")
@@ -114,45 +32,6 @@ class msvccompilerTestCase(support.TempdirManager,
         finally:
             _msvccompiler._find_vcvarsall = old_find_vcvarsall
 
-    def test_remove_visual_c_ref(self):
-        from distutils._msvccompiler import MSVCCompiler
-        tempdir = self.mkdtemp()
-        manifest = os.path.join(tempdir, 'manifest')
-        f = open(manifest, 'w')
-        try:
-            f.write(_MANIFEST_WITH_MULTIPLE_REFERENCES)
-        finally:
-            f.close()
-
-        compiler = MSVCCompiler()
-        compiler._remove_visual_c_ref(manifest)
-
-        # see what we got
-        f = open(manifest)
-        try:
-            # removing trailing spaces
-            content = '\n'.join([line.rstrip() for line in f.readlines()])
-        finally:
-            f.close()
-
-        # makes sure the manifest was properly cleaned
-        self.assertEqual(content, _CLEANED_MANIFEST)
-
-    def test_remove_entire_manifest(self):
-        from distutils._msvccompiler import MSVCCompiler
-        tempdir = self.mkdtemp()
-        manifest = os.path.join(tempdir, 'manifest')
-        f = open(manifest, 'w')
-        try:
-            f.write(_MANIFEST_WITH_ONLY_MSVC_REFERENCE)
-        finally:
-            f.close()
-
-        compiler = MSVCCompiler()
-        got = compiler._remove_visual_c_ref(manifest)
-        self.assertIsNone(got)
-
-
 def test_suite():
     return unittest.makeSuite(msvccompilerTestCase)