Skip to content
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

Remove dead-code from setup.py #4659

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pelson
Copy link
Contributor

@pelson pelson commented Sep 25, 2024

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 Install all the files #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)

  • Changes have tests
  • News fragment added in [newsfragments/].
    (See [documentation][PR docs] for details)

@abravalheri
Copy link
Contributor

  • 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
Copy link
Contributor

abravalheri commented Sep 25, 2024

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

@pelson
Copy link
Contributor Author

pelson commented Sep 25, 2024

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 (

global-include LICEN[CS]E* COPYING* NOTICE* AUTHORS*
). 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
Copy link
Contributor Author

pelson commented Sep 25, 2024

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

@abravalheri
Copy link
Contributor

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.

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.

2 participants