Open Krzmbrzl opened 1 year ago
Cherry pick https://github.com/pybind/pybind11/commit/dc7bb5db9a8688374bc16651fb81c964f9d62bf1 onto upstream pybind11.
Reason for fork is this is not included in upstream (for obvious reasons) plus several bug fixes
On Fri, Sep 8, 2023, 10:46 AM Robert Adam @.***> wrote:
When using clang to compile SeQuant, the Python bindings (enabled via the SEQUANT_PYTHON option) fail to compile with the error
In file included from /Data/home/user/Documents/Git/SeQuant/python/src/sequant/_sequant.cc:12: In file included from /Data/home/user/Documents/Git/SeQuant/python/src/sequant/mbpt.h:11: In file included from /Data/home/user/Documents/Git/SeQuant/python/src/sequant/python.h:4: In file included from /Data/home/user/Documents/Git/SeQuant/build_clang/_deps/pybind11-src/include/pybind11/boost/container/small_vector.h:3: In file included from /Data/home/user/Documents/Git/SeQuant/build_clang/_deps/pybind11-src/include/pybind11/boost/container/../../stl.h:12: /Data/home/user/Documents/Git/SeQuant/build_clang/_deps/pybind11-src/include/pybind11/boost/container/../../pybind11.h:1010:9: error: no matching function for call to 'operator delete' ::operator delete(p, s);
There is an upstream bug report https://github.com/pybind/pybind11/issues/1604 about this, that indicates that this has been fixed in pybind version >= 2.4.4. However, you are using a custom fork and within that you seem to be on version 2.4.3, which still contains this error.
I tried the quick fix of pointing to version 2.5.0 in your fork, but then an include to something like pybind11/boost/container/small_vector fails to be resolved (from SeQuant's files). Thus, I am not entirely sure how you would want to proceed with this. I was also wondering why you were using a fork to begin with. It doesn't seem like you have made any kind of patches to pybind11 🤔
— Reply to this email directly, view it on GitHub https://github.com/ValeevGroup/SeQuant/issues/143, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABH62KGBRPCFK7LWX6Q5AH3XZMOUDANCNFSM6AAAAAA4QNJX3E . You are receiving this because you are subscribed to this thread.Message ID: @.***>
Thanks! I just saw that you have the master branch unpatched, but have your patches located in https://github.com/ValeevGroup/pybind11/tree/ValeevGroup/v2.4.3. That's why I thought your fork was unchanged.
Anyway, I don't yet need Python bindings - it's just something I stumbled upon and thought I'd let you know. It seems a bit more complicated for me to first update your fork via PR and then submit a PR to this repo once the fork as been updated, than if someone of you (with write access to the repos) patched this. If you want me to create those PRs instead though, just let me know.
I ll fix it myself, next week. busy could days
On Fri, Sep 8, 2023, 11:04 AM Robert Adam @.***> wrote:
Thanks! I just saw that you have the master branch unpatched, but have your patches located in https://github.com/ValeevGroup/pybind11/tree/ValeevGroup/v2.4.3. That's why I thought your fork was unchanged.
Anyway, I don't yet need Python bindings - it's just something I stumbled upon and thought I'd let you know. It seems a bit more complicated for me to first update your fork via PR and then submit a PR to this repo once the fork as been updated, than if someone of you (with write access to the repos) patched this. If you want me to create those PRs instead though, just let me know.
— Reply to this email directly, view it on GitHub https://github.com/ValeevGroup/SeQuant/issues/143#issuecomment-1711727193, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABH62KBXDJBUN6Q4DF7QOITXZMQW5ANCNFSM6AAAAAA4QNJX3E . You are receiving this because you commented.Message ID: @.***>
@asadchev would it make sense to add https://github.com/pybind/pybind11/commit/dc7bb5db9a8688374bc16651fb81c964f9d62bf1 to SeQuant directly?
Let me look next week. Patches were necessary but should be easy to port and one maybe already in pybind tree
On Wed, Nov 8, 2023, 11:52 AM Eduard Valeyev @.***> wrote:
@asadchev https://github.com/asadchev would it make sense to add @.*** https://github.com/pybind/pybind11/commit/dc7bb5db9a8688374bc16651fb81c964f9d62bf1 to SeQuant directly?
— Reply to this email directly, view it on GitHub https://github.com/ValeevGroup/SeQuant/issues/143#issuecomment-1802552947, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABH62KFVKCQ7R6C5JQRMTITYDPPIPAVCNFSM6AAAAAA4QNJX3GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBSGU2TEOJUG4 . You are receiving this because you were mentioned.Message ID: @.***>
@Krzmbrzl switching to https://github.com/ValeevGroup/pybind11/tree/ValeevGroup/v2.11 should help
When using
clang
to compile SeQuant, the Python bindings (enabled via theSEQUANT_PYTHON
option) fail to compile with the errorThere is an upstream bug report about this, that indicates that this has been fixed in pybind version >= 2.4.4. However, you are using a custom fork and within that you seem to be on version 2.4.3, which still contains this error.
I tried the quick fix of pointing to version 2.5.0 in your fork, but then an include to something like
pybind11/boost/container/small_vector
fails to be resolved (from SeQuant's files). Thus, I am not entirely sure how you would want to proceed with this. I was also wondering why you were using a fork to begin with. It doesn't seem like you have made any kind of patches to pybind11 :thinking: