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

Ensure proper Pybind11 is found. #1961

Merged
merged 14 commits into from
Sep 20, 2022

Conversation

thorstenhater
Copy link
Contributor

@thorstenhater thorstenhater commented Aug 26, 2022

  • For now just report the version we included.
  • Blow up compilation if version is less than we need.

@brenthuisman
Copy link
Contributor

Should we do this for all submodules?

@thorstenhater
Copy link
Contributor Author

thorstenhater commented Sep 6, 2022

Yes, we can add these one by one, where needed. Here we struck a latent bug/config issue, so we added it.

@thorstenhater thorstenhater marked this pull request as ready for review September 6, 2022 12:47
@brenthuisman
Copy link
Contributor

  1. We ran into the issue with the Pybind submodule, but it is not specific to it. Let's add the same info to config() for all submodules.
  2. The Pybind version now lives in three places: python/CMakeLists.txt, spack/package.py and the commit-id of the submodule. Is there anything that can be done about this?

@thorstenhater
Copy link
Contributor Author

  1. One thing per PR, we are actively trying to get away from PRs with multiple concerns. This one is also it more involved as it adds a static assertion on the version (something we usually would not be doing). Also, it's not clear all submodules even have such a version tag. One would hope so, but...
  2. It also lives in the C++ static assertion. Maybe we can centralise, but I am not sure it's worth the effort. So, the way would be to have CMake parse out the tag from the submodule eg v2.10.0 turn that in to a C++ define PB11=0x020A00 test that against the version in the header. And so on. However, we'd like to handle version ranges for upward compatibility. Also, this does only work when the submodule is there (which it is not unless the user builds with BUNDLED_LIBS). We could make CMake fetch submodules though and invite doom onto our house.

@brenthuisman
Copy link
Contributor

  1. Adding a flag for all submodules is one thing imho.
  2. I see that FetchContent let's you specify a tag (i.e. commit hash) to clone, which actually not bad? Cmake 3.24 gives us FETCHCONTENT_TRY_FIND_PACKAGE_MODE, which would make switching between sytem provided libs and Arbor provided libs simpler. The biggest con is that we can't (easily) create a self-contained tarball anymore.

@thorstenhater
Copy link
Contributor Author

  1. So, why is this blocking acceptance? Is the proposed change missing something wrt to the problem it solves? Adding more fields will not solve the pybind11 any better.
  2. You can, if you make a target 'MakeTarBall' to be executed by make and configured via CMake.

@brenthuisman
Copy link
Contributor

  1. In my view, it's not that much extra to keep __config__ consistent by showing the same info for all submodules. But, since you ask so nicely, I'll stop whining about that now. One tiny request: shall we drop the Pybind11 upper limit (3.0.0)? It's not present in spack/package.py either, introducing a potential surprise factor. Also, someone told me that setting these ranges overly narrow is making live difficult for those configuring sets of packages.
  2. Aha.

@thorstenhater
Copy link
Contributor Author

The upper limit is to ensure we do not cross a major version. If they follow semver (fingers crossed) that would mean
they will not break API. (<1.0.0 rules are different 😉)

@thorstenhater
Copy link
Contributor Author

Ok, let's try to trust CMake here

The [version] argument requests a version with which the package found should be compatible.

The keyword here is 'compatible'.

@brenthuisman brenthuisman merged commit 8b388e6 into arbor-sim:master Sep 20, 2022
@thorstenhater thorstenhater deleted the bug/find-correct-cmake branch September 20, 2022 09:32
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