| Message ID | 20211204120548.31170-1-guillaume.bressaix@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [PATCH/next,v2,1/2] : pybind11: new package | expand |
Guillaume, All, On 2021-12-04 13:05 +0100, guillaume.bressaix@gmail.com spake thusly: > From: "Guillaume W. Bres" <guillaume.bressaix@gmail.com> > > fixes http://autobuild.buildroot.net/results/b89f1de64b308dffa73675f1f31ccb0b7be5a10d > fixes http://autobuild.buildroot.net/results/d0287b7f64f206b0f074908c5780a3632e0cb799 > fixes http://autobuild.buildroot.net/results/27efb545a5a719a5581c8f746d3a3555ff4216ce > fixes http://autobuild.buildroot.net/results/d2f0a0ad8f6c7178517df109e7d885dac9134c3a > fixes http://autobuild.buildroot.net/results/b57e9a3279260dae4a590f9421238fcabb2f7cab > fixes http://autobuild.buildroot.net/results/515e6f2fc6b5780260d98d6bb52b541ce4bf1afe > fixes http://autobuild.buildroot.net/results/d89c4ecc81222d4f80c951da2232d2e393fa1c69 > list goes on.. > > --- > v2: Reviewed by gwen@trabucayre.com, > Removed some non needed empty lines, > force -DFINDPYTHON=OFF when using pybind11 without python, > handle legacy package properly in a seperate patch > > v1: python-pybind was not the right approach and is in failure since > it's been upgraded to V2.6.1. This form here: > Building with setup.py now requires a cmake build first. > With this new approach we can build the package with cmake > for python bindings in C++ AND we also have the > C++ bindings in python as an option (depending & requiring the first one). > > I make this a host-only package, in the sense that other packages will > require it at build time, and I don't forsee any reasons to have > such a package as a target package. > > Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com> ... to here, should be part of the commit log, i.e. before the first '---' line, above. Otherwise, it is dropped by git when the patch is applied. However, I am still not sure what is going on here... First, you are removing pybind to then introduce pybind11, although they are the exact same package upstream: http://pybind11.readthedocs.org/en/master (home) https://github.com/pybind/pybind11 (repo) But since this is the same upstream, you should just fix the existing pybind package in-place; there is no reason to intriduce a new pacjage to fix an existing one. Then, I did not find the explanations in the commit log very convincing. Why state that "python-pybind was not the right approach"? As far as I understand, it was working as expected until the bump to 2.6.1, and thus was not a "failure". If the bump to 2.6.1 broke the package, then that means the bump was not careful, not that the package is a failure. Fionally, why is a host-only package? If it installs C++ headers, then we can expect packages built for the target to include those ehaders, and so we need them in staging too. And this is made obvious by your post-install hook, that uses STAGING_DIR. This is ultimately wrong, because then that means that host packages won't be able to find/use those headers. Minor nit: the commit log should not be in the first singular person, ie.e do not use "I", but in the `neutral' first person plural, i.e. use "we". However, I admit that the whole situation evades my understanding, so I may have miss more tricky details... I will gladly accept being corrected on those. ;-) Regards, Yann E. MORIN.
Hello Yann, thanks for the feedback. I integrated all your recommendations for my next submission, and I am currently testing all combinations. I will follow your suggestion and integrate this as a patch serie, so there won't be an actual v3 follow up, but this way you guys can see the whole picture. First, here's the answer to your question that is still pending: >But since this is the same upstream, you should just fix the existing >pybind package in-place; there is no reason to intriduce a new pacjage >to fix an existing one. This is not doable, because of the python-pkg naming convention. The previous approach was partially fine, that is true. It delivered C++/Py bindings (and only those) and was built as a python-pkg. So basically, the cmake side (Py/C++ bindings) was left out. Now the package requires cmake whatever happens, therefore it can no longer be a python-only package. >why is a host-only package? If it installs C++ headers, then >we can expect packages built for the target to include those headers, >and so we need them in staging too. And this is made obvious by your >post-install hook, that uses STAGING_DIR I am in the process of testing all the combinations, but I managed to have a target + staging install to also work. This being a pure library, would be handy to other people that might have a different use case than mine Guillaume W. Bres Software engineer <guillaume.bressaix@gmail.com> Le sam. 4 déc. 2021 à 14:46, Yann E. MORIN <yann.morin.1998@free.fr> a écrit : > Guillaume, All, > > On 2021-12-04 13:05 +0100, guillaume.bressaix@gmail.com spake thusly: > > From: "Guillaume W. Bres" <guillaume.bressaix@gmail.com> > > > > fixes > http://autobuild.buildroot.net/results/b89f1de64b308dffa73675f1f31ccb0b7be5a10d > > fixes > http://autobuild.buildroot.net/results/d0287b7f64f206b0f074908c5780a3632e0cb799 > > fixes > http://autobuild.buildroot.net/results/27efb545a5a719a5581c8f746d3a3555ff4216ce > > fixes > http://autobuild.buildroot.net/results/d2f0a0ad8f6c7178517df109e7d885dac9134c3a > > fixes > http://autobuild.buildroot.net/results/b57e9a3279260dae4a590f9421238fcabb2f7cab > > fixes > http://autobuild.buildroot.net/results/515e6f2fc6b5780260d98d6bb52b541ce4bf1afe > > fixes > http://autobuild.buildroot.net/results/d89c4ecc81222d4f80c951da2232d2e393fa1c69 > > list goes on.. > > > > --- > > v2: Reviewed by gwen@trabucayre.com, > > Removed some non needed empty lines, > > force -DFINDPYTHON=OFF when using pybind11 without python, > > handle legacy package properly in a seperate patch > > > > v1: python-pybind was not the right approach and is in failure since > > it's been upgraded to V2.6.1. > > This form here: > > > Building with setup.py now requires a cmake build first. > > With this new approach we can build the package with cmake > > for python bindings in C++ AND we also have the > > C++ bindings in python as an option (depending & requiring the first > one). > > > > I make this a host-only package, in the sense that other packages will > > require it at build time, and I don't forsee any reasons to have > > such a package as a target package. > > > > Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com> > > ... to here, should be part of the commit log, i.e. before the first > '---' line, above. Otherwise, it is dropped by git when the patch is > applied. > > However, I am still not sure what is going on here... > > First, you are removing pybind to then introduce pybind11, although they > are the exact same package upstream: > http://pybind11.readthedocs.org/en/master (home) > https://github.com/pybind/pybind11 (repo) > > But since this is the same upstream, you should just fix the existing > pybind package in-place; there is no reason to intriduce a new pacjage > to fix an existing one. > > Then, I did not find the explanations in the commit log very convincing. > Why state that "python-pybind was not the right approach"? As far as I > understand, it was working as expected until the bump to 2.6.1, and thus > was not a "failure". > > If the bump to 2.6.1 broke the package, then that means the bump was not > careful, not that the package is a failure. > > Fionally, why is a host-only package? If it installs C++ headers, then > we can expect packages built for the target to include those ehaders, > and so we need them in staging too. And this is made obvious by your > post-install hook, that uses STAGING_DIR. This is ultimately wrong, > because then that means that host packages won't be able to find/use > those headers. > > Minor nit: the commit log should not be in the first singular person, > ie.e do not use "I", but in the `neutral' first person plural, i.e. use > "we". > > However, I admit that the whole situation evades my understanding, so I > may have miss more tricky details... I will gladly accept being > corrected on those. ;-) > > Regards, > Yann E. MORIN. > > -- > > .-----------------.--------------------.------------------.--------------------. > | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' > conspiracy: | > | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ > | > | +33 561 099 427 `------------.-------: X AGAINST | \e/ There is > no | > | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v > conspiracy. | > > '------------------------------^-------^------------------^--------------------' >
On 05/12/2021 12:35, Guillaume Bres wrote: > Hello Yann, > thanks for the feedback. > > I integrated all your recommendations for my next submission, and I am currently > testing all combinations. > I will follow your suggestion and integrate this as a patch serie, so there > won't be an actual v3 follow up, > but this way you guys can see the whole picture. > > First, here's the answer to your question that is still pending: > > >But since this is the same upstream, you should just fix the existing > >pybind package in-place; there is no reason to intriduce a new pacjage > >to fix an existing one. > > This is not doable, because of the python-pkg naming convention. > The previous approach was partially fine, that is true. > It delivered C++/Py bindings (and only those) and was built as a python-pkg. I think you're conflating naming convention with package infrastructure here. The python-foo naming convention is just a name that indicates that it's some infrastructure that is strictly related to the Python language. pybind is a bit of a border case because it's about bindings between python and other languages, but Python is still pretty much central in it. It's true that it's theoretically possible to use the package without any python on the target, but is that really a use case? So maybe there are arguments to remove the python- prefix from it, but I would say the overhead of legacy handling makes it that those arguments would have to be really strong. (With overhead I mean the pain it causes people who upgrade buildroot, not so much the maintenance overhead.) The objection you're raising, however, is that the python infrastructure is not appropriate. However, it's perfectly acceptable for a python-foo package to have some other infrastructure. For example, python-gobject uses meson, python-sip uses generic (not surprisingly, both of those are also bindings packages). Bottom line: keep the name python-pybind, but change it from python-package to cmake-package. Regards, Arnout > So basically, the cmake side (Py/C++ bindings) was left out. > Now the package requires cmake whatever happens, therefore it can no longer be a > python-only package. > > >why is a host-only package? If it installs C++ headers, then > >we can expect packages built for the target to include those headers, > >and so we need them in staging too. And this is made obvious by your > >post-install hook, that uses STAGING_DIR > I am in the process of testing all the combinations, but I managed to have a > target + staging install to also work. > This being a pure library, would be handy to other people that might have a > different use case than mine > > Guillaume W. Bres > Software engineer > <guillaume.bressaix@gmail.com <mailto:guillaume.bressaix@gmail.com>> > > > Le sam. 4 déc. 2021 à 14:46, Yann E. MORIN <yann.morin.1998@free.fr > <mailto:yann.morin.1998@free.fr>> a écrit : > > Guillaume, All, > > On 2021-12-04 13:05 +0100, guillaume.bressaix@gmail.com > <mailto:guillaume.bressaix@gmail.com> spake thusly: > > From: "Guillaume W. Bres" <guillaume.bressaix@gmail.com > <mailto:guillaume.bressaix@gmail.com>> > > > > fixes > http://autobuild.buildroot.net/results/b89f1de64b308dffa73675f1f31ccb0b7be5a10d > <http://autobuild.buildroot.net/results/b89f1de64b308dffa73675f1f31ccb0b7be5a10d> > > fixes > http://autobuild.buildroot.net/results/d0287b7f64f206b0f074908c5780a3632e0cb799 > <http://autobuild.buildroot.net/results/d0287b7f64f206b0f074908c5780a3632e0cb799> > > fixes > http://autobuild.buildroot.net/results/27efb545a5a719a5581c8f746d3a3555ff4216ce > <http://autobuild.buildroot.net/results/27efb545a5a719a5581c8f746d3a3555ff4216ce> > > fixes > http://autobuild.buildroot.net/results/d2f0a0ad8f6c7178517df109e7d885dac9134c3a > <http://autobuild.buildroot.net/results/d2f0a0ad8f6c7178517df109e7d885dac9134c3a> > > fixes > http://autobuild.buildroot.net/results/b57e9a3279260dae4a590f9421238fcabb2f7cab > <http://autobuild.buildroot.net/results/b57e9a3279260dae4a590f9421238fcabb2f7cab> > > fixes > http://autobuild.buildroot.net/results/515e6f2fc6b5780260d98d6bb52b541ce4bf1afe > <http://autobuild.buildroot.net/results/515e6f2fc6b5780260d98d6bb52b541ce4bf1afe> > > fixes > http://autobuild.buildroot.net/results/d89c4ecc81222d4f80c951da2232d2e393fa1c69 > <http://autobuild.buildroot.net/results/d89c4ecc81222d4f80c951da2232d2e393fa1c69> > > list goes on.. > > > > --- > > v2: Reviewed by gwen@trabucayre.com <mailto:gwen@trabucayre.com>, > > Removed some non needed empty lines, > > force -DFINDPYTHON=OFF when using pybind11 without python, > > handle legacy package properly in a seperate patch > > > > v1: python-pybind was not the right approach and is in failure since > > it's been upgraded to V2.6.1. > > This form here: > > > Building with setup.py now requires a cmake build first. > > With this new approach we can build the package with cmake > > for python bindings in C++ AND we also have the > > C++ bindings in python as an option (depending & requiring the first one). > > > > I make this a host-only package, in the sense that other packages will > > require it at build time, and I don't forsee any reasons to have > > such a package as a target package. > > > > Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com > <mailto:guillaume.bressaix@gmail.com>> > > ... to here, should be part of the commit log, i.e. before the first > '---' line, above. Otherwise, it is dropped by git when the patch is > applied. > > However, I am still not sure what is going on here... > > First, you are removing pybind to then introduce pybind11, although they > are the exact same package upstream: > http://pybind11.readthedocs.org/en/master > <http://pybind11.readthedocs.org/en/master> (home) > https://github.com/pybind/pybind11 <https://github.com/pybind/pybind11> (repo) > > But since this is the same upstream, you should just fix the existing > pybind package in-place; there is no reason to intriduce a new pacjage > to fix an existing one. > > Then, I did not find the explanations in the commit log very convincing. > Why state that "python-pybind was not the right approach"? As far as I > understand, it was working as expected until the bump to 2.6.1, and thus > was not a "failure". > > If the bump to 2.6.1 broke the package, then that means the bump was not > careful, not that the package is a failure. > > Fionally, why is a host-only package? If it installs C++ headers, then > we can expect packages built for the target to include those ehaders, > and so we need them in staging too. And this is made obvious by your > post-install hook, that uses STAGING_DIR. This is ultimately wrong, > because then that means that host packages won't be able to find/use > those headers. > > Minor nit: the commit log should not be in the first singular person, > ie.e do not use "I", but in the `neutral' first person plural, i.e. use > "we". > > However, I admit that the whole situation evades my understanding, so I > may have miss more tricky details... I will gladly accept being > corrected on those. ;-) > > Regards, > Yann E. MORIN. > > -- > .-----------------.--------------------.------------------.--------------------. > | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | > | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | > | +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no | > | http://ymorin.is-a-geek.org/ <http://ymorin.is-a-geek.org/> | _/*\_ | / \ > HTML MAIL | v conspiracy. | > '------------------------------^-------^------------------^--------------------' > > > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot >
diff --git a/DEVELOPERS b/DEVELOPERS index 6f812eb564..8a04efa63f 100644 --- a/DEVELOPERS +++ b/DEVELOPERS @@ -1105,6 +1105,7 @@ F: package/libxcrypt/ F: package/liquid-dsp/ F: package/pixiewps/ F: package/python-pybind/ +F: package/pybind11/ F: package/reaver/ N: Guo Ren <ren_guo@c-sky.com> diff --git a/package/Config.in b/package/Config.in index 311004db2c..5749118ee3 100644 --- a/package/Config.in +++ b/package/Config.in @@ -2006,6 +2006,7 @@ endif source "package/protobuf/Config.in" source "package/protobuf-c/Config.in" source "package/protozero/Config.in" + source "package/pybind11/Config.in" source "package/qhull/Config.in" source "package/qlibc/Config.in" source "package/riemann-c-client/Config.in" diff --git a/package/pybind11/Config.in b/package/pybind11/Config.in new file mode 100644 index 0000000000..4fc6c5eebc --- /dev/null +++ b/package/pybind11/Config.in @@ -0,0 +1,25 @@ +comment "pybind11 needs a toolchain w/ C++, wchar" + depends on !BR2_INSTALL_LIBSTDCPP || !BR2_USE_WCHAR + +config BR2_PACKAGE_PYBIND11 + bool "pybind11" + depends on BR2_USE_WCHAR # boost + depends on BR2_INSTALL_LIBSTDCPP # boost + depends on !BR2_TOOLCHAIN_HAS_GCC_BUG_64735 # boost-thread + select BR2_PACKAGE_BOOST + help + Pybind11 is a lightweight header-only library that exposes C++ + types in Python and vice versa, mainly to create Python + bindings of existing C++ code. + + http://pybind11.readthedocs.org/en/master + +if BR2_PACKAGE_PYBIND11 + +config BR2_PACKAGE_PYBIND11_WITH_PYTHON + bool "pybind11-python" + depends on BR2_PACKAGE_PYTHON3 + help + Activate support for python + +endif diff --git a/package/pybind11/pybind11.hash b/package/pybind11/pybind11.hash new file mode 100644 index 0000000000..ab8825bf04 --- /dev/null +++ b/package/pybind11/pybind11.hash @@ -0,0 +1,3 @@ +# Locally calculated +sha256 f1bcc07caa568eb312411dde5308b1e250bd0e1bc020fae855bf9f43209940cc pybind11-2.8.1.tar.gz +sha256 83965b843b98f670d3a85bd041ed4b372c8ec50d7b4a5995a83ac697ba675dcb LICENSE diff --git a/package/pybind11/pybind11.mk b/package/pybind11/pybind11.mk new file mode 100644 index 0000000000..a67ce237ea --- /dev/null +++ b/package/pybind11/pybind11.mk @@ -0,0 +1,41 @@ +################################################################################ +# +# pybind11 +# +################################################################################ + +PYBIND11_VERSION = 2.8.1 +PYBIND11_SITE = $(call github,pybind,pybind11,v$(PYBIND11_VERSION)) +PYBIND11_LICENSE = BSD-3-Clause +PYBIND11_LICENSE_FILES = LICENSE +PYBIND11_INSTALL_STAGING = YES +PYBIND11_SUPPORTS_IN_SOURCE_BUILD = YES + +HOST_PYBIND11_CONF_OPTS = \ + -DBUILD_DOCS=OFF \ + -DDOWNLOAD_EIGEN=OFF + +# pybind11 python support activation +ifeq ($(BR2_PACKAGE_PYBIND11_WITH_PYTHON),y) +HOST_PYBIND11_DEPENDENCIES += host-python3 + +# pybind11 with python requires cmake install in $(@D) +HOST_PYBIND11_CONF_OPTS += \ + -DCMAKE_INSTALL_PREFIX=$(@D)/pybind11 \ + -DPYTHON=$(HOST_DIR)/bin/python3 \ + -DPYTHON_PREFIX=$(STAGING_DIR)/usr \ + -DPYBIND_FINDPYTHON=ON \ + -DPYBIND11_NOPYTHON=OFF + +define PYBIND11_PYTHON_BUILD + cd $(@D) && $(HOST_DIR)/bin/python setup.py install +endef + +HOST_PYBIND11_POST_INSTALL_HOOKS += PYBIND11_PYTHON_BUILD +else +HOST_PYBIND11_CONF_OPTS += \ + -DPYBIND_FINDPYTHON=OFF \ + -DPYBIND11_NOPYTHON=ON +endif + +$(eval $(host-cmake-package))