Fix loose files during install

[wingkitlee0]

To solve #374

[Updated since first review]
It turns out the solution is much simpler than originally thought!

Now we use setuptools-scm for better file discovery and path management.

Added the following lines to MANIFEST.in to exclude some newly included folders (were originally excluded)

prune docs/
prune .github/
...
exclude .*  # .gitignore etc

There are about 20 extra files in the sdist compared to the current master. They are mostly files in the subfolders of tests/

I have checked with pip-installing ., sdist and wheel. They all work fine:
that is, no stray source files outside site-packages/h3

[CLAassistant]

All committers have signed the CLA.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (d3f6c1d) to head (cdb881b).

Additional details and impacted files

@@            Coverage Diff            @@
##            master      #375   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines          326       325    -1     
=========================================
- Hits           326       325    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ajfriend]

Also, I excluded the following files previously listed in MANIFEST.in:

makefile
pyproject.toml
setup.py
src/h3lib  # probably we do not need the source files of H3 C library

I think the sdist would require h3lib, and probably also setup.py and pyproject.toml.

[ajfriend]

Now, after pip install . and pip uninstall h3, I got

This is great! This wouldn't be blocking, but I wonder if we could write a CI test to verify that these files stay in the right places?

[dfellis]

So it seems by moving the file definition into CMake instead of MANIFEST.in, it no longer includes all of the files into the source tarball (basically all of the files whose definition was moved, including the H3 C source). See https://github.com/uber/h3-py/actions/runs/9279810162/job/25533487042?pr=375 vs https://github.com/uber/h3-py/actions/runs/9149163261/job/25152501237#step:4:74 for comparison.

I'm not a python expert, and since h3-py is still on the legacy setup.py finding authoritative documentation is difficult, but it might be possible to fix this by manually extending the build class similar to how this example extends the install class: https://stackoverflow.com/a/21196195

[wingkitlee0]

I think the sdist would require h3lib, and probably also setup.py and pyproject.toml.

I think sdist is not affected by this. I think CMake was not involved and skbuild / setuptools to just tar the source
For example, I got this by running tar -tzvf h3-4.0.0b5.tar.gz | awk '{print $NF}'

h3-4.0.0b5/
h3-4.0.0b5/CHANGELOG.md
h3-4.0.0b5/CMakeLists.txt
h3-4.0.0b5/LICENSE
h3-4.0.0b5/MANIFEST.in
h3-4.0.0b5/PKG-INFO
h3-4.0.0b5/makefile
h3-4.0.0b5/pyproject.toml
h3-4.0.0b5/readme.md
h3-4.0.0b5/requirements.in
h3-4.0.0b5/setup.cfg
h3-4.0.0b5/setup.py
h3-4.0.0b5/src/
...

And I was able to pip install h3-4.0.0b5.tar.gz

[wingkitlee0]

Now, after pip install . and pip uninstall h3, I got

This is great! This wouldn't be blocking, but I wonder if we could write a CI test to verify that these files stay in the right places?

I think so. I would like to see such a test too.

[wingkitlee0]

So it seems by moving the file definition into CMake instead of MANIFEST.in, it no longer includes all of the files into the source tarball (basically all of the files whose definition was moved, including the H3 C source). See https://github.com/uber/h3-py/actions/runs/9279810162/job/25533487042?pr=375 vs https://github.com/uber/h3-py/actions/runs/9149163261/job/25152501237#step:4:74 for comparison.

I'm not a python expert, and since h3-py is still on the legacy setup.py finding authoritative documentation is difficult, but it might be possible to fix this by manually extending the build class similar to how this example extends the install class: https://stackoverflow.com/a/21196195

Thanks for pointing this out. I will take a look of how make_sdist is executed..

[ajfriend]

@wingkitlee0 what are you trying to figure out with these workflow changes?

[wingkitlee0]

@ajfriend nevermind about it. I will revert those changes.
I was trying to trigger the make_sdist step from a draft PR (to avoid notifications to everyone). It turns out it requires maintainers' approval... 😅

Anyway, I found this act tool to run the github action locally: https://github.com/nektos/act
and was able to reproduce the failed step!
I am good for now.

[wingkitlee0]

@ajfriend Fixed everything now...with a much simpler solution. See the new description.

As I touched and reverted the workflow, you may need to re-approve the pipeline. Thanks!

[ajfriend]

Awesome. Great work!

One question: Is there an easy way to toggle if the test files are included or not?

[wingkitlee0]

One question: Is there an easy way to toggle if the test files are included or not?

It should be just adding this line to MANIFEST.in:

prune tests/

[ajfriend]

Overall, this looks great. Thanks again!

The only soft blocker is that what setuptools-scm is doing under the hood is still a bit mysterious to me, so I'd like to take some time to make sure I understand that before landing. I'll try to figure that out and land ASAP.

[isaacbrodsky]

I'm not sure I totally understand what's going on in this change, but as I got it, adding this package changes something about how the build works. If that's the case, could a comment be added here that explains what we're relying on this for? I feel like otherwise it could be accidentally removed without understanding the context of this change.

[ajfriend]

@wingkitlee0 I tested locally with

pip install git+https://github.com/wingkitlee0/h3-py.git@fix-manifest-in

But when I pip uninstall h3, I think the problem remains:

❯ env/bin/pip uninstall h3
Found existing installation: h3 4.0.0b5
Uninstalling h3-4.0.0b5:
  Would remove:
    /Users/aj/work/daily/2024-06-01/env/CHANGELOG.md
    /Users/aj/work/daily/2024-06-01/env/CMakeLists.txt
    /Users/aj/work/daily/2024-06-01/env/LICENSE
    /Users/aj/work/daily/2024-06-01/env/include/h3/h3api.h
    /Users/aj/work/daily/2024-06-01/env/lib/cmake/h3/h3Config.cmake
    /Users/aj/work/daily/2024-06-01/env/lib/cmake/h3/h3ConfigVersion.cmake
    /Users/aj/work/daily/2024-06-01/env/lib/cmake/h3/h3Targets-release.cmake
    /Users/aj/work/daily/2024-06-01/env/lib/cmake/h3/h3Targets.cmake
    /Users/aj/work/daily/2024-06-01/env/lib/libh3.a
    /Users/aj/work/daily/2024-06-01/env/lib/python3.11/site-packages/h3-4.0.0b5.dist-info/*
    /Users/aj/work/daily/2024-06-01/env/lib/python3.11/site-packages/h3/*
    /Users/aj/work/daily/2024-06-01/env/makefile
    /Users/aj/work/daily/2024-06-01/env/pyproject.toml
    /Users/aj/work/daily/2024-06-01/env/readme.md
    /Users/aj/work/daily/2024-06-01/env/requirements.in
    /Users/aj/work/daily/2024-06-01/env/setup.py
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/CMakeLists.txt
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/LICENSE
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/README.md
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/VERSION
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/cmake/Config.cmake.in
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/cmake/TestWrapValgrind.cmake
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/cmake/toolchain.cmake
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/include/algos.h
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/include/alloc.h
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/include/baseCells.h
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/include/bbox.h
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/include/constants.h
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/include/coordijk.h
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/include/directedEdge.h
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/include/faceijk.h
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/include/h3Index.h
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/include/h3api.h.in
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/include/iterators.h
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/include/latLng.h
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/include/linkedGeo.h
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/include/localij.h
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/include/mathExtensions.h
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/include/polygon.h
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/include/polygonAlgos.h
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/include/vec2d.h
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/include/vec3d.h
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/include/vertex.h
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/include/vertexGraph.h
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/lib/algos.c
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/lib/baseCells.c
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/lib/bbox.c
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/lib/coordijk.c
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/lib/directedEdge.c
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/lib/faceijk.c
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/lib/h3Index.c
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/lib/iterators.c
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/lib/latLng.c
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/lib/linkedGeo.c
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/lib/localij.c
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/lib/mathExtensions.c
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/lib/polygon.c
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/lib/vec2d.c
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/lib/vec3d.c
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/lib/vertex.c
    /Users/aj/work/daily/2024-06-01/env/src/h3lib/src/h3lib/lib/vertexGraph.c
Proceed (Y/n)? y
  Successfully uninstalled h3-4.0.0b5

[ajfriend]

In parallel, I'm also trying #378

[wingkitlee0]

pip install git+https://github.com/wingkitlee0/h3-py.git@fix-manifest-in

Interesting. This is yet another way to install it..

I would like to know if people can pip install:

• .tar.gz
• .whl
  and see if there is any loose files. Probably the current PR can't solve all the cases (but hopefully it does solve some of them)

I dont have strong opinion as it may simply make more sense to migrate to other build system (if they work with Cython and CMake)