Skip to content

fix: FindPython by default logic error #5561

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

Merged
merged 1 commit into from
Mar 20, 2025

Conversation

henryiii
Copy link
Collaborator

Followup to #5553. Fixes an issue with "COMPAT" evaluating to false.

@PhilipDeegan
Copy link
Contributor

having tested this it does not appear to solve our issue

@henryiii
Copy link
Collaborator Author

henryiii commented Mar 17, 2025

Okay, I'll see if I can reproduce. Thanks for checking!

@henryiii
Copy link
Collaborator Author

henryiii commented Mar 20, 2025

I'm confused a bit by PHARE. Why is a pybind11 module being linked with pybind11::embed in src/initializer/CMakeLists.txt? The one in src/phare/CMakeLists.txt looks more reasonable, as that's not a Python module, so I'm guessing it's calling Python. But a Python module doesn't need to / want to embed Python, it's being called from Python. We don't have explicit support for sub interpreters, though you can do it with the C API and mix it with pybind11. (And if you just need to run Python code from a Python module, that's in pybind11/eval.h, not pybind11/embed.h)

In CMakeLists.txt, you do find_package(Python 3.8 COMPONENTS Interpreter Development.Module REQUIRED) - that is, you explicitly avoid the the Development.Embed (which is part of Development along with Development.Module). You shouldn't be using embed targets if you don't find the embed component?

I've tried building locally on my Mac, but I'm hitting:

tests/initializer/test-initializer
: && /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -O3 -DNDEBUG -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.2.sdk -Wl,-search_paths_first -Wl,-headerpad_max_install_names  tests/initializer/CMakeFiles/test-initializer.dir/test_initializer.cpp.o -o tests/initializer/test-initializer  -Wl,-rpath,/Users/henryschreiner/git/software/PHARE/build/subprojects/samrai/lib  subprojects/samrai/lib/libphare_core.a  lib/libgtest.a  lib/libgmock.a  subprojects/samrai/lib/libphare_initializer.dylib  /usr/local/Cellar/open-mpi/5.0.7/lib/libmpi.dylib  lib/libgtest.a && :
ld: warning: ignoring duplicate libraries: 'lib/libgtest.a'
Undefined symbols for architecture x86_64:
  "_PyBaseObject_Type", referenced from:
      pybind11::detail::make_object_base_type(_typeobject*) in test_initializer.cpp.o
  "_PyByteArray_AsString", referenced from:
      bool pybind11::detail::string_caster<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, false>::load_raw<char>(std::__1::enable_if<std::is_same<char, char>::value, pybind11::handle>::type) in test_initializer.cpp.o
...

Both using master and the previous tag. Is this where it normally fails? If I can get a pass-fail between the two commits, I can debug. If I change Development.Module to Development, it passes both before and after.

@PhilipDeegan
Copy link
Contributor

PhilipDeegan commented Mar 20, 2025

I have to test your latest commit for NEWPYTHON to see if it fixes the issue. That might simplify things. (Edit: I see it's a docstring change so unlikely)

The linker error you're seeing is the issue. The latest commit to phare pins the pybind version to the latest release which does not have the issue.

src/initializer/CMakeLists.txt has two targets, one pybind module and one shared lib with an interpreter.

I didn't know about Development.Embed, so I'll check

Our cmake is setup to use the system pybind by default if found, otherwise it clones to subprojects/pybind

You can force to ignore the system pybind with cmake like https://github.com/PHAREHUB/PHARE/blob/master/res/cmake/dep/pybind.cmake#L22

@PhilipDeegan
Copy link
Contributor

I'm going to guess it's the lack of Development.Embed and the previous pybind commits were getting it for us somehow, otherwise I don't see how things would have ever linked at all. Let me check that and I'll get back to you so you don't put more time than necessary into this

@PhilipDeegan
Copy link
Contributor

it was that, somehow the commits prior to #5553 were giving us embed. thanks for your time Henry.

@henryiii
Copy link
Collaborator Author

Okay, thanks! I think this still is a logic improvement, so will go with this too.

@henryiii henryiii merged commit bb504dd into pybind:master Mar 20, 2025
80 checks passed
@henryiii henryiii deleted the henryiii/fix/fpdef branch March 20, 2025 21:38
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Mar 20, 2025
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Mar 31, 2025
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