h3-py icon indicating copy to clipboard operation
h3-py copied to clipboard
verified publisher icon uber

Fix loose files during install

Open wingkitlee0 opened this issue 1 year ago • 17 comments

To solve https://github.com/uber/h3-py/issues/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

wingkitlee0 avatar May 29 '24 03:05 wingkitlee0

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar May 29 '24 03:05 CLAassistant

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (d3f6c1d) to head (cdb881b). Report is 6 commits behind head on master.

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 29 '24 03:05 codecov[bot]

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 avatar May 29 '24 04:05 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?

ajfriend avatar May 29 '24 04:05 ajfriend

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

dfellis avatar May 29 '24 16:05 dfellis

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 avatar May 29 '24 22:05 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 avatar May 29 '24 22:05 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..

wingkitlee0 avatar May 29 '24 22:05 wingkitlee0

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

ajfriend avatar May 30 '24 00:05 ajfriend

@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 avatar May 30 '24 00:05 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!

wingkitlee0 avatar May 30 '24 01:05 wingkitlee0

Awesome. Great work!

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

ajfriend avatar May 30 '24 01:05 ajfriend

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/

wingkitlee0 avatar May 30 '24 02:05 wingkitlee0

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.

ajfriend avatar May 30 '24 17:05 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 avatar Jun 01 '24 19:06 ajfriend

In parallel, I'm also trying https://github.com/uber/h3-py/pull/378

ajfriend avatar Jun 02 '24 20:06 ajfriend

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)

wingkitlee0 avatar Jun 03 '24 22:06 wingkitlee0

Closing since we moved to the new build system in https://github.com/uber/h3-py/pull/378

ajfriend avatar Sep 26 '24 19:09 ajfriend