-
Notifications
You must be signed in to change notification settings - Fork 192
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
Make finding Python in build system more robust, update Docker container #2672
Conversation
|
||
# We can't rely on CMake 3.14 quite yet so we backport the `Python::NumPy` | ||
# imported target that FindPython in CMake 3.14+ could also provide. | ||
find_package(NumPy 1.10 REQUIRED) |
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.
Do we/can we check that SciPy was correctly found with finding NumPy? I believe nowadays it is, but I don't know when that changed.
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.
Probably a good idea to check that, added a fixup
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.
LGTM! Please go ahead and squash :)
706bacc
to
61b8c24
Compare
@nilsleiffischer this needs a rebase to fix a (hopefully minor) conflict |
@wthrowe do you want to test this out on your machine? |
Followed by a bunch of similar errors. |
@wthrowe could you post the full cmake trace please? |
|
@wthrowe Have you installed pybind11 via pip? Why are you passing |
I'll add that something like |
No, I installed it from my distro's package.
This is a good question. I assume I added it for some reason in the past, but it doesn't seem to be necessary on develop. Removing it did not fix the errors, though. |
61b8c24
to
930be99
Compare
Yes it definitely should be supported. I also installed via @wthrowe which version of pybind11 did you install? Please try 2.6.0 or later (they added |
Question: is there a way we can require a specific version and newer? It sounds like part of the trouble is that? I noticed you updated the docs, not sure if the |
930be99
to
59b61d6
Compare
Sure, added the version requirement to the |
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.
LGTM thanks!
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.
@kidder have you tested this on your machine?
.github/workflows/Tests.yaml
Outdated
@@ -114,7 +114,7 @@ jobs: | |||
|| github.ref != 'refs/heads/develop' | |||
runs-on: ubuntu-latest | |||
container: | |||
image: sxscollaboration/spectrebuildenv:latest | |||
image: nilsleiffischer/spectrebuildenv:pybind11_with_pip |
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.
So we don't forget: this needs to get set back to the right container
Added another commit that cleans up the link dependencies for the Pybindings in the build system. |
I was using 2.5.0. It works after upgrading to 2.6.0. |
works for me; needs a rebase |
- Switch to CMake's FindPython - Install Pybind11 through pip. This should work on all systems that support Python. It means we can use Pybind11's CMake tools, don't have to bundle them and don't need special finding-code.
Add missing link dependencies and link explicitly with pybind11 when including its headers.
This ensures that pre-built binaries are used if possible.
7094afd
to
6b3d1dd
Compare
@nilsdeppe @kidder @wthrowe rebased, and added a few more container updates so we don't have to rebuild the container too many times. CI runs over the new container successfully here. I'll push the new container to |
&& ./build LIBS multicore-linux-x86_64 gcc ${PARALLEL_MAKE_ARG} -g -O0 \ | ||
&& ./build charm++ multicore-linux-x86_64 clang ${PARALLEL_MAKE_ARG} -g -O0 \ | ||
&& ./build LIBS multicore-linux-x86_64 clang ${PARALLEL_MAKE_ARG} -g -O0 \ | ||
&& ./build LIBS multicore-linux-x86_64 gcc \ |
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.
why build the shared libs here? Linux allows you to mix shared and static libs as long as the static libs are built with -fPIC
and our build system doesn't look for Charm++'s shared libs (because they stick them in the wrong directory)
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.
The Charm docs say to compile shared libs: https://github.com/UIUC-PPL/charm#building-dynamic-libraries. Then we can pass -charm-shared
to charmc --print-building-blocks
and get the compiler configuration for shared libs. If we don't do both (have the Charm shared libs compiled and pass the -charm-shared
flag) then I don't think they guarantee that things will work.
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.
Sounds good!
Proposed changes
support Python. It means we can use Pybind11's CMake tools,
don't have to bundle them and don't need special finding-code.
Upgrade instructions
To specify a Python executable when configuring CMake, use the variable
Python_EXECUTABLE
instead of the oldPYTHON_EXECUTABLE
, e.g.:More information on configuring CMake with Python: https://cmake.org/cmake/help/latest/module/FindPython.html
If you use the Python bindings at all, install Pybind11 via
pip
:More information on installing Pybind11: https://pybind11.readthedocs.io/en/stable/installing.html
CMake should find the installation automatically. If you run into trouble, make sure you use the Python executable from the environment that you have installed pybind11 in, then file an issue. More information on finding Pybind11 in CMake: https://pybind11.readthedocs.io/en/stable/cmake/index.html
Code review checklist
make doc
to generate the documentation locally intoBUILD_DIR/docs/html
.Then open
index.html
.code review guide.
bugfix
ormajor new feature
if appropriate.Further comments