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

Use system pybind11 and absl when available #73

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

ndl
Copy link
Contributor

@ndl ndl commented Jan 1, 2022

Current CMakeLists.txt always downloads and builds pybind11 and absl which conflicts with packaging best practices in some of the distributions. This change uses system libraries (if available) and falls back to the old behavior (if not).

Note: it's better to view the diffs with white space changes ignored, than it's much more clear than the change is just a couple lines of code.

set(ABSEIL_CMAKE_ARGS
${ABSEIL_CMAKE_ARGS}
"-DCMAKE_OSX_ARCHITECTURES=${CMAKE_OSX_ARCHITECTURES}")
find_package(pybind11 CONFIG)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we require a minimal pybind11 version here?

Copy link

Choose a reason for hiding this comment

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

For example, it compiles with 2.10.4.

find_package(pybind11 2.10.4 REQUIRED CONFIG)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for delay - updated, PTAL.

@BwL1289
Copy link

BwL1289 commented Aug 27, 2024

@superbobry bumping this. I'll fork if needed, but would prefer to use your version since it should already be done.

Current CMakeLists.txt always downloads and builds `pybind11` and `absl` which conflicts with packaging best practices in some of the distributions. This change uses system libraries (if available) and falls back to the old behavior (if not).
@BwL1289
Copy link

BwL1289 commented Aug 27, 2024

thank you @ndl.

@copybara-service copybara-service bot merged commit 63f25d4 into google-deepmind:master Aug 28, 2024
2 of 28 checks passed
@superbobry
Copy link
Collaborator

Thanks!

@BwL1289
Copy link

BwL1289 commented Aug 28, 2024

thank you so much @superbobry!

@BwL1289
Copy link

BwL1289 commented Aug 28, 2024

BTW - can confirm this works as expected.

@adamjstewart
Copy link

Now we just need a new release

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.

5 participants