Adds a basic MANIFEST.in file #2077
Conversation
jakirkham
force-pushed
the
add_manifest
branch
from
2204019
to
053526c
Compare
| @@ -0,0 +1,10 @@ | |||
| include README.rst | |||
| include LICENSE.txt | |||
| include CHANGELOG.md | |||
There was a problem hiding this comment.
Make sure the license file is definitely included. Include the README and CHANGELOG as well for good measure.
MANIFEST.in
Outdated
|
|
||
| recursive-include tests * | ||
| recursive-exclude * __pycache__ | ||
| recursive-exclude * *.py[co] |
There was a problem hiding this comment.
Ensure end users can run the tests.
MANIFEST.in
Outdated
| recursive-exclude * *.py[co] | ||
|
|
||
| recursive-include docs Makefile make.bat *.conf *.css *.html *.ico *.ipynb *.js *.json *.npy *.png *.py *.rst *.svg | ||
| recursive-include examples *.csv *.gz *.ipynb *.md *.npz *.png |
There was a problem hiding this comment.
Basically searched and included all extensions from the docs and the examples. Not sure if we want all of them or even if we want the docs and examples packaged. Feel free to let me know either way.
There was a problem hiding this comment.
Also as there are some data files listed here, it is good to be careful with packaging these in case they are big. Am happy to keep or remove these as deemed appropriate.
There was a problem hiding this comment.
We've deliberately kept the size of these small, so this should be fine.
There was a problem hiding this comment.
As long as the stuff in the doc submodules isn't pulled in, everything should be fine (i.e the reference/test data)/
There was a problem hiding this comment.
So the only submodules I see are doc/builder and doc/nbpublisher. Are we trying to exclude those?
Also I see doc/test_data/ as an ordinary directory. Should that be removed?
There was a problem hiding this comment.
So the only submodules I see are doc/builder and doc/nbpublisher. Are we trying to exclude those?
Yes please.
Also I see doc/test_data/ as an ordinary directory. Should that be removed?
It should also be excluded. It probably doesn't have to exist (i.e could be created dynamically) but it is used - when you run notebook tests locally it fills up.
There was a problem hiding this comment.
Have added lines to prune the two submodules. These seem to work as intended.
Was unclear on doc/test_data. Should we keep it just in case something uses it and forgets to create it or is that a non-issue?
There was a problem hiding this comment.
The presence of the directory doesn't really matter, as you say, it might be prudent to keep it. The important thing is that nothing from inside that directory should be included.
|
Thanks for doing this! Could you test one thing for me though. After building with sdist, could you try installing it in a clean environment and see if |
|
Seem to be running into a little snafu. $ python setup.py sdist
Traceback (most recent call last):
File "setup.py", line 164, in <module>
holoviews.__version__.verify(setup_args['version'])
File "/zopt/conda2/envs/test/lib/python3.6/site-packages/param/version.py", line 286, in verify
raise Exception("Supplied string version does not match current version.")
Exception: Supplied string version does not match current versionIs there something else I need to be doing here? Edit: Maybe I need to add a tag locally to fix this? |
|
This is a check we use when making PyPI releases to make sure the version as set with git tag matches the string version supplied in |
|
I think I got around it. I made a commit bumping the version in |
That's the right thing to do! 👍 |
|
Cool. Used |
jakirkham
force-pushed
the
add_manifest
branch
from
053526c
to
28eb74b
Compare
|
Is it full of notebooks? If so that is success! Otherwise this PR hasn't fixed an issue I was hoping it would fix - which isn't to say this PR shouldn't be merged. |
|
Yep. Just as an example... $ ls holoviews-examples/gallery/demos/matplotlib/
area_chart.ipynb histogram_example.ipynb measles_example.ipynb stocks_example.ipynb
autompg_histogram.ipynb iris_density_grid.ipynb network_graph.ipynb surface_3d.ipynb
bachelors_degrees_by_gender.ipynb iris_example.ipynb polar_scatter_demo.ipynb texas_choropleth_example.ipynb
bars_economic.ipynb iris_splom_example.ipynb quiver_demo.ipynb topographic_hillshading.ipynb
boxplot_chart.ipynb legend_example.ipynb scatter_economic.ipynb trisurf3d_demo.ipynb
dragon_curve.ipynb lorenz_attractor_example.ipynb square_limit.ipynb us_unemployment.ipynb
dropdown_economic.ipynb mandelbrot_section.ipynb step_chart.ipynb verhulst_mandelbrot.ipynb |
|
Fantastic! You've solved a packaging issue that was giving me a headache. I'm happy to see this PR merged when the tests pass - I'll let @philippjfr do the final review/merge. |
|
In particular this would solve #1791. I'll checkout this branch and try it out myself to double check! |
jakirkham
force-pushed
the
add_manifest
branch
from
28eb74b
to
8ce9ba3
Compare
MANIFEST.in
Outdated
| recursive-include doc Makefile make.bat *.conf *.css *.html *.ico *.ipynb *.js *.json *.npy *.png *.py *.rst *.svg | ||
| prune doc/builder | ||
| prune doc/nbpublisher | ||
| recursive-exclude doc/test_data * |
There was a problem hiding this comment.
So I couldn't figure out how to keep this directory empty. Seems it needs at least one file to stick around. Seems to only have a README in it in master. Maybe we can exclude all data or just include the README?
Currently the code here deletes that directory entirely.
There was a problem hiding this comment.
Sorry I couldn't help myself. Kept playing with it locally. Now it just keeps the README. Can revert back to this change if you prefer though.
There was a problem hiding this comment.
I don't think that's an issue. The user can't run the tests that would use this directory without the submodules anyway and we'll soon be replacing this machinery entirely.
There was a problem hiding this comment.
Alright feel free to override whatever sits on this line. I have no strong opinions. 😄
Make sure to include the README, LICENSE, and CHANGELOG. Also make sure to package the tests. Further ensure the docs and examples are included as well. This is useful for deploying sdists and other products and ensuring they are feature complete.
jakirkham
force-pushed
the
add_manifest
branch
from
8ce9ba3
to
aa4ed04
Compare
|
Anyway, I just double checked for myself and I think #1791 is fixed! I'm always suspicious of packaging but I really hope the the following command mirrors what would happen if you installed from PyPI: Unfortunately, I find packaging issues often turns out to be black magic where even simple assumptions like this don't hold the way you would like... |
|
Yep, that should be a good test. My experiences with Took the liberty of adding the fixes line in the OP. Please let me know if there is anything else. 😄 |
Nope. Thanks again for this, it is really helpful! I'm happy to merge when the tests pass. Do you agree @philippjfr ? |
|
Absolutely, thanks so much for solving this headache for us! |
|
Tests passed so I'll go ahead and merge. Thanks again! |
|
Absolutely! Glad to help. This should hopefully also make PyPI sdists a viable source in the conda-forge recipe, which should make packaging on that front a little easier. Should that be a direction that you would be willing to consider. |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Fixes #1791
Make sure to include the README, LICENSE, and CHANGELOG. Also make sure to package the tests. Further ensure the docs and examples are included as well. This is useful for deploying sdists and other products and ensuring they are feature complete.