Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bdist_wininst installers fail to load extensions built with Issue4120 patch #52081

Closed
cgohlke mannequin opened this issue Feb 1, 2010 · 38 comments
Closed

bdist_wininst installers fail to load extensions built with Issue4120 patch #52081

cgohlke mannequin opened this issue Feb 1, 2010 · 38 comments
Assignees
Labels
OS-windows stdlib Python modules in the Lib dir

Comments

@cgohlke
Copy link
Mannequin

cgohlke mannequin commented Feb 1, 2010

BPO 7833
Nosy @loewis, @mhammond, @vstinner, @ericvsmith, @tjguk, @tarekziade, @merwok, @zware, @zooba, @dstufft
Files
  • bdist_wininst.vcproj.patch
  • test-bdist_wininst.zip
  • msvc9compiler_stripruntimes_revised.patch
  • msvc9compiler_stripruntimes_trunk.patch
  • bug-7833-overridable-manifest-settings.patch: patch with same behaviour but more override friendly
  • bug-7833-overridable-manifest-settings-with-test.patch
  • bug-7833-tweaks-plus-news.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/mhammond'
    closed_at = <Date 2021-01-11.12:51:13.069>
    created_at = <Date 2010-02-01.22:12:51.098>
    labels = ['library', 'OS-windows']
    title = 'bdist_wininst installers fail to load extensions built with Issue4120 patch'
    updated_at = <Date 2021-01-11.14:02:38.455>
    user = 'https://bugs.python.org/cgohlke'

    bugs.python.org fields:

    activity = <Date 2021-01-11.14:02:38.455>
    actor = 'mark.dickinson'
    assignee = 'mhammond'
    closed = True
    closed_date = <Date 2021-01-11.12:51:13.069>
    closer = 'vstinner'
    components = ['Distutils', 'Windows']
    creation = <Date 2010-02-01.22:12:51.098>
    creator = 'cgohlke'
    dependencies = []
    files = ['16086', '16088', '16121', '17286', '23305', '23341', '23400']
    hgrepos = []
    issue_num = 7833
    keywords = ['patch']
    message_count = 38.0
    messages = ['98694', '98696', '98699', '98805', '104316', '104318', '104341', '104382', '105473', '106855', '106867', '128775', '129610', '129611', '129612', '129617', '130205', '144806', '145004', '145048', '145116', '145117', '145151', '145206', '145210', '145211', '145496', '145534', '145654', '145655', '145691', '145692', '152447', '152448', '152585', '176062', '233204', '384818']
    nosy_count = 20.0
    nosy_names = ['loewis', 'mhammond', 'vstinner', 'sable', 'eric.smith', 'techtonik', 'tim.golden', 'koen', 'tarek', 'eric.araujo', 'cgohlke', 'Garen', 'alexis', 'John.Cary', 'python-dev', 'abenard', 'almar', 'zach.ware', 'steve.dower', 'dstufft']
    pr_nums = []
    priority = 'normal'
    resolution = 'wont fix'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue7833'
    versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

    @cgohlke
    Copy link
    Mannequin Author

    cgohlke mannequin commented Feb 1, 2010

    Wininst-9.0.exe and wininst-9.0-amd64.exe are missing MSVCRT90 dependencies in the embedded manifest.

    While testing installers of pywin32 <http://sourceforge.net/projects/pywin32/\> version 214 built with Python 2.6.4 and the msvc9compiler_stripruntimes_regexp2.diff patch from bpo-4120 <http://bugs.python.org/issue4120\>, I noticed that the post_install script fails with an ImportError while trying to load extensions without embedded MSVCRT90 manifest:

    Traceback (most recent call last):
      File "<string>", line 601, in <module>
      File "<string>", line 311, in install
      File "<string>", line 149, in LoadSystemModule
    ImportError: DLL load failed: The specified module could not be found.

    The code that fails is:

        mod = imp.load_module(modname, None, filename, ('.dll', 'rb', imp.C_EXTENSION))

    where modname='pywintypes' and filename points to an existing 'pywintypes26.dll' file.

    The post_install script runs fine when executed from the main python.exe interpreter.

    A possible fix is to embed a MSVCRT90 dependency into wininst-9.0.exe and wininst-9.0-amd64.exe using the following linker switches:

    /MANIFESTDEPENDENCY:"type='Win32' name='Microsoft.VC90.CRT' version='9.0.21022.8' processorArchitecture='X86' publicKeyToken='1fc8b3b9a1e18e3b'"

    /MANIFESTDEPENDENCY:"type='Win32' name='Microsoft.VC90.CRT' version='9.0.21022.8' processorArchitecture='amd64' publicKeyToken='1fc8b3b9a1e18e3b'"

    See also <http://msdn.microsoft.com/en-us/library/ew0y5khy.aspx\>

    A patch against the Python 2.6 PCbuild\bdist_wininst.vcproj is attached. I tested it with Python 2.6.4, 32 and 64 bit, and pywin32 214 installers built with the following command:

    python.exe setup.py bdist_wininst --user-access-control=auto --install-script=wxpython_win_post_install.py

    @cgohlke cgohlke mannequin assigned tarekziade Feb 1, 2010
    @cgohlke cgohlke mannequin added stdlib Python modules in the Lib dir OS-windows labels Feb 1, 2010
    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Feb 1, 2010

    This doesn't look right. IIUC, wininst.exe loads python26.dll, which in turn loads pywintypes.dll. So it should be python26.dll which loads the CRT.

    IIUC, wininst.exe is linked statically, precisely to avoid a dependency on the CRT assembly.

    @cgohlke
    Copy link
    Mannequin Author

    cgohlke mannequin commented Feb 2, 2010

    The last line of my previous post should actually read
    python.exe setup.py bdist_wininst

    Anyway, here are three files (also attached) that can reproduce the problem:

    1. setup.py
    from distutils.core import setup, Extension
    setup(name='testpyd', scripts = ["testpyd_postinstall.py"],
        ext_modules=[Extension('testpyd', ['testpyd.c'],)],
        options = {"bdist_wininst": {"install_script": 
            "testpyd_postinstall.py", "user_access_control": "auto"},})
    1. testpyd.c
    #include "Python.h"
    PyMethodDef methods[] = {{NULL, NULL},};
    void inittestpyd() {(void)Py_InitModule("testpyd", methods);}
    1. testpyd_postinstall.py
    #!python
    import testpyd

    Build the installer with python 2.6 and bpo-4120 patch applied:
    python setup.py bdist_wininst

    Run the installer:
    dist\testpyd-0.0.0.win32-py2.6.exe

    The postinstall script fails with:

    Traceback (most recent call last):
      File "<string>", line 1, in <module>
    ImportError: DLL load failed: The specified module could not be found.

    According to Sysinternals process monitor python26.dll is loaded from the system32 directory, the testpyd.pyd extension is found at the right place, and then the Windows path is searched in vain for MSVCR90.DLL.

    Tested on Windows 7 Pro 64 bit.

    @cgohlke
    Copy link
    Mannequin Author

    cgohlke mannequin commented Feb 4, 2010

    I thought one conclusion of the discussion on bpo-4120 was that any executable, which embeds Python and imports MSVCR9 dependent extensions, must now provide the manifest for the MSVCR9 runtimes, either embedded or as a separate file. See <http://bugs.python.org/issue4120#msg94644\> and responses. Why shouldn't this apply to wininst executables?

    Another observation: importing the winsound extension in the postinstall script works even though it also depends on MSVCR90.DLL. Winsound.pyd does not have an embedded manifest. It is linked with /MANIFEST:NO. Apparently there is a difference between an "empty" manifest and no manifest.

    How about a patch that does:

    1. not embed any manifest into PYD files if the manifest contains only a single MSVCR90 dependentAssembly element.

    2. remove the MSVCR90 dependentAssembly element from manifests embedded into PYD files if the manifest defines additional dependentAssembly elements (e.g. for MFC or Common Controls). This is essentially what the msvc9compiler_stripruntimes patch does now.

    3. not embed manifests into any file (EXE, PYD, or DLL) if '/MANIFEST:NO' is defined in extra_link_args. Extension developers can then provide external manifest files.

    4. not touch the default manifests embedded into EXE and DLL files. The msvc9compiler_stripruntimes currently produces DLLs that can not be used standalone, such as pythoncom26.dll from the pywin32 package (I can file a separate bug if requested). Pythoncom26.dll is meant to be placed in the system32 folder and to be used outside of a Python environment, i.e. from the Windows Scripting Host. Several pywin32 tests fail with the pythoncom26.dll built with the msvc9compiler_stripruntimes patch. Placing a MSVCR9 manifest file into the system32 folder next to the pythoncom26.dll is not an option.

    A tentative patch against the Python 2.6 branch is attached. I will test it further.

    (1) and (4) will solve the original pywin32 wininstaller problem without changing wininst.exe.

    As it is now the installer and some functionality of the pywin32 package will likely break if pywin32 is built on Python 2.6.5.

    @koen
    Copy link
    Mannequin

    koen mannequin commented Apr 27, 2010

    There should be no manifest embedded into wininst, because then the cases which bpo-4120 fixed (a CRT installed into a local folder, instead of system-wide, due to limited access rights), will 'break' again: the installer can then no longer work unless there is a system-wide installation of the CRT.

    Option #1, #2 and #3 all sound reasonable (and #2 is the current situation).

    I have some doubts about option #4: it is a very specific use case, and then the whole benefit of bpo-4120 is lost, because stripping runtimes would have to be removed again. Why is putting a separate manifest file next to the DLL not an option? Combined with #3 (allow extension developer to disable embedding of manifests) a separate manifest can fix the problem.

    @koen
    Copy link
    Mannequin

    koen mannequin commented Apr 27, 2010

    Concerning the patch: what happens when the developer already added /MANIFEST:NO to the flags, and the code deduces that MSVCR9 is the only runtime, e.g. the case where /MANIFEST:NO is in the flags twice? Does the linker handle this OK, or does there need to be an additional check as to not have it twice?

    @cgohlke
    Copy link
    Mannequin Author

    cgohlke mannequin commented Apr 27, 2010

    I have some doubts about option #4: it is a very specific use case, and then the whole benefit of bpo-4120 is lost

    Pythoncom are pywintypes are indeed special cases: Out of the 170 DLL files in my Python site-packages directory, these seem to be the only ones built with distutils. All other DLLs are apparently built without Python involvement using make, nmake, CMake, or Visual Studio and most of them contain embedded manifests, which is the default when using nmake, CMake, or Visual Studio. Practically, to make a standalone distribution of any Python 2.6/3.1 application with external DLL dependencies likely requires to provide external manifest files. The bpo-4120 patch does not change this situation and I don't see any sane way to patch Python/distutils that could. The main benefit of the bpo-4120 patch, as I see it, is that PYD files produced by distutils work in a standalone distribution without any further attention. Msvc9compiler_stripruntimes_revised.patch does not change this.

    My reasoning for this patch (besides fixing the bdist_wininst installer issue) was to allow the popular pywin32 package to build without changes, and offer a way for other extension packages to exclude manifests from DLL files if required (apparently not that common). Alternatively one could provide a mechanism to embed specific manifests into DLLs. Is that currently possible? Then pywin32 setup.py could be fixed.

    Why is putting a separate manifest file next to the DLL not an option?

    Because the pythoncom dll is currently installed into the Windows system directory. Putting manifest files there will pollute the system directory even more and possibly interfere with other system components if not done right (not tested). But again, pywin32 setup.py could be fixed to not install the DLL/manifest files into the system directory.

    Which Python packages other than pywin32 build DLL files via distutils? I don't know any.

    Can anyone provide a minimal setup.py script and C file that produces a DLL file for testing?

    @mhammond
    Copy link
    Contributor

    the pywin32 DLLs have 2 heads. They are Python extension modules as well as regular DLLs. They are built by distutils and therefore have no manifests - I think many packages use distutils to build their extension modules - it is just that they usually don't have a .dll extension.

    I fear that simply adding a manifest to those DLLs will put them in the same position we have before bpo-4120 was addressed, and these .dlls do need a way to be installed into System32 (or somewhere else on the global PATH) to function as COM servers etc. I need to experiment with this.

    @cgohlke
    Copy link
    Mannequin Author

    cgohlke mannequin commented May 10, 2010

    The bdist_wininst and DLL build issues also exist in Python 2.7b2. A patch against svn trunk is attached.

    The pywin32 v214 package fails as reported earlier when built with Python 2.7b2. It installs and functions when built with this patch.

    @techtonik
    Copy link
    Mannequin

    techtonik mannequin commented Jun 1, 2010

    I see that this issue mentions --user-access-control option. Can somebody also check if bpo-8870 and bpo-8871 are related to this one?

    @koen
    Copy link
    Mannequin

    koen mannequin commented Jun 1, 2010

    bpo-8870 and bpo-8871 are not related to this one. There, the UAC elevation fails, here the issue is with the MS runtimes, elevation is working fine.

    @mhammond
    Copy link
    Contributor

    I'm failing to get a new pywin32 out of the door due to this issue. I've spent a few hours playing with this and I think the analysis is generally correct here. The key thing is that when using distutils, the extensions end up with a manifest (albeit one without a reference to the CRT) whereas the extensions shipped with Python have no manifest at all.

    I agree with Martin that it seems strange the CRT fails to be used even though the CRT is obviously already loaded, but it seems to be a fact. I can't find much on this, but suspect it relates to the different "activation contexts" in use and how the activation contexts are designed to allow side-by-side loading of DLLs; Windows doesn't know if the version of the DLL already loaded is suitable. I also guess that the fact the DLL has *any* manifest means they use a more strict interpretation of things (ie, refuse to use already loaded ones) whereas a dll with no manifest gets given a little more slack.

    I can confirm that with the attached patch, pywin32 builds and installs fine on a machine without the CRT installed globally - so I'm +1 on this patch with one caveat: The check for '.pyd' should either be expanded to include '.dll', or else the check should just use the 'target_desc == CCompiler.EXECUTABLE' condition already used.

    I'm happy to make the change once I get some feedback and/or guidance about where I should check this in - I believe it is too late for python 2.6 which is a shame...

    @mhammond
    Copy link
    Contributor

    Thinking more about this, I think the approach of this patch is more complex than necessary. I think a better patch would be one which *unconditionally* removes the manifest from extension modules. For maximum flexibility though, we should probably allow a hook so setup.py can specify the name of (or contents of) a manifest file to use when linking with the default being None.

    @cgohlke
    Copy link
    Mannequin Author

    cgohlke mannequin commented Feb 27, 2011

    The proposed patch was meant to be backwards compatible. Unconditionally removing the whole assembly/manifest from extensions could break extensions that have additional dependencies, such as MFC or Common Controls. PyQt4 extensions for example depend on Common Controls.

    Btw, there is a discussion at <http://psycopg.lighthouseapp.com/projects/62710/tickets/20\> and <http://groups.google.com/group/modwsgi/browse_thread/thread/137f88ac6927df59\> about the need to put the msvcr90 manifest back into the psycopg.pyd file in order to work properly under Apache's mod_wsgi, which is linked against msvcrt.

    @mhammond
    Copy link
    Contributor

    I'm wondering if, in practice, extensions which need a manifest can have the manifest being generated completely by the linker - ie, I expect that in most cases where something other than the CRT or MFC is needed in the manifest, the author will want to specify an explicit manifest file rather than one generated by the linker.

    And worse, those extensions are going to be screwed anyway - the fact the manifest remains at all will mean they probably fail to load for reasons already discussed in this bug. IOW, not having a manifest at all is about the only way to ensure the module will be loaded correctly.

    Re adding a manifest back in - I've struck the exact same problem with pywin32 - any DLLs which are "entry points" into Python need to have a manifest - eg, pythoncomxx and a few other pywin32 ones. This left me with a dilemma for pythoncom - as it is both an "entry-point" (ie, COM loads that DLL) and a regular Python module, I simultaneously needed a manifest (to work when loaded by COM) and needed to *not* have one (to work when loaded by Python). My solution has been to introduce another DLL with a manifest and have COM servers register using that. This strategy seems to be working well in all my tests.

    @cgohlke
    Copy link
    Mannequin Author

    cgohlke mannequin commented Feb 27, 2011

    Actually, PyQt4 was not a good example since it is not build using distutils. Pywin32 and wxPython extensions are the only ones on my system specifying a dependency on Common Controls or MFC. No dependencies other than CRT, MFC, and Common Controls are specified in any assemblies of over 1000 pyd and dll files in my Python27 directory.

    @JohnCary
    Copy link
    Mannequin

    JohnCary mannequin commented Mar 6, 2011

    Just to follow up. My case is an application that is almost
    all statically linked, but it loads in the python library, and
    at runtime it needs to load the tables module, and as distributed
    by Python, I get the load-time error on Windows.

    Using Christoph Gohlke's exe's works great for me, but I cannot
    redistribute due to the linking in of szip for tables and
    MKL for numpy. So I build my own version of tables without
    szip and numpy without MKL, but that failed until I applied
    Christoph's patch on Python. (I also have to patch tables'
    setup.py not to include szip.dll and zlib1.dll in dll_files.)

    So the result is that with Christoph's patch, all is working
    for me. I hope it (or something similar) makes it into 2.7.

    John Cary

    @mhammond
    Copy link
    Contributor

    mhammond commented Oct 3, 2011

    This is biting people (including me :) so I'm going to try hard to get this fixed. One user on the python-win32 mailing list resorts to rebuilding every 3rd party module he uses with this patch to get things working again (although apps which use only builtin modules or pywin32 modules - which already hacks this basic functionality in - don't see the problem.)

    I'm attaching a different patch that should have the same default effect as Christoph's, but also allows the behaviour to be overridden. Actually overriding it is quite difficult as distutils isn't setup to easily allow such compiler customizations - but at least it *is* possible. To test this out I hacked both the pywin32 and py2erxe build processes to use those customizations and it works fine and allows them both to customize the behaviour to meet various modules' requirements.

    Finally, this whole thing is still fundamentally broken for extensions which need a manifest (eg, to reference the common controls or the requestedExecutionLevel cruft). These extension will still need to include the CRT reference in their manifest and thus will need a copy of the CRT next to each of them. I *think* this also means they basically get a private copy of the CRT - they are not sharing the CRT with Python, which means they are at risk of hitting problems such as trying to share FILE * objects. In practice, this means such modules are probably better of just embedding the CRT statically. This is the route I've taken for one pywin32 module so the module can have a manifest and still work without a complete, private copy of the CRT needing to live next to it. But even with that problem I think this patch should land.

    It would be great if someone can review and test this version of the patch and I'll check it in.

    @merwok
    Copy link
    Member

    merwok commented Oct 6, 2011

    Can the patch include regression tests?

    @merwok merwok changed the title Bdist_wininst installers fail to load extensions built with Issue4120 patch bdist_wininst installers fail to load extensions built with Issue4120 patch Oct 6, 2011
    @mhammond
    Copy link
    Contributor

    mhammond commented Oct 6, 2011

    I'm reluctant to commit to adding test infrastructure for the distutils build commands - if I've missed existing infrastructure and adding such tests would actually be relatively simple, please educate me! Or if someone else would like to help with the infrastructure so I can test just this patch, that would be awesome. But I don't think this fix should block on tests given it can easily be tested and verified manually.

    @merwok
    Copy link
    Member

    merwok commented Oct 7, 2011

    There is a already a test for manifest things in Lib/distutils/tests/test_msvc9compiler.py, and higher-level tests for building extension modules in Lib/distutils/tests/test_build_ext.py and Lib/distutils/tests/test_install.py.

    @merwok
    Copy link
    Member

    merwok commented Oct 7, 2011

    Let me add that I don’t know the MS toolchain at all, but if you tell me what a test should do (e.g. compile a C extension, open some compiled file and look for some bytes) I can write a test.

    @mhammond
    Copy link
    Contributor

    mhammond commented Oct 8, 2011

    My apologies Eric - I had completely overlooked those tests. Attaching a new patch with a test. Note the existing test doesn't actually perform a build so the new test also doesn't, but it does check the core logic (ie, that a manifest with only the msvcrt reference gets it scrubbed).

    @merwok
    Copy link
    Member

    merwok commented Oct 9, 2011

    Note the existing test doesn't actually perform a build so the new
    test also doesn't, but it does check the core logic
    Sounds good to me.

    + def manifest_setup_ldargs
    I’d make all new methods private ones (i.e. leading underscore).

    an embedded manifests
    Typo: extra s

    return None if not temp_manifest else (temp_manifest, mfid)
    Using a ternary expression runs afoul of PEP-291: distutils should remain compatible with 2.3. (I’m not sure it is right now, we use modern unittest methods in tests and all, but it is no reason for making it worse in new code :)

    Your patch will also need an entry in Misc/NEWS; at first glance, there is no documentation file to edit.

    Will you port the patch to packaging in 3.3? I can do it if you don’t have the time, but I’m not set up yet to test on Windows, so I can ask you to test a patch. Also, for the distutils2 backport (which I can do too), we would need to run tests with all versions from 2.4 to 3.3.

    @mhammond
    Copy link
    Contributor

    mhammond commented Oct 9, 2011

    Thanks for the review. One note:

    | + def manifest_setup_ldargs
    | I’d make all new methods private ones (i.e. leading underscore).

    They aren't strictly private and are designed to be overridden by subclasses (although in practice, subclassing the compiler is much harder than it should be, so pywin32 monkey-patches the instance.) This is actually the entire point of my updated patch - to give setup.py authors some level of control over the manifest behaviour.

    I do intend forward-porting to 3.3 and also to check it is is too late for 3.2 (a quick check before implied it might be OK, but I'm not sure)

    @merwok
    Copy link
    Member

    merwok commented Oct 9, 2011

    They aren't strictly private and are designed to be overridden by
    subclasses
    OK.

    I do intend forward-porting to 3.3 and also to check it is is too late
    for 3.2 (a quick check before implied it might be OK, but I'm not sure)
    2.7 and 3.2 are open for bug fixes, as indicated by the versions field of this bug (it’s actually a matrix: component distutils + versions 2.7, 3.2, 3.3, component distutils2 + version 3.3 == packaging and distutils2 + third-party == the distutils2 backport :)

    @merwok merwok assigned mhammond and unassigned tarekziade Oct 9, 2011
    @mhammond
    Copy link
    Contributor

    New version of the patch with the small tweaks requested plus a NEWS entry.

    @merwok
    Copy link
    Member

    merwok commented Oct 14, 2011

    Looks good.

    Style nit: I don’t put backslashes at end-of-lines in parens (in one re.compile call you have that). Also, I use -- where I can’t use —.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 17, 2011

    New changeset fb886858024c by Mark Hammond in branch '2.7':
    Issue bpo-7833: Ext. modules built using distutils on Windows no longer get a manifest
    http://hg.python.org/cpython/rev/fb886858024c

    New changeset 9caeb7215344 by Mark Hammond in branch '3.2':
    Issue bpo-7833: Ext. modules built using distutils on Windows no longer get a manifest
    http://hg.python.org/cpython/rev/9caeb7215344

    New changeset 3073ef853647 by Mark Hammond in branch 'default':
    Issue bpo-7833: Ext. modules built using distutils on Windows no longer get a manifest
    http://hg.python.org/cpython/rev/3073ef853647

    @mhammond
    Copy link
    Contributor

    I pushed the changes to 2.7, 3.2 and 3.3. I'm happy to help with distutils2/packaging but I'll need to do that later once I work out where to start :) Therefore I'm not yet closing this issue.

    @merwok
    Copy link
    Member

    merwok commented Oct 17, 2011

    To port the patch to packaging, go into your CPython 3.3 checkout and edit Lib/packaging/compiler/msvc9compiler.py (and its test :).

    To port the patch to distutils2, clone http://hg.python.org/distutils2/ and edit distutils/compiler/msvc9compiler.py (same :). Test with Python 2.4, 2.5, 2.6 and 2.7. Then hg update python3, hg merge default, test with Python 3.1, 3.2 and 3.3. Then you can push :)

    If you don’t have the necessary Pythons or roundtuits, I’ll port the patch when I get a Windows VM. There’s plenty of time before Python 3.3 is out.

    @merwok
    Copy link
    Member

    merwok commented Oct 17, 2011

    s/distutils/distutils2/ !

    @mdickinson
    Copy link
    Member

    Mark: Possibly a stupid question, but in your commit (see snippet below), why is the processorArchitecture hard coded to "x86"? Is it appropriate to replace this with "amd64" for 64-bit builds, or should it always be "x86"?

    + <dependentAssembly>
    + <assemblyIdentity type="win32" name="Microsoft.VC90.CRT"
    + version="9.0.21022.8" processorArchitecture="x86"
    + publicKeyToken="XXXX">
    + </assemblyIdentity>
    + </dependentAssembly>

    @mhammond
    Copy link
    Contributor

    mhammond commented Feb 2, 2012

    ack - that is a really good point. IIRC it can be "*". I'll try and look at this over the next day or 2.

    @mhammond
    Copy link
    Contributor

    mhammond commented Feb 4, 2012

    Actually, I think this is OK - the reference to the "x86" is in the tests and those tests don't actually perform a build, just check the manifest is detected and stripped (ie, the test should still work fine on 64bit boxes). Ideally the test could also check a manifest with a 64bit architecture, but I don't think that's really necessary...

    @almar
    Copy link
    Mannequin

    almar mannequin commented Nov 21, 2012

    Just checking in to point out a possible problem with the code that strips the MSVCR dependency from the embedded manifest. The used regexpr is too greedy: the first bit can trigger on an earlier "assemblyIdentity" tag, so that after the removal the manifest is no longer valid XML.

    The key problem is that the "<assemblyIdentity" and the name attribute are allowed to be on a separate lines. To fix this I removed the re.DOTALL flag and replaced the second occurrence of ".*?" to also allow newlines:

      pattern = re.compile(
    -     r"""<assemblyIdentity.*?name=("|')Microsoft\."""\
    -     r"""VC\d{2}\.CRT("|').*?(/>|</assemblyIdentity>)""",
    -     re.DOTALL)
    +     r"""<assemblyIdentity.*?name=("|')Microsoft\."""\
    +     r"""VC\d{2}\.CRT("|')(.|\r|\r)*?(/>|</assemblyIdentity>)""")

    It is well possible that the problem does not causes any problems for the intended usage of the code. I'm using the code to strip other DLL's and ran into this issue (with tk85.dll to be precise).

    • Almar

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Dec 30, 2014

    This is referenced from bpo-4431 which has been closed for over six years but keeps getting comments.

    @vstinner
    Copy link
    Member

    The distutils bdist_wininst command has been removed in Python 3.10: see bpo-42802.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    OS-windows stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants