Skip to content

Update msc_ver detection#41

Merged
jaraco merged 6 commits into
pypa:mainfrom
imba-tjd:patch-1
Nov 18, 2021
Merged

Update msc_ver detection#41
jaraco merged 6 commits into
pypa:mainfrom
imba-tjd:patch-1

Conversation

@imba-tjd

@imba-tjd imba-tjd commented May 16, 2021

Copy link
Copy Markdown
Contributor

See pypa/setuptools#2675

After checking the dependencies of those pyd in .whl built by MSVC and python39.dll through objdump -p xxx.pyd | code -, I personally think both ucrt and vcruntime140 should be specified.

Comment thread distutils/cygwinccompiler.py Outdated
return ['msvcr120']
elif int(msc_ver) >= 1900 and int(msc_ver) < 2000:
# VS2015 / MSVC 14.0
return ['ucrt', 'vcruntime140']

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can I confirm these are the right values to confirm? Have you tested distutils/setuptools with this patch? Does it fix the issue reported in the linked discussion?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @lazka

@imba-tjd

imba-tjd commented Jul 4, 2021

Copy link
Copy Markdown
Contributor Author

How can I confirm these are the right values to confirm?

  1. pip download lxml
  2. Use an unarchiver to unzip the whl
  3. objdump -p etree.cp39-win_amd64.pyd, you can see DLL Name: VCRUNTIME140.dll and DLL Name: api-ms-win-crt-stdio-l1-1-0.dll which is ucrt

You can also do this to python39.dll and it turns out the same DLL.

Does it fix the issue reported in the linked discussion

Yes and no. Actually it will introduce another issue. Firstly, VCRUNTIME140.dll is not included in MinGW. When build_ext, the building process won't be stucked at checking which dll is to be linked, but will report ld.exe: cannot find -lvcruntime140.

However I think this get_msvcr function is to get the CRT dlls that are linked with when compiling python39.dll. I personally think it's correct to return ucrt and vcruntime140 because that's exactly what python39.dll is linked with.

@jaraco

jaraco commented Jul 4, 2021

Copy link
Copy Markdown
Member

Thanks for rebasing, where now there are CI tests. Unfortunately, now you can see that the tests fail with this change, so clearly there's more to do.

@isuruf

isuruf commented Nov 15, 2021

Copy link
Copy Markdown
Contributor

Looks like we just need to update the tests

@imba-tjd imba-tjd marked this pull request as ready for review November 15, 2021 08:45
@imba-tjd imba-tjd marked this pull request as draft November 15, 2021 08:46
@imba-tjd

imba-tjd commented Nov 15, 2021

Copy link
Copy Markdown
Contributor Author

I have added test case for test_cygwinccompiler.py.

The impact should be small once merged. Because the default compiler is MSVC. Mingw is opt-in, and it can't work for years.

What will be changed is that when users try to use Mingw, they will get the error of ld.exe: cannot find -lvcruntime140 rather than ValueError: Unknown MS Compiler.


Besides in GitHub codespaces it fails when I use tox on main (i.e. without my change):

============================================ FAILURES =============================================
__________________________ DirUtilTestCase.test_mkpath_with_custom_mode ___________________________

self = <distutils.tests.test_dir_util.DirUtilTestCase testMethod=test_mkpath_with_custom_mode>

    @unittest.skipIf(sys.platform.startswith('win'),
        "This test is only appropriate for POSIX-like systems.")
    def test_mkpath_with_custom_mode(self):
        # Get and set the current umask value for testing mode bits.
        umask = os.umask(0o002)
        os.umask(umask)
        mkpath(self.target, 0o700)
        self.assertEqual(
            stat.S_IMODE(os.stat(self.target).st_mode), 0o700 & ~umask)
        mkpath(self.target2, 0o555)
>       self.assertEqual(
            stat.S_IMODE(os.stat(self.target2).st_mode), 0o555 & ~umask)
E       AssertionError: 364 != 365

distutils/tests/test_dir_util.py:66: AssertionError
===================================== short test summary info =====================================
FAILED distutils/tests/test_dir_util.py::DirUtilTestCase::test_mkpath_with_custom_mode - Asserti...
============================ 1 failed, 263 passed, 30 skipped in 5.25s ============================
ERROR: InvocationError for command /workspaces/distutils/.tox/python/bin/pytest (exited with code 1)
_____________________________________________ summary _____________________________________________
ERROR:   python: commands failed

@imba-tjd imba-tjd marked this pull request as ready for review November 16, 2021 10:22
@isuruf

isuruf commented Nov 16, 2021

Copy link
Copy Markdown
Contributor

@jaraco, can you approve for the CI to run?

@imba-tjd

Copy link
Copy Markdown
Contributor Author

@njsmith Hi, would you mind reviewing this? Since you originally reviewd numpy/pull/6875

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants