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

Sample_Code, Scripts and Tests are wrongly installed as top-level packages #727

Closed
mgorny opened this issue Apr 10, 2022 · 6 comments · Fixed by #728
Closed

Sample_Code, Scripts and Tests are wrongly installed as top-level packages #727

mgorny opened this issue Apr 10, 2022 · 6 comments · Fixed by #728

Comments

@mgorny
Copy link
Contributor

mgorny commented Apr 10, 2022

The 2.7.2 release includes a regression where additional top-level packages are wrongly installed, e.g.:

/usr/lib/python3.10/site-packages/Sample_Code
/usr/lib/python3.10/site-packages/Scripts
/usr/lib/python3.10/site-packages/Tests

If these files are really supposed to be installed, they should be inside PyPDF2 and not at top level.

@MartinThoma
Copy link
Member

I'm pretty confused by that. This is how the diff between 1.26.0 and 1.27.2 looks like (of the source package):

image

I've added Scripts, Sample_Code, and Tests because I have seen that they were present before. I don't know how they were added before, because they were neither in the Manifest nor there as packages.

Is this currently causing issues?

  • I want to remove Sample_Code soon completely, see: MAINT: Remove Sample_Code #726
  • I don't like the Tests and Resources being added either and I have a hard time imagining that removing them would cause any trouble
  • Scripts ... here I'm not sure how I should continue. Either I add them as a sub-package or I remove it. Is there another option?

@MartinThoma
Copy link
Member

When I add it just to the manifest and not as a package, would that lead to the desired result?

@mgorny
Copy link
Contributor Author

mgorny commented Apr 10, 2022

I've added Scripts, Sample_Code, and Tests because I have seen that they were present before. I don't know how they were added before, because they were neither in the Manifest nor there as packages.

Yes, I suppose it makes sense to include them in sdist but they shouldn't be installed into site-packages. I'm afraid I can't help you figuring out why it worked before, setuptools sdist magic was always totally confusing to me (and I've seen that with my own packages too) but…

When I add it just to the manifest and not as a package, would that lead to the desired result?

…yes, I think adding these directories to MANIFEST.in should achieve the desired result without causing them to be installed.

MartinThoma added a commit that referenced this issue Apr 10, 2022
Only distribute it in the source

Closes #727
MartinThoma added a commit that referenced this issue Apr 10, 2022
Only distribute it in the source

Closes #727
MartinThoma added a commit that referenced this issue Apr 10, 2022
Only distribute it in the source

Closes #727
@MartinThoma
Copy link
Member

I've just tried it out locally. Seems to do what is expected. I will make a release today (but probably not within the next 2 hours ... might be in the evening).

@mgorny
Copy link
Contributor Author

mgorny commented Apr 10, 2022

Ok, thanks!

@mgorny
Copy link
Contributor Author

mgorny commented Apr 10, 2022

I can confirm that 1.27.3 looks fine.

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 a pull request may close this issue.

2 participants