-
Notifications
You must be signed in to change notification settings - Fork 2
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
Enable binary wheels for all python versions and OS #30
Conversation
A bit cleaner
Never set in script, allow to be overwritten by user
(new one for me)
Thanks to pybind11 magic in scikit-build-core
Hello 2024!
Probably we should set up a time to chat about this PR. Shoot me an email at ian.bell@nist.gov and we can discuss a few things. |
CMakeLists.txt
Outdated
# set(CMAKE_CXX_COMPILER /usr/bin/g++) # Setting these can, on some systems, cause cmake to go into an infinite loop (see: build.sh to set the compiler) | ||
# set(CMAKE_C_COMPILER /usr/bin/gcc) # Setting these can, on some systems, cause cmake to go into an infinite loop (see: build.sh to set the compiler) | ||
if(APPLE) | ||
set(CMAKE_CXX_FLAGS "-Wall -Wextra -Wno-unknown-pragmas -Wno-sign-compare -pthread -arch arm64") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooops this should be fixed. The arch should not be specified this way. It should instead be something like: https://github.com/usnistgov/teqp/blob/201ceb7c59780bd93df484b90c2b2e986823116f/CMakeLists.txt#L2
Nice stuff! I'll have a closer look at it in the coming couple of days, then we can resolve any final tweaks :) |
Aside: it is better to leave the name the same in debug and release builds. Then you can just flag to build one or the other. |
I agree, what you've caught there are some more leftovers from early development that was never cleaned out. I've had a list of to-do's in cleaning up and/or streamlining quite a bit of stuff (pybind11 bindings, the integration module for collision integrals, the factorial module, etc.) which I try to find time for every now and then. |
Yup, that's the way these things go. We'll get there. The sticking point right now is a version of thermopack that will work ok on all platforms and python versions. I guess they are not doing cibuildwheels for that either; I had volunteered to help with that some time back. |
I've had a little bit of time to look at this now, the only issue I can see is that As of now, this package is (to my knowlege) far more commonly compiled from source for consumption rather than distribution. The way I see it, making sure it is easy for others than the lead devs to compile from source is more important than streamlining the distribution builds. In the same line, the |
You shouldn't need to do anything magic with the python executable, it should be found properly, automatically. I know that works for me almost always and I use conda environments heavily, and I should hope the same thing would work properly with venv. For pybind11, I typically use it as a git submodule because I don't like global installs either. Let's plan to sit down together in Boulder and go over the details. |
For some reason |
Hrm. Usually that "just works" but to be honest I don't use venv, I always use conda environments and I think the scientific python folks do too. I would make the leap to scikit-build-core, I have been using that extensively in a new library of mine and been happy with it, allows for a very responsive build system and easy debugging. |
I've figured out of the remaining issues and put them up on #32, so I'll close this. In case anyone comes looking here I'll mention that the major problem was that files ignored by
to |
I also looked into act because I had the same problem and I recall that I couldn't get it working, but I don't remember why. |
I just jumped ahead and went right to scikit-build-wheels. We should test that the wheels all work, once we figure out a reasonable version of thermopack.
See #28