setuptools icon indicating copy to clipboard operation
setuptools copied to clipboard

Remove dead-code from setup.py

Open pelson opened this issue 1 year ago • 5 comments

Summary of changes

Removes the dead code in setup.py:

  • the definition of package data has no effect (since in pyproject.toml we declare include-package-data=true (from https://github.com/pypa/setuptools/pull/4479)
  • the lingering pypi_link function 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)

pelson avatar Sep 25 '24 10:09 pelson

  • the definition of package data has no effect (since in pyproject.toml we declare include-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.

abravalheri avatar Sep 25 '24 10:09 abravalheri

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).

abravalheri avatar Sep 25 '24 10:09 abravalheri

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.

pelson avatar Sep 25 '24 11:09 pelson

I'm sorry to say that I forgot to install pre-commit, and the tests failed on lint. Have now installed, and re-pushed.

pelson avatar Sep 25 '24 11:09 pelson

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.

abravalheri avatar Sep 25 '24 16:09 abravalheri

Let's proceed with this change.

jaraco avatar May 31 '25 23:05 jaraco