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

Fix cross-platform compilation using distutils._msvccompiler.MSVCCompiler #298

Merged
merged 3 commits into from
Sep 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion distutils/_msvccompiler.py
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ def initialize(self, plat_name=None):
f"--plat-name must be one of {tuple(_vcvars_names)}"
)

plat_spec = _get_vcvars_spec(get_host_platform(), get_platform())
plat_spec = _get_vcvars_spec(get_host_platform(), plat_name)

vc_env = _get_vc_env(plat_spec)
if not vc_env:
Expand Down
26 changes: 26 additions & 0 deletions distutils/tests/test_msvccompiler.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@

import os
import sys
import sysconfig
import threading
import unittest.mock as mock
from distutils import _msvccompiler
from distutils.errors import DistutilsPlatformError
from distutils.tests import support
from distutils.util import get_platform

import pytest

Expand All @@ -28,6 +30,30 @@ def _find_vcvarsall(plat_spec):
'wont find this version',
)

@pytest.mark.skipif(
not sysconfig.get_platform().startswith("win"),
Copy link
Contributor Author

@Avasam Avasam Sep 14, 2024

Choose a reason for hiding this comment

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

Alternatively:

Suggested change
not sysconfig.get_platform().startswith("win"),
sysconfig.get_platform() not in _msvccompiler._vcvars_names,

or

Suggested change
not sysconfig.get_platform().startswith("win"),
get_platform() not in _msvccompiler._vcvars_names,

But that's starting to look a lot like tests following the implementation

reason="Only run test for non-mingw Windows platforms",
)
@pytest.mark.parametrize(
"plat_name, expected",
[
("win-arm64", "win-arm64"),
("win-amd64", "win-amd64"),
(None, get_platform()),
],
)
def test_cross_platform_compilation_paths(self, monkeypatch, plat_name, expected):
"""
Ensure a specified target platform is passed to _get_vcvars_spec.
"""
compiler = _msvccompiler.MSVCCompiler()

def _get_vcvars_spec(host_platform, platform):
assert platform == expected

monkeypatch.setattr(_msvccompiler, '_get_vcvars_spec', _get_vcvars_spec)
compiler.initialize(plat_name)

@needs_winreg
def test_get_vc_env_unicode(self):
test_var = 'ṰḖṤṪ┅ṼẨṜ'
Expand Down
1 change: 1 addition & 0 deletions newsfragments/298.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix cross-platform compilation using ``distutils._msvccompiler.MSVCCompiler`` -- by :user:`saschanaz` and :user:`Avasam`
Copy link
Member

Choose a reason for hiding this comment

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

Oh, gosh. I'm realizing now that we can't use these news fragments. They refer to issues in this repository, which is merged into Setuptools, so adding them here will cause setuptools to render them as their own issues. I'll just delete it.

Copy link
Contributor Author

@Avasam Avasam Sep 15, 2024

Choose a reason for hiding this comment

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

Oh right!
Well you could still have newsfragments without issue # if it starts with + (for example: +distutils298.bugfix.rst)

In this case it could refer to the issue I opened on setuptools' repo.

Copy link
Member

Choose a reason for hiding this comment

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

Good idea. I'll aim to do things like that in the future.

Loading