setuptools
setuptools copied to clipboard
Remove dead-code from setup.py
Summary of changes
Removes the dead code in setup.py:
- the definition of package data has no effect (since in
pyproject.tomlwe declareinclude-package-data=true(from https://github.com/pypa/setuptools/pull/4479) - the lingering
pypi_linkfunction has no effect
I hesitated to also remove the os.chdir functionality, but considered that maybe this is important for something else (although unusual).
Pull Request Checklist
(not applicable)
- [x] Changes have tests
- [x] News fragment added in [
newsfragments/]. (See [documentation][PR docs] for details)
- the definition of package data has no effect (since in
pyproject.tomlwe declareinclude-package-data=true(from Install all the files #4479)
This theoretically may have some effect (I am not sure). include-package-data=true will only add stuff that is already listed by MANIFEST.in. package-data will add files regardless of MANIFEST.in. This is the truth table: https://github.com/abravalheri/experiment-setuptools-package-data?tab=readme-ov-file#results-for-setuptools6060. Now I have not checked if in this particular case the configuration is redundant, but in theory the 2 configurations may exist without overlap.
It is probably good to add some sanity checks/regression tests in the style of https://github.com/pypa/setuptools/blob/v75.1.0/setuptools/tests/test_sdist.py#L969-L975 (but instead of sdist, checking the wheel) for those removals.
In particular we want to make sure that all the licenses of the vendored code is included (because that is kind of a legal obligation).
Thanks for the feedback. Note that there are already tests for the cli.exe existence (from https://github.com/pypa/setuptools/pull/4479/files#diff-b033a4738946c4c459abca1cbab3460d9aea8060f4394bf3376b573662a84eb3R317).
I didn't yet check regarding testing LICENSE etc., but note that it is declared in the MANIFEST (https://github.com/pypa/setuptools/blob/3106af0512fe67464a8b5e7524c07fddf7717660/MANIFEST.in#L13). Adding a wheel test for the license inclusion amounts to testing that MANIFEST is declared correctly, and that setuptools is doing the right thing. I can see a little value in those things, so if you want me to proceed would be fine adding the test.
I'm sorry to say that I forgot to install pre-commit, and the tests failed on lint. Have now installed, and re-pushed.
Adding a wheel test for the license inclusion amounts to testing that MANIFEST is declared correctly, and that setuptools is doing the right thing. I can see a little value in those things, so if you want me to proceed would be fine adding the test.
I think it would be interesting to add this, as they would work as regression tests if in the future we change the configuration.
Let's proceed with this change.