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

Update msc_ver detection #41

Merged
merged 6 commits into from
Nov 18, 2021
Merged

Update msc_ver detection #41

merged 6 commits into from
Nov 18, 2021

Conversation

imba-tjd
Copy link
Contributor

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

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.

distutils/cygwinccompiler.py Outdated Show resolved Hide resolved
return ['msvcr120']
elif int(msc_ver) >= 1900 and int(msc_ver) < 2000:
# VS2015 / MSVC 14.0
return ['ucrt', 'vcruntime140']
Copy link
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
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
Copy link
Contributor Author

imba-tjd commented Jul 4, 2021

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
Copy link
Member

jaraco commented Jul 4, 2021

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
Copy link
Contributor

isuruf commented Nov 15, 2021

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
Copy link
Contributor Author

imba-tjd commented Nov 15, 2021

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
Copy link
Contributor

isuruf commented Nov 16, 2021

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

@imba-tjd
Copy link
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