KEMBAR78
Distutils C++ support by sciyoshi · Pull Request #221 · pypa/distutils · GitHub
Skip to content

Conversation

@sciyoshi
Copy link
Contributor

(Reopened from pypa/setuptools#4157) Note: I am not the author of this patch. I am simply proposing that this be upstreamed from Nix (https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/python-modules/setuptools/setuptools-distutils-C%2B%2B.patch) because it solves issues that other users of setuptools have run into.

Closes pypa/setuptools#1732

Copy link
Member

@jaraco jaraco left a comment

Choose a reason for hiding this comment

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

This change seems reasonable.

I have two concerns.

First, I'm a little concerned about the changes to distutils.sysconfig, since that module is essentially deprecated in favor of the stdlib sysconfig module. Distutils has a vested interest in migrating all of its reliance on distutils.sysconfig to sysconfig (or elsewhere). Is there a world in which the changes to distutils.sysconfig are made unnecessary by relying on sysconfig upstream? Is the C++ support for sysconfig available upstream or does this change become a new impediment for adopting sysconfig upstream?

Second, this change is interspersed across dozens of lines in three modules. The separation of concerns is weak and the naming actually betrays the capability (CygwinCCompiler and UnixCCompiler now are dual-mode compilers). My instinct is that the C++ and C compilers should in fact be separate classes, rather than one class that alters its behavior based on the source. This concern is complicated by the fact that C++ is largely (historically?) a superset of C, so a C++ compiler is a C compiler. Since you're not the author, I'm unsure what to request here. On one hand, I'm inclined to just accept it as the best humanity has to offer for now and defer refactoring to a future poor soul. On the other hand, I'm inclined to push back and say that this separation should be made explicit before being adopted by the ecosystem. Can you make a compelling argument for adopting it as-is?

@sciyoshi
Copy link
Contributor Author

Thanks @jaraco for your comment.

To your first point: the changes to distutils.sysconfig are limited to the customize_compiler function, which is not available in the stdlib sysconfig, so although I'm admittedly not very familiar with the internals of distutils.sysconfig and sysconfig, I imagine that this change wouldn't have any impact on the effort involved in migrating from one to the other.

To your second point: I believe this patch was originally authored and published as part of a now 19-year-old bug filed against CPython, and the comments there (and the linked setuptools issue) might provide some insight into the rationale behind it and the issues that people are currently running into. The final suggestion there is to reopen against setuptools/distutils, and I'm not sure if there's been any followup since. The issue I have specifically been running into is probably niche and limited to Darwin platforms (I could do a more detailed writeup if that's helpful), but it seems that there's other scenarios where C++ support is wanted.

That said, while I've looked at the patch and it seems straightforward enough, I can't directly endorse it, and there's certainly improvements that could be made like you suggested. The primary argument I would make in favor of still including it as-is is simply 1) that it fixes a current pain point, and 2) that it is used by the entire nix ecosystem without any apparent problems.

@jaraco jaraco merged commit d70e4da into pypa:main Feb 13, 2024
@sciyoshi sciyoshi deleted the distutils-cpp branch February 13, 2024 16:15
@jaraco
Copy link
Member

jaraco commented Feb 13, 2024

I swear the tests were passing with this change, but after merging it, tests are failing (https://github.com/pypa/distutils/actions/runs/7889130804/job/21528245684). Two of those failures were just style nitpicks, fixed in 0e57a23. The other two are:

>       assert comp.exes['compiler'] == 'env_cc --sc-cflags --env-cflags --env-cppflags'
E       AssertionError: assert 'env_cc --env...-env-cppflags' == 'env_cc --sc-...-env-cppflags'
E         
E         - env_cc --sc-cflags --env-cflags --env-cppflags
E         ?       ------------
E         + env_cc --env-cflags --env-cppflags

distutils/tests/test_sysconfig.py:132: AssertionError

and

>           assert call_args[:4] == expected
E           AssertionError: assert ['my_cxx', 'g...'-o', 'a.out'] == ['my_cxx', '-...namic_lookup']
E             
E             At index 1 diff: 'gcc-4.2' != '-bundle'
E             Use -v to get more diff

distutils/tests/test_unixccompiler.py:276: AssertionError

@jaraco
Copy link
Member

jaraco commented Feb 13, 2024

I've force-pushed main without this merge and re-submitted the PR as #228.

@jaraco jaraco mentioned this pull request Feb 13, 2024
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.

setuptools using CC for compiling C++ files

2 participants