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

redesign cmake entrypoint #768

Merged
merged 8 commits into from
Jun 14, 2024
Merged

redesign cmake entrypoint #768

merged 8 commits into from
Jun 14, 2024

Conversation

kiwifb
Copy link
Contributor

@kiwifb kiwifb commented Jun 14, 2024

This is a sort of follow up to #413 and #414. In sage-on-gentoo we discovered that the presence of scikit-build-core breaks the building sagemath (cschwan/sage-on-gentoo#783). scikit-build-core also happens to be an indirect dependency for building sagemath's documentation.
On the basis that the presence of setuptolls_rust does not break sagemath's build, I studied the difference with scikit-build-core. I noticed that the elements `bundled in the single "finalize_distribution_options" entrypoint in scikit is split between two different setuptools entrypoints.
This PR reproduce that setup and does a bit of clean up. The bundling of the old _prepare_extension_detection and _prepare_build_cmake_command into finalize_distribution_options being redundant after attributing them directly to separate entrypoints. I also renamed the functions to better characterise them.

Using this PR, sagemath builds without problems and pyzmq, which uses scikit-build-core also builds properly.

Copy link
Collaborator

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

I think it might be better to match the entry points and names?

src/scikit_build_core/setuptools/build_cmake.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jun 14, 2024

Codecov Report

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

Project coverage is 81.96%. Comparing base (d9c41f9) to head (cbbe4c0).
Report is 91 commits behind head on main.

Files with missing lines Patch % Lines
src/scikit_build_core/setuptools/build_cmake.py 91.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #768      +/-   ##
==========================================
- Coverage   81.98%   81.96%   -0.02%     
==========================================
  Files          68       68              
  Lines        3919     3921       +2     
==========================================
+ Hits         3213     3214       +1     
- Misses        706      707       +1     

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

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@henryiii
Copy link
Collaborator

Could you try it with my changes and see if that fixes sagemath?

@henryiii
Copy link
Collaborator

(pyzmq doesn't use the setuptools plugin, so it should be fine. I think we only have ~1 user so far of the setuptools plugin.)

@kiwifb
Copy link
Contributor Author

kiwifb commented Jun 14, 2024

I just checked and sagemath still builds with your newest changes. I must say, I was not sure it would.

@henryiii
Copy link
Collaborator

This makes sure nothing is injected unless a cmake_* keyword is present. Sagemath doesn't have one of those, so it's fine. :)

The injection should have been pretty safe, but this is safer.

@henryiii henryiii merged commit edddcc7 into scikit-build:main Jun 14, 2024
47 of 51 checks passed
@henryiii
Copy link
Collaborator

Thanks!

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