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

Use scikit-build-core #263

Merged
merged 29 commits into from
Apr 23, 2024
Merged

Use scikit-build-core #263

merged 29 commits into from
Apr 23, 2024

Conversation

gmloose
Copy link
Collaborator

@gmloose gmloose commented Mar 26, 2024

Use scikit-build-core to build the Python casacore bindings. It completely replaces the custom-made stuff in setup.py.

First working version. May still need a bit of tweaking.
Created separate `CMakeLists.txt` files for the `src` and `tests` directory.
Removed unused environment variables in `pyproject.toml`.
Try to configure as much as possible, so that more than one error can be reported.
The same CMake settings need to be applied to all libraries. Do this in a `foreach` loop that loops over all targets.
Formatted the `CMakeLists.txt` files using `cmake-format`.
@gmloose gmloose self-assigned this Mar 26, 2024
@gmloose gmloose requested a review from tammojan March 26, 2024 11:02
@gmloose gmloose marked this pull request as draft March 26, 2024 11:04
Editable installs don't work with `pip` and `pyproject.toml`.
The new build system uses `pkg-config` to locate Casacore. At it to the list of packages that must be installed.
Based `py3_casacore_master` on `ubuntu:24.04` and fixed the job.
Replaced `nose` with the more modern `pytest` and `pytest-cov` for measuring test coverage. Coverage report is sent to stdout.
The coverage badge pointed to a page that hasn't been updated since 2017. It has been removed.
TODO: perform coverage tests and add a new badge.
@gmloose gmloose marked this pull request as ready for review March 27, 2024 10:59
Copy link
Contributor

@tammojan tammojan left a comment

Choose a reason for hiding this comment

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

Great to get rid of this setup.py with the awful boost check, and move to CMake and modern python build systems!

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
src/CMakeLists.txt Show resolved Hide resolved
Not sure why, but when using `FindCasacore.cmake` (adopted from `schaapcommon`), we need to have `liblapack-dev` installed.
Do not depend on an explicit Ubunut version, use `latest` instead.
Forgot to uppercase _all_ references to variable names `Casacore_*`.
@gmloose
Copy link
Collaborator Author

gmloose commented Apr 8, 2024

I think I addressed all your review comments. Ready for second review.

@gmloose
Copy link
Collaborator Author

gmloose commented Apr 8, 2024

I do see that my current one-shoe-fits-all solution for linking results in over-linking. I don't think this is a very big issue, because all libraries are in the binary wheel anyway. It will, of course, give a little run-time overhead and a waste of some RAM memory. @tammojan, if you think it's worth the effort to dive into this, I will.

Only link to the libraries that are actually needed by passing the `--as-needed` option to the linker.
@gmloose
Copy link
Collaborator Author

gmloose commented Apr 8, 2024

I fixed the over-linking issue in commit 693f7ca

@gmloose
Copy link
Collaborator Author

gmloose commented Apr 8, 2024

The OS-X build is again broken. Now with:

 CMake Error at /usr/local/Cellar/cmake/3.28.1/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find Python3 (missing: Python3_NumPy_INCLUDE_DIRS NumPy) (found
  version "3.12.1")

I have no idea how to fix this, because I don't know how to reproduce this problem.

Undid some unnecessary changes to some of the no longer used GitHub workflows. These changes wouldn't have had an effect anyway.
README.rst Outdated Show resolved Hide resolved
@tammojan
Copy link
Contributor

The OS-X build is again broken. Now with:

 CMake Error at /usr/local/Cellar/cmake/3.28.1/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find Python3 (missing: Python3_NumPy_INCLUDE_DIRS NumPy) (found
  version "3.12.1")

I have no idea how to fix this, because I don't know how to reproduce this problem.

It's a problem in the installation of casacore itself. I think this could be solved by making boost-python an optional requirement in casacore's CMake.

Point to main WCSlib page, instead of tar ball.
Copy link
Contributor

@tammojan tammojan left a comment

Choose a reason for hiding this comment

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

Looks great to me! I think this is ready to merge.

@gmloose gmloose merged commit aedace0 into master Apr 23, 2024
4 of 5 checks passed
@gmloose gmloose deleted the use_scikit-build-core branch August 30, 2024 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants