add license, docs, authors to manifest, exclude spa files #580
Conversation
|
LGTM!
The following files from the manifest are in the source:
here's the full contents of the source distribution: |
MANIFEST.in
Outdated
| recursive-include pvlib/data * | ||
| include README.md | ||
|
|
||
| include pvlib/spa_c_files/*.h |
There was a problem hiding this comment.
IMO this should be removed, b/c I don't think there are any header files are in spa_c_files that should be distributed, and this might accidentally cause the NREL SPA C-files to be distributed without their consent.
There was a problem hiding this comment.
I agree. Do we need to replace it with anything else so that the subpackage is properly distributed (minus the NREL files)?
For the record... the only way this would cause the NREL files to be distributed is if the files were manually added to the pvlib/spa_c_files directory before making a release. I usually make a fresh clone of pvlib/pvlib-python before making a release so that things like this don't happen. It's possible that the files are in a few releases in the 0.2-0.3 era, but I'd be surprised if they are in any of the 0.4-0.5 releases.
MANIFEST.in
Outdated
| include AUTHORS.md | ||
| include LICENSE | ||
| include README.md | ||
|
|
||
| include versioneer.py | ||
| include pvlib/_version.py |
There was a problem hiding this comment.
AFAIK, this line is not necessary, since pvlib is a package, and is already listed in setup.py
There was a problem hiding this comment.
I agree, but I believe this was part of the versioneer setup instructions. Other projects include this line as well. So I recommend we leave it for now.
MANIFEST.in
Outdated
| recursive-exclude * *.py[co] | ||
|
|
||
| recursive-include docs * | ||
| prune docs/tutorials |
There was a problem hiding this comment.
just curious, why prune the Jupyter notebook tutorials? IMO these could be run locally more interactively, than viewing them in nbviewer or GitHub. IMO pvlib should include these in the sdist.
There was a problem hiding this comment.
Yes, better to include them.
|
I think the MANIFEST now properly handles the spa_c_files directory. With the NREL spa.c/h files present in the spa_c_files directory... |
|
Can you also manually exclude `build/` and any `*.so` or `*.pyd` files from `spa_c_files` please? This should be explicitly a pure source distribution.
Sent from Yahoo Mail on Android
On Sun, Sep 16, 2018 at 10:49 AM, Will Holmgren<[email protected]> wrote:
I think the MANIFEST now properly handles the spa_c_files directory. With the NREL spa.c/h files present in the spa_c_files directory...
$ ls -1a pvlib/spa_c_files
.
..
README.md
SPA_NOTICE.md
__init__.py
__init__.pyc
__pycache__
build
cspa_py.pxd
setup.py
spa.c
spa.h
spa_py.cpython-36m-darwin.so
spa_py.pyx
spa_py_example.py
$ python setup.py sdist
$ ls -1a dist/pvlib-0.6.0a2+10.g01e8c3e/pvlib/spa_c_files
.
..
README.md
SPA_NOTICE.md
__init__.py
cspa_py.pxd
setup.py
spa_py.pyx
spa_py_example.py
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Ok I can specifically exclude them but note they are not in the dist.
On Sun, Sep 16, 2018 at 11:04 AM Mark Mikofski <[email protected]>
wrote:
… Can you also manually exclude `build/` and any `*.so` or `*.pyd` files
from `spa_c_files` please? This should be explicitly a pure source
distribution.
Sent from Yahoo Mail on Android
On Sun, Sep 16, 2018 at 10:49 AM, Will ***@***.***>
wrote:
I think the MANIFEST now properly handles the spa_c_files directory. With
the NREL spa.c/h files present in the spa_c_files directory...
$ ls -1a pvlib/spa_c_files
.
..
README.md
SPA_NOTICE.md
__init__.py
__init__.pyc
__pycache__
build
cspa_py.pxd
setup.py
spa.c
spa.h
spa_py.cpython-36m-darwin.so
spa_py.pyx
spa_py_example.py
$ python setup.py sdist
$ ls -1a dist/pvlib-0.6.0a2+10.g01e8c3e/pvlib/spa_c_files
.
..
README.md
SPA_NOTICE.md
__init__.py
cspa_py.pxd
setup.py
spa_py.pyx
spa_py_example.py
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#580 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AELiRwoVtSCJ_Q1WiFysC75omH_X1xcTks5ubpK-gaJpZM4Wqm92>
.
|
|
Oh ok sorry, my bad, either way. I'm not at a computer so I can't test |
|
Reworked the manifest to use graft. The behavior now is to include everything under pvlib/ except for the specifically excluded items. This follows the prevailing attitude online of "better to include too much than not enough." Contents of distributed |
|
I like this a lot, I just tested, and it excludes a lot more cruft than previously ( maybe also |
|
Thanks @mikofski. The I see that I've accidentally been including eggs in some of the previous source distributions. |
|
something weird, dunno if/how you want to handle - I had put spa.h and spa.c in a subfolder of spa_c_files, and since they weren't explicitly excluded, they ended up in the tarball. adding this seems to work: |
no, didn't work for me either, frustrating |
|
Good idea to make a more flexible spa exclusion. I can't find much guidance online for how to exclude the egg. I'd say better to look the other way if it's not easy to exclude. Note that the egg is included in the v0.6.0a2 release that TravisCI pushed to PyPI last week. |
|
SO Q/A say:
None of the workarounds seem to work for me, I agree with your conclusion, AFAICT this folder was meant to be included in any distribution. |
wholmgren
changed the title
|
rabbit hole that egg-info, glad you moved on 🐰 |
Closes #579. @mikofski does this look ok to you? I wasn't sure about
spa_c_filesso I left it alone. Or maybe we can merge this and then you can make further updates in your PR if it's important.