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

Download pybind11 via CMake instead of shipping it #86

Merged
merged 6 commits into from
Sep 14, 2022

Conversation

volkm
Copy link
Contributor

@volkm volkm commented Aug 31, 2022

Download pybind11 in the configure step such that it is available for add_subdirectory.
Should best be merged simultaneous to the similar PR in pycarl moves-rwth/pycarl#15.

@volkm
Copy link
Contributor Author

volkm commented Sep 1, 2022

Will also update pybind to version 2.10.0 and should fix #68.

@sjunges
Copy link
Contributor

sjunges commented Sep 1, 2022

I think that ultimately, it would be good if we could set the pybind version as an option. That way, if someone depends on a particular pybind version, it is easier to fix. Not urgent though.

Otherwise: LGTM and many thanks

@volkm
Copy link
Contributor Author

volkm commented Sep 1, 2022

One can already define the version in include_pybind11.cmake. Setting it in CMake and from Python setup.py should be easy to implement. Setting the version should however be an expert option, because pybind updates might require code adaptions.

@sjunges
Copy link
Contributor

sjunges commented Sep 1, 2022

As it currently must be the same as pycarls version, does it make sense to take the pycarl version as default?

@sjunges sjunges mentioned this pull request Sep 1, 2022
@volkm
Copy link
Contributor Author

volkm commented Sep 1, 2022

The recent changes allow to set the pybind version to use. As default we are using the pybind version exported by pycarl (see moves-rwth/pycarl#15). Note that this requires to import pycarl during the build process and pycarl was added to setup_requires.

@volkm
Copy link
Contributor Author

volkm commented Sep 1, 2022

Tests are currently failing, because moves-rwth/pycarl#15 is not used yet.

@sjunges
Copy link
Contributor

sjunges commented Sep 14, 2022

LGTM

@volkm volkm merged commit 3671d48 into moves-rwth:master Sep 14, 2022
@volkm volkm deleted the pybind_update branch September 14, 2022 11:03
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