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

Trying pyproject.toml and scikit-build-core #378

Merged
merged 38 commits into from
Sep 4, 2024
Merged

Trying pyproject.toml and scikit-build-core #378

merged 38 commits into from
Sep 4, 2024

Conversation

ajfriend
Copy link
Contributor

@ajfriend ajfriend commented Jun 1, 2024

Trying for a PEP 621 compatible setup using pyproject.toml instead of setup.py. Also trying to use scikit-build-core.

@ajfriend ajfriend marked this pull request as draft June 1, 2024 21:22
@ajfriend ajfriend added the help wanted Extra attention is needed label Jun 1, 2024
@ajfriend ajfriend changed the title Trying a scikit-build-core setup Trying pyproject.toml and scikit-build-core Jun 1, 2024
@ajfriend
Copy link
Contributor Author

ajfriend commented Jun 2, 2024

Currently running into an issue:

  CMake Error at src/h3/_cy/CMakeLists.txt:1 (find_package):
    No "FindCython.cmake" found in CMAKE_MODULE_PATH.


  CMake Warning (dev) at src/h3/_cy/CMakeLists.txt:1 (find_package):
    FindCython.cmake must either be part of this project itself, in this case
    adjust CMAKE_MODULE_PATH so that it points to the correct location inside
    its source tree.

    Or it must be installed by a package which has already been found via
    find_package().  In this case make sure that package has indeed been found
    and adjust CMAKE_MODULE_PATH to contain the location where that package has
    installed FindCython.cmake.  This must be a location provided by that
    package.  This error in general means that the buildsystem of this project
    is relying on a Find-module without ensuring that it is actually available.

Based on the comment "You’ll need to handle the generation of files by Cython directly at the moment. A helper (similar to scikit-build classic) might be added in the future." in https://scikit-build-core.readthedocs.io/en/latest/getting_started.html#cmake-file, I'm wondering if this is something from scikit-build classic that we need to remove and just do manually?

Copy link

codecov bot commented Jun 21, 2024

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Project coverage is 99.33%. Comparing base (cec946a) to head (121314a).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
tests/test_cython/test_cython.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##            master     #378      +/-   ##
===========================================
- Coverage   100.00%   99.33%   -0.67%     
===========================================
  Files           10       28      +18     
  Lines          343     1346    +1003     
===========================================
+ Hits           343     1337     +994     
- Misses           0        9       +9     

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

@ajfriend
Copy link
Contributor Author

ajfriend commented Jun 22, 2024

Tests are running fine, but we need to figure out how to build the annotations again: https:/scikit-build/cython-cmake/blob/main/src/cython_cmake/cmake/UseCython.cmake#L201-L202

I've tried just dropping in an --annotate in the CMakeLists.txt but that doesn't seem to be working.

@ajfriend ajfriend marked this pull request as ready for review June 22, 2024 03:00
@ajfriend
Copy link
Contributor Author

TODO: We were previously excluding unnecessary files in the sdist with MANIFEST.in. Those files are now being included, so we need to recover that functionality with this setup.

@ajfriend
Copy link
Contributor Author

Coverage decreased because I added the test files themselves to be included in the coverage report.

@ajfriend
Copy link
Contributor Author

Coverage decreased because I added the test files themselves to be included in the coverage report.

And a good thing too, because it caught a bug!

Our Cython example is no longer working:

❯ env/bin/cythonize -i tests/test_cython/cython_example.pyx
/Users/aj/work/h3-py/tests/test_cython/cython_example.c:22015:13: warning: code will never be executed [-Wunreachable-code]
            goto bad;
            ^~~~~~~~
1 warning generated.
ld: warning: duplicate -rpath '/Users/aj/.pyenv/versions/3.12.3/lib' ignored
ld: warning: duplicate -rpath '/opt/homebrew/lib' ignored
error: could not create 'src/tests/test_cython/cython_example.cpython-312-darwin.so': No such file or directory

pyproject.toml Outdated


[project.optional-dependencies]
numpy = ['numpy<2']
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need to be limited to numpy<2?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope! Dropped it.

pieterprovoost and others added 9 commits September 3, 2024 18:16
* Update CHANGELOG.md

* Update h3lib.pxd

* Update latlng.pxd

* Update latlng.pyx

* Update util.pxd

* Update util.pyx

* Update _version.py

* Update __init__.py

* Update __init__.py

* Update test_h3.py

* Update __init__.py

* Update test_h3.py

* Update CHANGELOG.md

* Update __init__.py

* Update test_h3.py

* Update _version.py

* Update test_h3.py

* Update __init__.py

* Update CMakeLists.txt

* Update latlng.pxd

* Update latlng.pyx

* Create vertex.pxd

* Create vertex.pyx

* Update __init__.py

* Update test_h3.py

* Update __init__.py

* Update api_quick.md
…ows (#392)

Bumps [actions/download-artifact](https:/actions/download-artifact) from 4.1.4 to 4.1.7.
- [Release notes](https:/actions/download-artifact/releases)
- [Commits](actions/download-artifact@v4.1.4...v4.1.7)

---
updated-dependencies:
- dependency-name: actions/download-artifact
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@ajfriend
Copy link
Contributor Author

ajfriend commented Sep 4, 2024

Everything should work, except for #396

I think we should merge this change to master and fix the issue from there.

@ajfriend ajfriend merged commit 65c3d9e into master Sep 4, 2024
37 of 39 checks passed
@ajfriend ajfriend deleted the aj/toml branch September 4, 2024 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants