-
Notifications
You must be signed in to change notification settings - Fork 80
Distutils C++ support #221
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
Conversation
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
There was a problem hiding this 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?
|
Thanks @jaraco for your comment. To your first point: the changes to 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. |
|
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: and |
|
I've force-pushed main without this merge and re-submitted the PR as #228. |
(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