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

tests package is not included in the source tarball on PyPi #2635

Closed
moorepants opened this issue Oct 13, 2019 · 18 comments
Closed

tests package is not included in the source tarball on PyPi #2635

moorepants opened this issue Oct 13, 2019 · 18 comments
Assignees
Milestone

Comments

@moorepants
Copy link
Contributor

The setup.py file has:

packages=['pelican', 'pelican.tools'],

thus the tests package is not available if you install from anything but a github clone. It would be helpful if the tests package was included, as is common with many other python distributions. Maybe use find_packages() on this line instead.

@Lucas-C
Copy link
Contributor

Lucas-C commented Oct 13, 2019

I made the same analysis as yours recently while working on pelican-plugins .travis.yml.

The idea to include it by using find_packages() sounds good to me.

Could you submit a PR ?

@moorepants
Copy link
Contributor Author

There is this note in the setup.py:

        # we manually collect the package data, as opposed to using,
        # include_package_data=True because we don't want the tests to be
        # included automatically as package data (MANIFEST.in is too greedy)

Seems to indicate that the tests are desired to be packaged?

@moorepants
Copy link
Contributor Author

The tests folder does have a lot of pngs and other data files. It is 2.5 MB in size. Maybe that's why it wasn't included?

@avaris
Copy link
Member

avaris commented Oct 13, 2019

see #1618 and #1778

@moorepants
Copy link
Contributor Author

I can see from that issue that some said "can you stop including tests in the sdist?" and someone said "sure". But it isn't completely clear why you would do this, or why they did this. It seems to me that the vast majority of python packages ship the tests in the source distribution. So that you can simply install the package and then run the tests to make sure it works properly on your system. For downstream packagers and plugin/theme makers it is very helpful to have the tests in the source dist so that you don't have to install pelican from the git repo. I raised this issue because many plugins use functions/classes available in the tests package of pelican, but you can't run the test with a pip/conda/apt/etc install Pelican. I'm not sure what the reason would be to not include the tests package, expected if you want to shave off some of the size of the resulting tarball.

@moorepants
Copy link
Contributor Author

Digging down in the issues some more shows that people were getting unicode errors installing from pip on some OSs and the bandaid was to not include the tests/ in the source distribution. Seems like a proper fix would be to address the unicode errors directly (and even install on CI on the three main OSes).

@avaris
Copy link
Member

avaris commented Oct 13, 2019

I linked those for some background of "why". At the time, "fix" for those unicode errors would entail fixing pip, not pelican. At least not without dropping some unittests we have, which is rather undesired. Maybe pip already fixed the underlying issue.

@moorepants
Copy link
Contributor Author

Yes, pip/setuptools have 4 years of development, so maybe it isn't an issue. Do you all have CI setup for testing installs on different OSes?

@avaris
Copy link
Member

avaris commented Oct 13, 2019

Current travis config runs tests on ubuntu. PRs extending it with other OSs would be welcome :).

@avaris
Copy link
Member

avaris commented Oct 13, 2019

FWIW, I still get UnicodeDecodeErrors on windows with pip==19.2.3

@avaris
Copy link
Member

avaris commented Oct 13, 2019

Related: pypa/pip#7138 and pypa/setuptools#1399

So, still an issue.

@stale
Copy link

stale bot commented Dec 12, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your participation and understanding.

@stale stale bot added the stale Marked for closure due to inactivity label Dec 12, 2019
@moorepants
Copy link
Contributor Author

It isn't stale, it just isn't fixed.

@justinmayer
Copy link
Member

Update on this topic... As far as I can tell, building via Poetry and then installing both the sdist and wheel on Windows works fine, even with the tests included: https://github.com/getpelican/pelican/runs/660597734?check_suite_focus=true#step:4:126 🎉

I did manage to trigger a Unicode-related error when trying to Pip-uninstall, which went away as soon as I went back to excluding the tests: https://github.com/getpelican/pelican/runs/660612810?check_suite_focus=true#step:4:150

That said, I don't know if that's a big enough reason to not include the tests in the package.

@justinmayer
Copy link
Member

Update: Potential fix was just merged into Pip a few minutes ago: pypa/pip#8223

@justinmayer
Copy link
Member

justinmayer commented May 19, 2020

Unless I am mistaken, this test run indicates that the above Pip change does indeed fix installing Pelican sdist/wheel on Windows, even when tests are included in the package.

I'm not sure when Pip 20.2 is due for release, but once it has been released, I think we can safely resume including tests in Pelican packages. 🎉🎈🎊

@justinmayer justinmayer added this to the Pelican 4.5 milestone Jul 23, 2020
@justinmayer
Copy link
Member

justinmayer commented Jul 23, 2020

It appears Pip 20.2 is due for release on Monday, July 27. Once that happens, what should be the appropriate action to take here? Set include_package_data=True in setup.py and remove the manual package_data collection stanza? Now that we are building Pelican via Poetry, it probably doesn't matter how package_data is configured in setup.py.

@justinmayer
Copy link
Member

Now that Pip 20.2 has been released, I confirmed that building, installing, and uninstalling current Pelican master on Windows — from both sdist and wheel — appears to be successful. Even the new (off-by-default) dependency resolver in Pip 20.2 works just fine, which is very cool.

In short, I think we are clear to include the tests in the next distribution to PyPI. 🎉

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.

4 participants