distutils icon indicating copy to clipboard operation
distutils copied to clipboard

Distutils C++ support

Open jaraco opened this issue 1 year ago • 11 comments

Upstreamed fix from nix, see patch here: https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/python-modules/setuptools/setuptools-distutils-C%2B%2B.patch

Resubmit of #221.

jaraco avatar Feb 13 '24 18:02 jaraco

@sciyoshi Would you be willing to investigate the two failures and suggest a fix?

jaraco avatar Feb 13 '24 18:02 jaraco

@jaraco sure. Not sure why the tests would have started failing after, but this patch fixes the tests for me:

diff --git a/distutils/sysconfig.py b/distutils/sysconfig.py
index bc579163..4e1cd3ac 100644
--- a/distutils/sysconfig.py
+++ b/distutils/sysconfig.py
@@ -335,7 +335,7 @@ def customize_compiler(compiler):  # noqa: C901
             ldshared = ldshared + ' ' + os.environ['LDFLAGS']
             ldcxxshared = ldcxxshared + ' ' + os.environ['LDFLAGS']
         if 'CFLAGS' in os.environ:
-            cflags = os.environ['CFLAGS']
+            cflags = cflags + ' ' + os.environ['CFLAGS']
             ldshared = ldshared + ' ' + os.environ['CFLAGS']
         if 'CXXFLAGS' in os.environ:
             cxxflags = os.environ['CXXFLAGS']
diff --git a/distutils/tests/test_sysconfig.py b/distutils/tests/test_sysconfig.py
index 6cbf5168..3177c6e1 100644
--- a/distutils/tests/test_sysconfig.py
+++ b/distutils/tests/test_sysconfig.py
@@ -133,7 +133,7 @@ class TestSysconfig:
         assert comp.exes['compiler_so'] == (
             'env_cc --sc-cflags ' '--env-cflags ' '--env-cppflags --sc-ccshared'
         )
-        assert comp.exes['compiler_cxx'] == 'env_cxx --env-cxx-flags'
+        assert comp.exes['compiler_cxx'] == 'env_cxx --env-cxx-flags --sc-cflags --env-cppflags'
         assert comp.exes['linker_exe'] == 'env_cc'
         assert comp.exes['linker_so'] == (
             'env_ldshared --env-ldflags --env-cflags' ' --env-cppflags'
@@ -161,7 +161,7 @@ class TestSysconfig:
         assert comp.exes['preprocessor'] == 'sc_cc -E'
         assert comp.exes['compiler'] == 'sc_cc --sc-cflags'
         assert comp.exes['compiler_so'] == 'sc_cc --sc-cflags --sc-ccshared'
-        assert comp.exes['compiler_cxx'] == 'sc_cxx'
+        assert comp.exes['compiler_cxx'] == 'sc_cxx --sc-cflags'
         assert comp.exes['linker_exe'] == 'sc_cc'
         assert comp.exes['linker_so'] == 'sc_ldshared'
         assert comp.shared_lib_extension == 'sc_shutil_suffix'
diff --git a/distutils/tests/test_unixccompiler.py b/distutils/tests/test_unixccompiler.py
index c1e57a01..165d9944 100644
--- a/distutils/tests/test_unixccompiler.py
+++ b/distutils/tests/test_unixccompiler.py
@@ -247,9 +247,13 @@ class TestUnixCCompiler(support.TempdirManager):
         def gcv(v):
             if v == 'LDSHARED':
                 return 'gcc-4.2 -bundle -undefined dynamic_lookup '
+            elif v == 'LDCXXSHARED':
+                return 'g++-4.2 -bundle -undefined dynamic_lookup '
             elif v == 'CXX':
                 return 'g++-4.2'
-            return 'gcc-4.2'
+            elif v == 'CC':
+                return 'gcc-4.2'
+            return ''

         def gcvs(*args, _orig=sysconfig.get_config_vars):
             if args:

Again, I'm not fully familiar with the intricacies involved, but the change in sysconfig.py seems to probably be an oversight, and the remaining changes in the tests are probably expected differences.

sciyoshi avatar Feb 13 '24 19:02 sciyoshi

Thanks! After applying that patch, there's one remaining failure in the pypy tests.

            if 'LDFLAGS' in os.environ:
                ldshared = ldshared + ' ' + os.environ['LDFLAGS']
>               ldcxxshared = ldcxxshared + ' ' + os.environ['LDFLAGS']
E               TypeError: unsupported operand type(s) for +: 'NoneType' and 'str'

distutils/sysconfig.py:336: TypeError

It appears that LDCXXSHARED is not defined in pypy:

>>>> import sysconfig
>>>> sysconfig.get_config_vars
<function get_config_vars at 0x000000011855d880>
>>>> sysconfig.get_config_vars('LDCXXSHARED')
[None]

jaraco avatar Feb 14 '24 00:02 jaraco

@jaraco yes I'm not sure how that would have worked before, but in any case it's probably easiest to change it like this:

diff --git a/distutils/sysconfig.py b/distutils/sysconfig.py
index 4e1cd3ac..937482a4 100644
--- a/distutils/sysconfig.py
+++ b/distutils/sysconfig.py
@@ -296,7 +296,6 @@ def customize_compiler(compiler):  # noqa: C901
             cflags,
             ccshared,
             ldshared,
-            ldcxxshared,
             shlib_suffix,
             ar,
             ar_flags,
@@ -306,13 +305,13 @@ def customize_compiler(compiler):  # noqa: C901
             'CFLAGS',
             'CCSHARED',
             'LDSHARED',
-            'LDCXXSHARED',
             'SHLIB_SUFFIX',
             'AR',
             'ARFLAGS',
         )

         cxxflags = cflags
+        ldcxxshared = ""

         if 'CC' in os.environ:
             newcc = os.environ['CC']

sciyoshi avatar Mar 01 '24 14:03 sciyoshi

More test failures? 😭

sciyoshi avatar Mar 02 '24 03:03 sciyoshi

I applied that diff, but unfortunately, that led to more failures in test_cc_overrides_ldshared_for_cxx_correctly... and while debugging the issue, I found that test is itself fragile - it fails differently depending on whether it's run in isolation or not (even if before the patch).

 distutils 765bbbd7 @ tox -e py311 -- -p no:cov -k test_cc_overrides_ldshared_for_cxx_correctly
.pkg-cpython311: _optional_hooks> python '/Users/jaraco/Library/Application Support/pipx/venvs/tox/lib/python3.12/site-packages/pyproject_api/_backend.py' True setuptools.build_meta
.pkg-cpython311: get_requires_for_build_editable> python '/Users/jaraco/Library/Application Support/pipx/venvs/tox/lib/python3.12/site-packages/pyproject_api/_backend.py' True setuptools.build_meta
.pkg-cpython311: build_editable> python '/Users/jaraco/Library/Application Support/pipx/venvs/tox/lib/python3.12/site-packages/pyproject_api/_backend.py' True setuptools.build_meta
py311: install_package> python -I -m pip install --force-reinstall --no-deps /Users/jaraco/code/pypa/distutils/.tox/.tmp/package/20/distutils-0.1.dev3944+g765bbbd-0.editable-py3-none-any.whl
py311: commands[0]> pytest -p no:cov -k test_cc_overrides_ldshared_for_cxx_correctly
============================================================== test session starts ===============================================================
platform darwin -- Python 3.11.8, pytest-8.0.2, pluggy-1.4.0
cachedir: .tox/py311/.pytest_cache
rootdir: /Users/jaraco/code/pypa/distutils
configfile: pytest.ini
plugins: enabler-3.0.0, checkdocs-2.10.1, pyfakefs-5.3.5, ruff-0.2.1
collected 478 items / 477 deselected / 1 selected                                                                                                

distutils/tests/test_unixccompiler.py F                                                                                                    [100%]

==================================================================== FAILURES ====================================================================
_________________________________________ TestUnixCCompiler.test_cc_overrides_ldshared_for_cxx_correctly _________________________________________

self = <distutils.tests.test_unixccompiler.TestUnixCCompiler object at 0x1031f8b50>

    @pytest.mark.skipif('platform.system == "Windows"')
    def test_cc_overrides_ldshared_for_cxx_correctly(self):
        """
        Ensure that setting CC env variable also changes default linker
        correctly when building C++ extensions.
    
        pypa/distutils#126
        """
    
        def gcv(v):
            if v == 'LDSHARED':
                return 'gcc-4.2 -bundle -undefined dynamic_lookup '
            elif v == 'LDCXXSHARED':
                return 'g++-4.2 -bundle -undefined dynamic_lookup '
            elif v == 'CXX':
                return 'g++-4.2'
            elif v == 'CC':
                return 'gcc-4.2'
            return ''
    
        def gcvs(*args, _orig=sysconfig.get_config_vars):
            if args:
                return list(map(sysconfig.get_config_var, args))
            return _orig()
    
        sysconfig.get_config_var = gcv
        sysconfig.get_config_vars = gcvs
        with mock.patch.object(
            self.cc, 'spawn', return_value=None
        ) as mock_spawn, mock.patch.object(
            self.cc, '_need_link', return_value=True
        ), mock.patch.object(
            self.cc, 'mkpath', return_value=None
        ), EnvironmentVarGuard() as env:
            env['CC'] = 'ccache my_cc'
            env['CXX'] = 'my_cxx'
            del env['LDSHARED']
>           sysconfig.customize_compiler(self.cc)

distutils/tests/test_unixccompiler.py:275: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

compiler = <distutils.tests.test_unixccompiler.compiler_wrapper.<locals>.CompilerWrapper object at 0x1031f8f50>

    def customize_compiler(compiler):  # noqa: C901
        """Do any platform-specific customization of a CCompiler instance.
    
        Mainly needed on Unix, so we can plug in the information that
        varies across Unices and is stored in Python's Makefile.
        """
        if compiler.compiler_type == "unix":
            if sys.platform == "darwin":
                # Perform first-time customization of compiler-related
                # config vars on OS X now that we know we need a compiler.
                # This is primarily to support Pythons from binary
                # installers.  The kind and paths to build tools on
                # the user system may vary significantly from the system
                # that Python itself was built on.  Also the user OS
                # version and build tools may not support the same set
                # of CPU architectures for universal builds.
                global _config_vars
                # Use get_config_var() to ensure _config_vars is initialized.
                if not get_config_var('CUSTOMIZED_OSX_COMPILER'):
                    import _osx_support
    
                    _osx_support.customize_compiler(_config_vars)
>                   _config_vars['CUSTOMIZED_OSX_COMPILER'] = 'True'
E                   TypeError: 'NoneType' object does not support item assignment

distutils/sysconfig.py:291: TypeError
============================================================ short test summary info =============================================================
FAILED distutils/tests/test_unixccompiler.py::TestUnixCCompiler::test_cc_overrides_ldshared_for_cxx_correctly - TypeError: 'NoneType' object does not support item assignment
======================================================= 1 failed, 477 deselected in 0.16s ========================================================
py311: exit 1 (0.30 seconds) /Users/jaraco/code/pypa/distutils> pytest -p no:cov -k test_cc_overrides_ldshared_for_cxx_correctly pid=70507
.pkg-cpython311: _exit> python '/Users/jaraco/Library/Application Support/pipx/venvs/tox/lib/python3.12/site-packages/pyproject_api/_backend.py' True setuptools.build_meta
  py311: FAIL code 1 (0.86=setup[0.56]+cmd[0.30] seconds)
  evaluation failed :( (0.90 seconds)

Probably we should figure out why that test is reliant on other tests and fix that too.

jaraco avatar Mar 02 '24 03:03 jaraco

At main, that test runs fine in isolation.

 distutils main @ tox -e py311 -q -- -q -p no:cov -k test_cc_overrides_ldshared_for_cxx_correctly
.                                                                                                                                          [100%]
1 passed, 477 deselected in 0.18s
  py311: OK (0.98 seconds)
  congratulations :) (1.02 seconds)

jaraco avatar Mar 02 '24 14:03 jaraco

The issue seems to be surfaced by this change, where the fallback value is now '', which allows the darwin-specific behavior to be reached:

https://github.com/pypa/distutils/blob/e651e536ef1fbafbe48856815602127df0371835/distutils/sysconfig.py#L285-L291

That code attempts to mutate a global variable, which, if it wasn't initialized by another test, will lead to the error.

I believe the proposed diff is safer and more correct. We'll want to fix that undesirable entanglement.

jaraco avatar Mar 02 '24 14:03 jaraco

After fixing #231 and rebasing onto those changes, I'm now able to elicit the intrinsic error by invoking the test selectively:

 distutils feature/cpp-support @ tox -qq -- -q -p no:cov -k test_cc_overrides_ldshared_for_cxx_correctly
F                                                                                                                                          [100%]
==================================================================== FAILURES ====================================================================
_________________________________________ TestUnixCCompiler.test_cc_overrides_ldshared_for_cxx_correctly _________________________________________

self = <distutils.tests.test_unixccompiler.TestUnixCCompiler object at 0x1055eadb0>

    @pytest.mark.skipif('platform.system == "Windows"')
    @pytest.mark.usefixtures('disable_macos_customization')
    def test_cc_overrides_ldshared_for_cxx_correctly(self):
        """
        Ensure that setting CC env variable also changes default linker
        correctly when building C++ extensions.
    
        pypa/distutils#126
        """
    
        def gcv(v):
            if v == 'LDSHARED':
                return 'gcc-4.2 -bundle -undefined dynamic_lookup '
            elif v == 'LDCXXSHARED':
                return 'g++-4.2 -bundle -undefined dynamic_lookup '
            elif v == 'CXX':
                return 'g++-4.2'
            elif v == 'CC':
                return 'gcc-4.2'
            return ''
    
        def gcvs(*args, _orig=sysconfig.get_config_vars):
            if args:
                return list(map(sysconfig.get_config_var, args))
            return _orig()
    
        sysconfig.get_config_var = gcv
        sysconfig.get_config_vars = gcvs
        with mock.patch.object(
            self.cc, 'spawn', return_value=None
        ) as mock_spawn, mock.patch.object(
            self.cc, '_need_link', return_value=True
        ), mock.patch.object(
            self.cc, 'mkpath', return_value=None
        ), EnvironmentVarGuard() as env:
            env['CC'] = 'ccache my_cc'
            env['CXX'] = 'my_cxx'
            del env['LDSHARED']
            sysconfig.customize_compiler(self.cc)
            assert self.cc.linker_so[0:2] == ['ccache', 'my_cc']
>           self.cc.link(None, [], 'a.out', target_lang='c++')

distutils/tests/test_unixccompiler.py:290: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
distutils/unixccompiler.py:271: in link
    env, linker_ne = _split_env(linker)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

cmd = []

    def _split_env(cmd):
        """
        For macOS, split command into 'env' portion (if any)
        and the rest of the linker command.
    
        >>> _split_env(['a', 'b', 'c'])
        ([], ['a', 'b', 'c'])
        >>> _split_env(['/usr/bin/env', 'A=3', 'gcc'])
        (['/usr/bin/env', 'A=3'], ['gcc'])
        """
        pivot = 0
>       if os.path.basename(cmd[0]) == "env":
E       IndexError: list index out of range

distutils/unixccompiler.py:56: IndexError
============================================================ short test summary info =============================================================
FAILED distutils/tests/test_unixccompiler.py::TestUnixCCompiler::test_cc_overrides_ldshared_for_cxx_correctly - IndexError: list index out of range
1 failed, 477 deselected in 0.18s
py: exit 1 (0.32 seconds) /Users/jaraco/code/pypa/distutils> pytest -q -p no:cov -k test_cc_overrides_ldshared_for_cxx_correctly pid=79617

jaraco avatar Mar 02 '24 15:03 jaraco

The issue seems to be that self.linker_so_cxx is not set ~~on macOS~~ here.

> /Users/jaraco/code/pypa/distutils/distutils/unixccompiler.py(56)_split_env()
-> if os.path.basename(cmd[0]) == "env":
(Pdb) u
> /Users/jaraco/code/pypa/distutils/distutils/unixccompiler.py(271)link()
-> env, linker_ne = _split_env(linker)
(Pdb) linker
[]
(Pdb) self.linker_exe
['ccache', 'my_cc']
(Pdb) building_exe
False
(Pdb) self.linker_so_cxx
[]

~~That doesn't explain why tests are failing on Linux, though.~~ The error is the same on Linux.

jaraco avatar Mar 02 '24 15:03 jaraco

@sciyoshi Since the issue is in the proposed diff, can you investigate?

jaraco avatar Mar 02 '24 15:03 jaraco

@jaraco we're running into a related issue. attempting a fork of this but would love to see it merged into main.

BwL1289 avatar Aug 01 '24 16:08 BwL1289

preliminary tests show it works. would prefer not to maintain a forked version for too long if possible.

BwL1289 avatar Aug 01 '24 16:08 BwL1289

Any thougths on the remaining failing test? I'll try to take a look but won't have much time.

sciyoshi avatar Aug 02 '24 01:08 sciyoshi

The issue seems to be that self.linker_so_cxx is not set ~on macOS~ here.

It looks like in test_cc_overrides_ldshared_for_cxx_correctly, when sysconfig.customize_compiler is called, it causes self.cc.linker_so_cxx to become [] so later when self.cc.link is called, the linker isn't valid and crashes when _split_env is expecting a non-empty list of command parameters.

I need someone to figure out what this test was asserting before and what it needs to be asserting now, and is it catching a legitimate bug, or does the test need to be changed to accommodate the new behavior?

jaraco avatar Aug 02 '24 01:08 jaraco

I do see an asymmetry in these lines:

https://github.com/pypa/distutils/blob/95088c5886327cca5c01426cc9ea0d1063677f26/distutils/sysconfig.py#L301-L322

There, I see ldshared is initialized using get_config_vars('LDSHARED'), but ldcxxshared is initialized to "". Why the discrepancy?

jaraco avatar Aug 02 '24 01:08 jaraco

It looks like making the behavior symmetrical allows the tests to pass:

diff --git a/distutils/sysconfig.py b/distutils/sysconfig.py
index 31bdbec1b..847a26ba4 100644
--- a/distutils/sysconfig.py
+++ b/distutils/sysconfig.py
@@ -304,6 +304,7 @@ def customize_compiler(compiler):  # noqa: C901
             cflags,
             ccshared,
             ldshared,
+            ldcxxshared,
             shlib_suffix,
             ar,
             ar_flags,
@@ -313,13 +314,13 @@ def customize_compiler(compiler):  # noqa: C901
             'CFLAGS',
             'CCSHARED',
             'LDSHARED',
+            'LDCXXSHARED',
             'SHLIB_SUFFIX',
             'AR',
             'ARFLAGS',
         )
 
         cxxflags = cflags
-        ldcxxshared = ""
 
         if 'CC' in os.environ:
             newcc = os.environ['CC']

I'm unsure if that's the correct implementation, but since tests are passing, it feels more correct and at least makes the PR acceptable.

jaraco avatar Aug 02 '24 01:08 jaraco

Seems that's what the upstream patch does as well. This was probably a mistake on my end when I was updating it. Nice catch!

sciyoshi avatar Aug 02 '24 01:08 sciyoshi

Actually, I think the new error in test_customize_compiler is what I was attempting to fix by setting it to "".

sciyoshi avatar Aug 02 '24 01:08 sciyoshi

Actually, I think the new error in test_customize_compiler is what I was attempting to fix by setting it to "".

In PyPy only, it seems. Seems to be that PyPy has a different definition for ~~LIBCXXSHARED~~ LDCXXSHARED than other Pythons. We'll probably need to root cause why that is.

jaraco avatar Aug 02 '24 01:08 jaraco

@jaraco I may be wrong, but it looks like PyPy doesn't preserve LDCXXSHARED at all in its configuration.

CPython does that explicitely here: https://github.com/python/cpython/blob/5a7f7c48644baf82988f30bcb43e03dcfceb75dd/configure.ac#L3395.

Screenshot 2024-08-02 at 10 40 41

gorloffslava avatar Aug 02 '24 05:08 gorloffslava

@jaraco I also checked PyPy, and I don't see any reference to LIBCXXSHARED in the repo.

BwL1289 avatar Aug 02 '24 13:08 BwL1289

My first instinct was to replace None with '' for ldcxxshared, but that also seems improper, as it still invokes compiler.set_executables(ldcxxshared=) for either the empty string or just the parameters with no executable, neither of which is valid.

My next instinct is not to set_executables for ldcxxshared when get_config_var('LDCXXSHARED') is None, but that requires touching several different places in the already messy customize_compiler function. This approach would, however, leave Compiler.linker_so_cxx set to the default. I'd really like to avoid special-casing this one variable.

My next thought was to just skip this test on PyPy, but that probably will cause compilers to break on PyPy on Unix.

Another thought was to inject LDCXXSHARED when it's missing, similar to how is done for add_ext_suffix_39. The problem is, what value to present? Again, an empty string is wrong. And the default value isn't a string at all but a list.

@mattip Do you have any insight into why PyPy doesn't have LDCXXSHARED set in sysconfig? Will PyPy users ever expect to be taking advantage of compiling C++ shared libraries under PyPy?

jaraco avatar Aug 02 '24 14:08 jaraco

In fcef963, I found a satisfactory workaround - don't attempt to add flags if the value is None. It first required a refactor in 3e4b457.

With this change, Compiler.set_executable(linker_so_cxx=None) will be called, which should trigger failures later on PyPy if something tries to compile C++. I believe that's an acceptable behavior... and could be corrected by PyPy exporting LDCXXSHARED.

jaraco avatar Aug 02 '24 14:08 jaraco

The only remaining failure is in diffcov, which is ignorable, since there's no coverage aggregation.

jaraco avatar Aug 02 '24 14:08 jaraco

why PyPy doesn't have LDCXXSHARED set in sysconfig?

The values there have been added on an as-needed basis, in response to issues. PyPy does not use a Makefile or config.

On CPython windows it is not defined.

I'm not sure how PyPY would create that value, it appears to want to invoke a compiler. How does CPython know what compiler is available on the host platform? What happens if clang is installed instead of gcc?

mattip avatar Aug 02 '24 14:08 mattip

Thank you @jaraco! Really appreciate all of the hard work.

BwL1289 avatar Aug 02 '24 14:08 BwL1289

On CPython windows it is not defined.

This particular code path is only for Unix, so that's why the discrepancy hasn't been an issue.

I'm not sure how PyPY would create that value, it appears to want to invoke a compiler. How does CPython know what compiler is available on the host platform? What happens if clang is installed instead of gcc?

It looks like it's built from CXX. On my macOS Python 3.12 as compiled by homebrew, here's what I see:

 🐚 py -3.12 -c "import sysconfig; print(sysconfig.get_config_var('LDCXXSHARED'))"
clang++ -bundle -undefined dynamic_lookup

jaraco avatar Aug 02 '24 14:08 jaraco

We don't use c++ in PyPy, so the best we could do would be to copy that value (and the one for linux) into the _sysconfigdata.py we ship with PyPy, assuming that is appropriate. Maybe distutils could also provide a default value per-platform, in a similar way.

mattip avatar Aug 02 '24 15:08 mattip

Here are the hard-coded PyPy values https://github.com/pypy/pypy/blob/py3.9/lib_pypy/_sysconfigdata.py

mattip avatar Aug 02 '24 15:08 mattip