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

feat: migrate to scikit-build-core #7

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

cringeyburger
Copy link

This pull request will migrate the build backend from "setuptools" and "wheel" to "scikit-build-core" along will the updated workflows.

@cringeyburger cringeyburger requested a review from a team as a code owner October 24, 2024 02:48
Copy link
Contributor

@kratman kratman left a comment

Choose a reason for hiding this comment

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

Partial review

.github/workflows/build_wheels.yml Outdated Show resolved Hide resolved
OUTPUT_STRIP_TRAILING_WHITESPACE)
else()
set(CASADI_DIR $ENV{CASADI_PATH})
message("it is alr set: ${CASADI_DIR}")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a good message for printing, please clean this up

Copy link
Author

Choose a reason for hiding this comment

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

Oh, I forgot to change this. Thanks

Comment on lines +232 to +236
if(DEFINED ENV{CIBUILDWHEEL})
install(TARGETS idaklu DESTINATION "pybammsolvers")
else()
install(TARGETS idaklu DESTINATION "${CMAKE_CURRENT_SOURCE_DIR}/src/pybammsolvers")
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be consistent about where files are placed whenever possible

FindSUNDIALS.cmake Outdated Show resolved Hide resolved
"numpy<2.0"
]
license = { file = "LICENSE" }
version = "0.0.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't version be dynamic anymore?

Copy link
Author

Choose a reason for hiding this comment

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

The thing is, scikit-build-core has 3 built-in plugins for dynamic metadata.
The one used for versioning is setuptools_scm, and when I tried using it, it gave out responses similar to: 0.0.1.dev143+81. I tried tweaking it a bit and I found that we might have to use git tags. So I opted for manual version, instead of dynamic.
LMK if you are okay with that default scheme, I can change it.

Copy link
Member

Choose a reason for hiding this comment

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

I'd very much like to use VCS-based versioning (it's better in many scenarios), but better to avoid it in this PR and proceed with the metadata updates later.

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
where = ["src"]
include = ["pybammsolvers"]
[tool.hatch.metadata.hooks.fancy-pypi-readme]
content-type = "text/markdown"

Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we using hatch here when the rest is scikit-build-core?

Copy link
Author

Choose a reason for hiding this comment

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

scikit-build-core ships with 3 built-in plugins for dynamic metadata. One of them is hatch, and we need to use it if we want a dynamic readme.

Comment on lines +44 to +45
[[tool.hatch.metadata.hooks.fancy-pypi-readme.fragments]]
path = "README.md"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

Copy link
Author

Choose a reason for hiding this comment

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

As explained above, we need to use hatch if we want a dynamic readme.

cringeyburger and others added 2 commits October 24, 2024 20:14
style: format code

Co-authored-by: Eric G. Kratz <kratman@users.noreply.github.com>
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.

3 participants