Fix 2849: Problem with custom commands inheriting directly from distutils
#2855
Conversation
According to pypa#2849, some projects, including important data-science packages rely on `distutils` when creating custom commands, instead of extending the ones provided by setuptools. This change should accomodate this use case, while also warning the users to migrate.
abravalheri
changed the title
distutils
|
Thanks in advance, @abravalheri, for tackling this problem! |
|
That is an interesting Windows error: I suppose Windows had some problem starting the processes. Maybe running again solves the problem? |
|
Please merge it as soon as possible, Thanks in advance |
| if hasattr(build_py, 'get_data_files_without_manifest'): | ||
| return build_py.get_data_files_without_manifest() | ||
|
|
||
| log.warn( |
There was a problem hiding this comment.
I'm unsure whether to invoke log.warn here or issue a DeprecationWarning. The latter can be controlled by warnings filters. In any case, I quite like the approach taken here to direct the users away from using distutils, which is deprecated.
There was a problem hiding this comment.
I wasn't sure myself 😅
If you prefer a DeprecationWarning, I can work in a change over the weekend.
changelog.d/2849.change.rst
Outdated
| @@ -0,0 +1,3 @@ | |||
| Add fallback for custom ``build_py`` commands inheriting directly from | |||
There was a problem hiding this comment.
I'd consider this a bugfix for the feature, so I'll rename this file to .misc.
There was a problem hiding this comment.
Looks great. Just one nitpick on the changelog scope. I'll make the change and get this out asap. Brilliant work creating a test. I also agree on resisting integration tests with a large and complex suite like scikit. I'd love to see some integration tests, but I'd like to see them outside this project.
|
Fix going out as v58.5.3. |
|
Thank you very much @jaraco 👏 |
|
Thanks for the fix. As I mentioned elsewhere, the danger of a new version of setuptools inadvertently breaking project's workflow is that then the project will begin pinning setuptools to an earlier version, which means setuptools will never find out that its HEAD no longer works for that project. Perhaps a compromise, assuming not every change can be tested, would be to add tests before a release. |
…cikit-learn#21549)" This reverts commit 953d101. The fix for setuptools#21549 has been introduced in 58.5.3. See: pypa/setuptools#2855 (comment)
…ools#2849)" This reverts commit 1f69351. This should be ok now, as pypa/setuptools#2855 should fix it.
…21549)" (#21560) This reverts commit 953d101. The fix for setuptools#21549 has been introduced in 58.5.3. See: pypa/setuptools#2855 (comment)
|
I agree it would be nice to have integration tests for at least some common, popular use-cases. I'd like for these tests to be separate from the normal development workflow (either deselected by default or run as a separate suite) so that the test suite execution time and CI test time isn't dramatically affected. I'm thinking maybe another GH action between test and release. |
As discussed in pypa#2855, using an actual warning instead of the logger allow users to control what gets displayed via warning filters.
|
I can try to have a look on this, if the idea is to do it as an action before the release. There was a previous attempt from PyPA to have integration tests: https://github.com/pypa/integration-test |
…cikit-learn#21549)" (scikit-learn#21560) This reverts commit 953d101. The fix for setuptools#21549 has been introduced in 58.5.3. See: pypa/setuptools#2855 (comment)
…cikit-learn#21549)" (scikit-learn#21560) This reverts commit 953d101. The fix for setuptools#21549 has been introduced in 58.5.3. See: pypa/setuptools#2855 (comment)
…cikit-learn#21549)" (scikit-learn#21560) This reverts commit 953d101. The fix for setuptools#21549 has been introduced in 58.5.3. See: pypa/setuptools#2855 (comment)
…21549)" (#21560) This reverts commit 953d101. The fix for setuptools#21549 has been introduced in 58.5.3. See: pypa/setuptools#2855 (comment)
…cikit-learn#21549)" (scikit-learn#21560) This reverts commit 953d101. The fix for setuptools#21549 has been introduced in 58.5.3. See: pypa/setuptools#2855 (comment)
Summary of changes
This PR provides a fallback when packages extend
build_pydirectly from distutils instead of setuptools.It is pretty much the same changed implemented in 8a0e63a, but now we have a compelling reason to leave it there.
I added a unit test for that scenario, however other people have suggest trying to build
scipyagainst 'setuptools'.I understand that, it would be a nice integration test, but it is also very resource and time heavy, so I am not sure about how to proceed with that.
Closes #2849
Pull Request Checklist
changelog.d/.(See documentation for details)