| Message ID | 20211206193109.21218-1-guillaume.bressaix@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] package/python-pybind: fix upgrade to version 2.6.1 | expand |
Guillaume, All, On 2021-12-06 20:31 +0100, guillaume.bressaix@gmail.com spake thusly: > From: "Guillaume W. Bres" <guillaume.bressaix@gmail.com> > > The python 'setup.py' script needs header files > in $(@D)/pybind11 to work since v2.6.1, and these > files are generated by an internal minimalist cmake build. > > Fixes > http://autobuild.buildroot.net/results/b89f1de64b308dffa73675f1f31ccb0b7be5a10d > http://autobuild.buildroot.net/results/d0287b7f64f206b0f074908c5780a3632e0cb799 > http://autobuild.buildroot.net/results/27efb545a5a719a5581c8f746d3a3555ff4216ce > http://autobuild.buildroot.net/results/d2f0a0ad8f6c7178517df109e7d885dac9134c3a > http://autobuild.buildroot.net/results/b57e9a3279260dae4a590f9421238fcabb2f7cab > http://autobuild.buildroot.net/results/515e6f2fc6b5780260d98d6bb52b541ce4bf1afe > http://autobuild.buildroot.net/results/d89c4ecc81222d4f80c951da2232d2e393fa1c69 > > Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com> > > --- > This remains a python-package. > > 'python setup.py' actually hardcodes a system call to cmake now, > which seems dirty at first. But if we have a cmake env when they > do that, the install variables are passed and installation > is smooth. They probably introduced this call to make setup.py > self sufficient, as most people install this package from 'pip'. > > --- > package/python-pybind/python-pybind.mk | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/package/python-pybind/python-pybind.mk b/package/python-pybind/python-pybind.mk > index a6a1bdb976..bfd7f6f59a 100644 > --- a/package/python-pybind/python-pybind.mk > +++ b/package/python-pybind/python-pybind.mk > @@ -9,5 +9,12 @@ PYTHON_PYBIND_SITE = $(call github,pybind,pybind11,v$(PYTHON_PYBIND_VERSION)) > PYTHON_PYBIND_LICENSE = BSD-3-Clause > PYTHON_PYBIND_LICENSE_FILES = LICENSE > PYTHON_PYBIND_SETUP_TYPE = setuptools > +PYTHON_PYBIND_INSTALL_STAGING = YES > + > +# every single 'python setup.py' call actually calls cmake > +# internally, to populate $(@D)/pybind11 with build requirements. > +# If we have a host-cmake env at that moment, > +# then the final installation paths are properly defined > +PYTHON_PYBIND_DEPENDENCIES = host-cmake We do not hard-code a dependency to host-cmake, in case the system cmake is sufficient. Instead, we use $(BR2_CMAKE_HOST_DEPENDENCY) which is set appropriately. Regards, Yann E. MORIN. > $(eval $(python-package)) > -- > 2.20.1 >
Guillaume, All, On 2021-12-06 20:31 +0100, guillaume.bressaix@gmail.com spake thusly: > From: "Guillaume W. Bres" <guillaume.bressaix@gmail.com> > > The python 'setup.py' script needs header files > in $(@D)/pybind11 to work since v2.6.1, and these > files are generated by an internal minimalist cmake build. > > Fixes > http://autobuild.buildroot.net/results/b89f1de64b308dffa73675f1f31ccb0b7be5a10d > http://autobuild.buildroot.net/results/d0287b7f64f206b0f074908c5780a3632e0cb799 > http://autobuild.buildroot.net/results/27efb545a5a719a5581c8f746d3a3555ff4216ce > http://autobuild.buildroot.net/results/d2f0a0ad8f6c7178517df109e7d885dac9134c3a > http://autobuild.buildroot.net/results/b57e9a3279260dae4a590f9421238fcabb2f7cab > http://autobuild.buildroot.net/results/515e6f2fc6b5780260d98d6bb52b541ce4bf1afe > http://autobuild.buildroot.net/results/d89c4ecc81222d4f80c951da2232d2e393fa1c69 > > Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com> > > --- > This remains a python-package. > > 'python setup.py' actually hardcodes a system call to cmake now, > which seems dirty at first. But if we have a cmake env when they > do that, the install variables are passed and installation > is smooth. They probably introduced this call to make setup.py > self sufficient, as most people install this package from 'pip'. > > --- > package/python-pybind/python-pybind.mk | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/package/python-pybind/python-pybind.mk b/package/python-pybind/python-pybind.mk > index a6a1bdb976..bfd7f6f59a 100644 > --- a/package/python-pybind/python-pybind.mk > +++ b/package/python-pybind/python-pybind.mk > @@ -9,5 +9,12 @@ PYTHON_PYBIND_SITE = $(call github,pybind,pybind11,v$(PYTHON_PYBIND_VERSION)) > PYTHON_PYBIND_LICENSE = BSD-3-Clause > PYTHON_PYBIND_LICENSE_FILES = LICENSE > PYTHON_PYBIND_SETUP_TYPE = setuptools > +PYTHON_PYBIND_INSTALL_STAGING = YES > + > +# every single 'python setup.py' call actually calls cmake > +# internally, to populate $(@D)/pybind11 with build requirements. > +# If we have a host-cmake env at that moment, > +# then the final installation paths are properly defined > +PYTHON_PYBIND_DEPENDENCIES = host-cmake I am sorry, but I fail to see how this actually fixes things. As I explained in my previous review, headers are supposed to be in $(STAGING_DIR)/usr/include/pybind11/. That's at least where they are in Debian-based distributions; $ apt-file search 'pybind11/pybind11.h' pybind11-dev: /usr/include/pybind11/pybind11.h However, with the way pybind is currently packaged, the ehaders end up in: $ sort output/build/python-pybind-2.8.1/.files-list.txt python-pybind,./usr/bin/pybind11-config [...] python-pybind,./usr/lib/python3.9/site-packages/pybind11/include/pybind11/pybind11.h [...] So, those headers will most probably not be found by the compiler. Also, since pybind is supposed to be a headers'-only library, there is nothing supposed to go in target, so it should be; PYTHON_PYBIND_INSTALL_TARGET = NO Furthermore, /usr/bin/pybind11-config is a python script which hardcodes its shabang to #!/usr/bin/python, and this is incorrect: 1. /usr/bin/python might be python2 or python3, we can't know 2. anyway, it would not use our host python, so it would miss the proper modules search paths. So, if another package want to call that /usr/bin/pybind11-config to find pybind, they're gonna have incorrect results, if at all... So I think we really, really need a package (like scipy) that actually makes use of pybind, so we can see how it works, and to fix it... Regards, Yann E. MORIN. > $(eval $(python-package)) > -- > 2.20.1 >
On 06/12/2021 22:57, Yann E. MORIN wrote: > Guillaume, All, > > On 2021-12-06 20:31 +0100, guillaume.bressaix@gmail.com spake thusly: >> From: "Guillaume W. Bres" <guillaume.bressaix@gmail.com> >> >> The python 'setup.py' script needs header files >> in $(@D)/pybind11 to work since v2.6.1, and these >> files are generated by an internal minimalist cmake build. >> >> Fixes >> http://autobuild.buildroot.net/results/b89f1de64b308dffa73675f1f31ccb0b7be5a10d >> http://autobuild.buildroot.net/results/d0287b7f64f206b0f074908c5780a3632e0cb799 >> http://autobuild.buildroot.net/results/27efb545a5a719a5581c8f746d3a3555ff4216ce >> http://autobuild.buildroot.net/results/d2f0a0ad8f6c7178517df109e7d885dac9134c3a >> http://autobuild.buildroot.net/results/b57e9a3279260dae4a590f9421238fcabb2f7cab >> http://autobuild.buildroot.net/results/515e6f2fc6b5780260d98d6bb52b541ce4bf1afe >> http://autobuild.buildroot.net/results/d89c4ecc81222d4f80c951da2232d2e393fa1c69 >> >> Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com> >> >> --- >> This remains a python-package. >> >> 'python setup.py' actually hardcodes a system call to cmake now, >> which seems dirty at first. But if we have a cmake env when they >> do that, the install variables are passed and installation >> is smooth. They probably introduced this call to make setup.py >> self sufficient, as most people install this package from 'pip'. >> >> --- >> package/python-pybind/python-pybind.mk | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/package/python-pybind/python-pybind.mk b/package/python-pybind/python-pybind.mk >> index a6a1bdb976..bfd7f6f59a 100644 >> --- a/package/python-pybind/python-pybind.mk >> +++ b/package/python-pybind/python-pybind.mk >> @@ -9,5 +9,12 @@ PYTHON_PYBIND_SITE = $(call github,pybind,pybind11,v$(PYTHON_PYBIND_VERSION)) >> PYTHON_PYBIND_LICENSE = BSD-3-Clause >> PYTHON_PYBIND_LICENSE_FILES = LICENSE >> PYTHON_PYBIND_SETUP_TYPE = setuptools >> +PYTHON_PYBIND_INSTALL_STAGING = YES >> + >> +# every single 'python setup.py' call actually calls cmake >> +# internally, to populate $(@D)/pybind11 with build requirements. >> +# If we have a host-cmake env at that moment, >> +# then the final installation paths are properly defined >> +PYTHON_PYBIND_DEPENDENCIES = host-cmake > > I am sorry, but I fail to see how this actually fixes things. > > As I explained in my previous review, headers are supposed to be in > $(STAGING_DIR)/usr/include/pybind11/. That's at least where they are in > Debian-based distributions; > > $ apt-file search 'pybind11/pybind11.h' > pybind11-dev: /usr/include/pybind11/pybind11.h > > However, with the way pybind is currently packaged, the ehaders end up > in: > > $ sort output/build/python-pybind-2.8.1/.files-list.txt > python-pybind,./usr/bin/pybind11-config > [...] > python-pybind,./usr/lib/python3.9/site-packages/pybind11/include/pybind11/pybind11.h > [...] > > So, those headers will most probably not be found by the compiler. > > Also, since pybind is supposed to be a headers'-only library, there is > nothing supposed to go in target, so it should be; > > PYTHON_PYBIND_INSTALL_TARGET = NO Note that this is only relevant to make sure that pybind11-config doesn't appear on target. The headers themselves are removed automatically by Buildroot. > > Furthermore, /usr/bin/pybind11-config is a python script which hardcodes > its shabang to #!/usr/bin/python, and this is incorrect: > > 1. /usr/bin/python might be python2 or python3, we can't know > 2. anyway, it would not use our host python, so it would miss the > proper modules search paths. Yeah, this is something that needs to be fixed - in particular, there may not be a /usr/bin/python at all. However, I think that is kind of orthogonal to this patch. This patch fixes a build failure of pybind itself. Your proposed change would be to fix the build of a dependency of pybind. > So, if another package want to call that /usr/bin/pybind11-config to > find pybind, they're gonna have incorrect results, if at all... > > So I think we really, really need a package (like scipy) that actually > makes use of pybind, so we can see how it works, and to fix it... Note that there are already two [1][2] submissions of scipy in patchwork, so feel free to review those. Note also that scipy depends on host-pybind, not pybind itself. I'm not sure how *that* is supposed to work - although, being header-only, it probably doesn't really matter if you link with the host or the target version of the library, and the host-installed pybind11-config makes sure that you pick up the host headers (which are normally not in include path). So conceptually wrong but works in practice. In conclusion: I think this patch is going in the right direction (modulo BR2_CMAKE_HOST_DEPENDENCY). It's still "conceptually wrong but works" because it doesn't set the toolchain file and other cmake options, but that happens to work because it's header-only so it doesn't actually need any of the cmake options. So it's not entirely the correct thing to do, but it does guarantee to fix things. I'm not sure if that means we should merge it or not... The proper fix, I think, would require patching setup.py so we can pass in additional cmake options to it. Regards, Arnout [1] https://patchwork.ozlabs.org/project/buildroot/list/?series=168992 [2] https://patchwork.ozlabs.org/project/buildroot/list/?series=186146 > > Regards, > Yann E. MORIN. > >> $(eval $(python-package)) >> -- >> 2.20.1 >> >
On 06/12/2021 22:45, Yann E. MORIN wrote: > Guillaume, All, > > On 2021-12-06 20:31 +0100, guillaume.bressaix@gmail.com spake thusly: >> From: "Guillaume W. Bres" <guillaume.bressaix@gmail.com> >> >> The python 'setup.py' script needs header files >> in $(@D)/pybind11 to work since v2.6.1, and these >> files are generated by an internal minimalist cmake build. >> >> Fixes >> http://autobuild.buildroot.net/results/b89f1de64b308dffa73675f1f31ccb0b7be5a10d >> http://autobuild.buildroot.net/results/d0287b7f64f206b0f074908c5780a3632e0cb799 >> http://autobuild.buildroot.net/results/27efb545a5a719a5581c8f746d3a3555ff4216ce >> http://autobuild.buildroot.net/results/d2f0a0ad8f6c7178517df109e7d885dac9134c3a >> http://autobuild.buildroot.net/results/b57e9a3279260dae4a590f9421238fcabb2f7cab >> http://autobuild.buildroot.net/results/515e6f2fc6b5780260d98d6bb52b541ce4bf1afe >> http://autobuild.buildroot.net/results/d89c4ecc81222d4f80c951da2232d2e393fa1c69 >> >> Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com> >> >> --- >> This remains a python-package. >> >> 'python setup.py' actually hardcodes a system call to cmake now, >> which seems dirty at first. But if we have a cmake env when they >> do that, the install variables are passed and installation >> is smooth. They probably introduced this call to make setup.py >> self sufficient, as most people install this package from 'pip'. >> >> --- >> package/python-pybind/python-pybind.mk | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/package/python-pybind/python-pybind.mk b/package/python-pybind/python-pybind.mk >> index a6a1bdb976..bfd7f6f59a 100644 >> --- a/package/python-pybind/python-pybind.mk >> +++ b/package/python-pybind/python-pybind.mk >> @@ -9,5 +9,12 @@ PYTHON_PYBIND_SITE = $(call github,pybind,pybind11,v$(PYTHON_PYBIND_VERSION)) >> PYTHON_PYBIND_LICENSE = BSD-3-Clause >> PYTHON_PYBIND_LICENSE_FILES = LICENSE >> PYTHON_PYBIND_SETUP_TYPE = setuptools >> +PYTHON_PYBIND_INSTALL_STAGING = YES >> + >> +# every single 'python setup.py' call actually calls cmake >> +# internally, to populate $(@D)/pybind11 with build requirements. >> +# If we have a host-cmake env at that moment, >> +# then the final installation paths are properly defined >> +PYTHON_PYBIND_DEPENDENCIES = host-cmake > > We do not hard-code a dependency to host-cmake, in case the system cmake > is sufficient. Instead, we use $(BR2_CMAKE_HOST_DEPENDENCY) which is set > appropriately. This turns out not to work in Guillaume's test setup. It turns out that he has a preinstalled cmake which is very old, but also cmake3 which is recent enough. Therefore, BR2_CMAKE is set to /usr/bin/cmake3 and BR2_CMAKE_HOST_DEPENDENCY is empty. However, setup.py hardcodes "cmake". So, possible solutions: - Forget about setup.py and instead hack it into a cmake-package. Not ideal from a maintenance perspective, because later updates may have changes to setup.py that we need to replicate. - Patch setup.py so we can tell it to use BR2_CMAKE instead of cmake-in-path. This is nice because we can use the occasion to also pass in the rest of the cmake options for proper cross-compilation. - In host-skeleton, symlink BR2_CMAKE to host/bin/cmake, so that will be the 'cmake' that is found in $PATH. This has the advantage of also covering other potential cases of the same problem, but I feel it's a bit meh. So I have a slight preference for the second option, but Yann you may have other ideas. Regards, Arnout
Hi all, On 07.12.21 09:04, Arnout Vandecappelle wrote: > > > On 06/12/2021 22:57, Yann E. MORIN wrote: >> Guillaume, All, >> >> On 2021-12-06 20:31 +0100, guillaume.bressaix@gmail.com spake thusly: >>> From: "Guillaume W. Bres" <guillaume.bressaix@gmail.com> >>> >>> The python 'setup.py' script needs header files >>> in $(@D)/pybind11 to work since v2.6.1, and these >>> files are generated by an internal minimalist cmake build. >>> >>> Fixes >>> >>> http://autobuild.buildroot.net/results/b89f1de64b308dffa73675f1f31ccb0b7be5a10d >>> >>> >>> http://autobuild.buildroot.net/results/d0287b7f64f206b0f074908c5780a3632e0cb799 >>> >>> >>> http://autobuild.buildroot.net/results/27efb545a5a719a5581c8f746d3a3555ff4216ce >>> >>> >>> http://autobuild.buildroot.net/results/d2f0a0ad8f6c7178517df109e7d885dac9134c3a >>> >>> >>> http://autobuild.buildroot.net/results/b57e9a3279260dae4a590f9421238fcabb2f7cab >>> >>> >>> http://autobuild.buildroot.net/results/515e6f2fc6b5780260d98d6bb52b541ce4bf1afe >>> >>> >>> http://autobuild.buildroot.net/results/d89c4ecc81222d4f80c951da2232d2e393fa1c69 >>> >>> >>> Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com> >>> >>> --- >>> This remains a python-package. >>> >>> 'python setup.py' actually hardcodes a system call to cmake now, >>> which seems dirty at first. But if we have a cmake env when they >>> do that, the install variables are passed and installation >>> is smooth. They probably introduced this call to make setup.py >>> self sufficient, as most people install this package from 'pip'. >>> >>> --- >>> package/python-pybind/python-pybind.mk | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/package/python-pybind/python-pybind.mk >>> b/package/python-pybind/python-pybind.mk >>> index a6a1bdb976..bfd7f6f59a 100644 >>> --- a/package/python-pybind/python-pybind.mk >>> +++ b/package/python-pybind/python-pybind.mk >>> @@ -9,5 +9,12 @@ PYTHON_PYBIND_SITE = $(call >>> github,pybind,pybind11,v$(PYTHON_PYBIND_VERSION)) >>> PYTHON_PYBIND_LICENSE = BSD-3-Clause >>> PYTHON_PYBIND_LICENSE_FILES = LICENSE >>> PYTHON_PYBIND_SETUP_TYPE = setuptools >>> +PYTHON_PYBIND_INSTALL_STAGING = YES >>> + >>> +# every single 'python setup.py' call actually calls cmake >>> +# internally, to populate $(@D)/pybind11 with build requirements. >>> +# If we have a host-cmake env at that moment, >>> +# then the final installation paths are properly defined >>> +PYTHON_PYBIND_DEPENDENCIES = host-cmake >> >> I am sorry, but I fail to see how this actually fixes things. >> >> As I explained in my previous review, headers are supposed to be in >> $(STAGING_DIR)/usr/include/pybind11/. That's at least where they are in >> Debian-based distributions; >> >> $ apt-file search 'pybind11/pybind11.h' >> pybind11-dev: /usr/include/pybind11/pybind11.h >> >> However, with the way pybind is currently packaged, the ehaders end up >> in: >> >> $ sort output/build/python-pybind-2.8.1/.files-list.txt >> python-pybind,./usr/bin/pybind11-config >> [...] >> >> python-pybind,./usr/lib/python3.9/site-packages/pybind11/include/pybind11/pybind11.h >> >> [...] >> >> So, those headers will most probably not be found by the compiler. >> >> Also, since pybind is supposed to be a headers'-only library, there is >> nothing supposed to go in target, so it should be; >> >> PYTHON_PYBIND_INSTALL_TARGET = NO > > Note that this is only relevant to make sure that pybind11-config > doesn't appear on target. The headers themselves are removed > automatically by Buildroot. > >> >> Furthermore, /usr/bin/pybind11-config is a python script which hardcodes >> its shabang to #!/usr/bin/python, and this is incorrect: >> >> 1. /usr/bin/python might be python2 or python3, we can't know >> 2. anyway, it would not use our host python, so it would miss the >> proper modules search paths. > > Yeah, this is something that needs to be fixed - in particular, there > may not be a /usr/bin/python at all. > > However, I think that is kind of orthogonal to this patch. This patch > fixes a build failure of pybind itself. Your proposed change would be to > fix the build of a dependency of pybind. > > >> So, if another package want to call that /usr/bin/pybind11-config to >> find pybind, they're gonna have incorrect results, if at all... >> >> So I think we really, really need a package (like scipy) that actually >> makes use of pybind, so we can see how it works, and to fix it... By chance I was playing with such a package just yesterday: zxing-cpp. Not the version buildroot uses (from github glassechidna), but the nu-book one which claims to be faster. (And is the one where the python bindings in the PyPi package zxing-cpp originate from). Until recently they shipped their own pybind11 version, which throws an error when included in a cmake project in a crosscompile environment. This is a known problem, https://github.com/pybind/pybind11/issues/2139 but non of the proposed fixes has been merged yet. However, the unreleased master version of zxing-cpp hast been changed so it can be configured to use the a system pybind11. So I tried to include the br packaged one (2.6.1 as well as 2.8.1). They both built on my system (no problem with the hardcoded host cmake here) but I needed to change three things: 1. Add PYTHON_PYBIND_INSTALL_STAGING = YES 2. Set ZXING_CPP_CONF_OPTS += -Dpybind11_DIR=$(STAGING_DIR)/usr/lib/python$(PYTHON3_VERSION_MAJOR)/site-packages/pybind11/share/cmake/pybind11 3. Apply one of the proposed fixes for the pybind11 crosscompile problem (to tools/pybind11Tools.cmake) So this patch here seems to help my (and possibly others) because 1. is already included. If interested, I could send a patch for 3. > > Note that there are already two [1][2] submissions of scipy in > patchwork, so feel free to review those. > > Note also that scipy depends on host-pybind, not pybind itself. I'm > not sure how *that* is supposed to work - although, being header-only, One assumption why scipy includes the host version of pybind is that it nicely avoids the problems where I need fix 1. and 3. for. > it probably doesn't really matter if you link with the host or the > target version of the library, and the host-installed pybind11-config > makes sure that you pick up the host headers (which are normally not in > include path). So conceptually wrong but works in practice. For getting conceptually right with my zxing-cpp I would appreciate some advice: When compiling it with above modificiations and BUILD_PYTHON_MODULE=ON, cmake happily compiles the zxing pybind wrapper. But of course wrappers/python/setup.py would need to be run as well. However mixing $(eval $(python-package)) and $(eval $(cmake-package)) is not possible. So do I still need to create a second package python-zxingcpp for the bindings only (which might be a pain because the wrappers CMakeLists is not written to use an existant installation of zxing-cpp but rather downloads it again) ? regards, Andreas > > > In conclusion: I think this patch is going in the right direction > (modulo BR2_CMAKE_HOST_DEPENDENCY). It's still "conceptually wrong but > works" because it doesn't set the toolchain file and other cmake > options, but that happens to work because it's header-only so it doesn't > actually need any of the cmake options. So it's not entirely the correct > thing to do, but it does guarantee to fix things. I'm not sure if that > means we should merge it or not... > > The proper fix, I think, would require patching setup.py so we can > pass in additional cmake options to it. > > > Regards, > Arnout > > [1] https://patchwork.ozlabs.org/project/buildroot/list/?series=168992 > [2] https://patchwork.ozlabs.org/project/buildroot/list/?series=186146 So > >> >> Regards, >> Yann E. MORIN. >> >>> $(eval $(python-package)) >>> -- >>> 2.20.1 >>> >> > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot >
Arnout, I also prefer your second option. I need to run some more tests & inquire what Yann proposed on IRC. This might take a couple of days, I am not submitting until I reach a solution, but will keep in touch on IRC. At least I understand what is going on in all scenarios now Andreas, >They both built on my >but I needed to change three things: >1. Add PYTHON_PYBIND_INSTALL_STAGING = YES >2. Set ZXING_CPP_CONF_OPTS += >-Dpybind11_DIR=$(STAGING_DIR)/usr/lib/python$(PYTHON3_VERSION_MAJOR)/site-packages/pybind11/share/cmake/pybind11 >3. Apply one of the proposed fixes for the pybind11 crosscompile problem >(to tools/pybind11Tools.cmake) Once I come to a solution, my next contribution will allow to install pybind to staging. At that time we should have a solution good enough so other packages will no longer have to internally ship pybind. >Apply one of the proposed fixes for the pybind11 crosscompile problem (to tools/pybind11Tools.cmake) We should keep an eye on the pybind devs topics you pointed out. We might have a near future solution, but it's probably going to need to involve (hopefuly sanitized/simplified) in also a near future Guillaume W. Bres Software engineer <guillaume.bressaix@gmail.com> Le mer. 8 déc. 2021 à 12:19, Andreas Naumann <dev@andin.de> a écrit : > Hi all, > > On 07.12.21 09:04, Arnout Vandecappelle wrote: > > > > > > On 06/12/2021 22:57, Yann E. MORIN wrote: > >> Guillaume, All, > >> > >> On 2021-12-06 20:31 +0100, guillaume.bressaix@gmail.com spake thusly: > >>> From: "Guillaume W. Bres" <guillaume.bressaix@gmail.com> > >>> > >>> The python 'setup.py' script needs header files > >>> in $(@D)/pybind11 to work since v2.6.1, and these > >>> files are generated by an internal minimalist cmake build. > >>> > >>> Fixes > >>> > >>> > http://autobuild.buildroot.net/results/b89f1de64b308dffa73675f1f31ccb0b7be5a10d > >>> > >>> > >>> > http://autobuild.buildroot.net/results/d0287b7f64f206b0f074908c5780a3632e0cb799 > >>> > >>> > >>> > http://autobuild.buildroot.net/results/27efb545a5a719a5581c8f746d3a3555ff4216ce > >>> > >>> > >>> > http://autobuild.buildroot.net/results/d2f0a0ad8f6c7178517df109e7d885dac9134c3a > >>> > >>> > >>> > http://autobuild.buildroot.net/results/b57e9a3279260dae4a590f9421238fcabb2f7cab > >>> > >>> > >>> > http://autobuild.buildroot.net/results/515e6f2fc6b5780260d98d6bb52b541ce4bf1afe > >>> > >>> > >>> > http://autobuild.buildroot.net/results/d89c4ecc81222d4f80c951da2232d2e393fa1c69 > >>> > >>> > >>> Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com> > >>> > >>> --- > >>> This remains a python-package. > >>> > >>> 'python setup.py' actually hardcodes a system call to cmake now, > >>> which seems dirty at first. But if we have a cmake env when they > >>> do that, the install variables are passed and installation > >>> is smooth. They probably introduced this call to make setup.py > >>> self sufficient, as most people install this package from 'pip'. > >>> > >>> --- > >>> package/python-pybind/python-pybind.mk | 7 +++++++ > >>> 1 file changed, 7 insertions(+) > >>> > >>> diff --git a/package/python-pybind/python-pybind.mk > >>> b/package/python-pybind/python-pybind.mk > >>> index a6a1bdb976..bfd7f6f59a 100644 > >>> --- a/package/python-pybind/python-pybind.mk > >>> +++ b/package/python-pybind/python-pybind.mk > >>> @@ -9,5 +9,12 @@ PYTHON_PYBIND_SITE = $(call > >>> github,pybind,pybind11,v$(PYTHON_PYBIND_VERSION)) > >>> PYTHON_PYBIND_LICENSE = BSD-3-Clause > >>> PYTHON_PYBIND_LICENSE_FILES = LICENSE > >>> PYTHON_PYBIND_SETUP_TYPE = setuptools > >>> +PYTHON_PYBIND_INSTALL_STAGING = YES > >>> + > >>> +# every single 'python setup.py' call actually calls cmake > >>> +# internally, to populate $(@D)/pybind11 with build requirements. > >>> +# If we have a host-cmake env at that moment, > >>> +# then the final installation paths are properly defined > >>> +PYTHON_PYBIND_DEPENDENCIES = host-cmake > >> > >> I am sorry, but I fail to see how this actually fixes things. > >> > >> As I explained in my previous review, headers are supposed to be in > >> $(STAGING_DIR)/usr/include/pybind11/. That's at least where they are in > >> Debian-based distributions; > >> > >> $ apt-file search 'pybind11/pybind11.h' > >> pybind11-dev: /usr/include/pybind11/pybind11.h > >> > >> However, with the way pybind is currently packaged, the ehaders end up > >> in: > >> > >> $ sort output/build/python-pybind-2.8.1/.files-list.txt > >> python-pybind,./usr/bin/pybind11-config > >> [...] > >> > >> > python-pybind,./usr/lib/python3.9/site-packages/pybind11/include/pybind11/pybind11.h > > >> > >> [...] > >> > >> So, those headers will most probably not be found by the compiler. > >> > >> Also, since pybind is supposed to be a headers'-only library, there is > >> nothing supposed to go in target, so it should be; > >> > >> PYTHON_PYBIND_INSTALL_TARGET = NO > > > > Note that this is only relevant to make sure that pybind11-config > > doesn't appear on target. The headers themselves are removed > > automatically by Buildroot. > > > >> > >> Furthermore, /usr/bin/pybind11-config is a python script which hardcodes > >> its shabang to #!/usr/bin/python, and this is incorrect: > >> > >> 1. /usr/bin/python might be python2 or python3, we can't know > >> 2. anyway, it would not use our host python, so it would miss the > >> proper modules search paths. > > > > Yeah, this is something that needs to be fixed - in particular, there > > may not be a /usr/bin/python at all. > > > > However, I think that is kind of orthogonal to this patch. This patch > > fixes a build failure of pybind itself. Your proposed change would be to > > fix the build of a dependency of pybind. > > > > > >> So, if another package want to call that /usr/bin/pybind11-config to > >> find pybind, they're gonna have incorrect results, if at all... > >> > >> So I think we really, really need a package (like scipy) that actually > >> makes use of pybind, so we can see how it works, and to fix it... > > By chance I was playing with such a package just yesterday: zxing-cpp. > Not the version buildroot uses (from github glassechidna), but the > nu-book one which claims to be faster. (And is the one where the python > bindings in the PyPi package zxing-cpp originate from). > > Until recently they shipped their own pybind11 version, which throws an > error when included in a cmake project in a crosscompile environment. > This is a known problem, https://github.com/pybind/pybind11/issues/2139 > but non of the proposed fixes has been merged yet. > > However, the unreleased master version of zxing-cpp hast been changed so > it can be configured to use the a system pybind11. So I tried to include > the br packaged one (2.6.1 as well as 2.8.1). They both built on my > system (no problem with the hardcoded host cmake here) but I needed to > change three things: > 1. Add PYTHON_PYBIND_INSTALL_STAGING = YES > 2. Set ZXING_CPP_CONF_OPTS += > > -Dpybind11_DIR=$(STAGING_DIR)/usr/lib/python$(PYTHON3_VERSION_MAJOR)/site-packages/pybind11/share/cmake/pybind11 > 3. Apply one of the proposed fixes for the pybind11 crosscompile problem > (to tools/pybind11Tools.cmake) > > So this patch here seems to help my (and possibly others) because 1. is > already included. If interested, I could send a patch for 3. > > > > > > Note that there are already two [1][2] submissions of scipy in > > patchwork, so feel free to review those. > > > > Note also that scipy depends on host-pybind, not pybind itself. I'm > > not sure how *that* is supposed to work - although, being header-only, > > > One assumption why scipy includes the host version of pybind is that it > nicely avoids the problems where I need fix 1. and 3. for. > > > it probably doesn't really matter if you link with the host or the > > target version of the library, and the host-installed pybind11-config > > makes sure that you pick up the host headers (which are normally not in > > include path). So conceptually wrong but works in practice. > > For getting conceptually right with my zxing-cpp I would appreciate some > advice: When compiling it with above modificiations and > BUILD_PYTHON_MODULE=ON, cmake happily compiles the zxing pybind wrapper. > But of course wrappers/python/setup.py would need to be run as well. > However mixing $(eval $(python-package)) and $(eval $(cmake-package)) is > not possible. So do I still need to create a second package > python-zxingcpp for the bindings only (which might be a pain because the > wrappers CMakeLists is not written to use an existant installation of > zxing-cpp but rather downloads it again) ? > > > regards, > Andreas > > > > > > > > > In conclusion: I think this patch is going in the right direction > > (modulo BR2_CMAKE_HOST_DEPENDENCY). It's still "conceptually wrong but > > works" because it doesn't set the toolchain file and other cmake > > options, but that happens to work because it's header-only so it doesn't > > actually need any of the cmake options. So it's not entirely the correct > > thing to do, but it does guarantee to fix things. I'm not sure if that > > means we should merge it or not... > > > > The proper fix, I think, would require patching setup.py so we can > > pass in additional cmake options to it. > > > > > > Regards, > > Arnout > > > > [1] https://patchwork.ozlabs.org/project/buildroot/list/?series=168992 > > [2] https://patchwork.ozlabs.org/project/buildroot/list/?series=186146 > > So > > > > >> > >> Regards, > >> Yann E. MORIN. > >> > >>> $(eval $(python-package)) > >>> -- > >>> 2.20.1 > >>> > >> > > _______________________________________________ > > buildroot mailing list > > buildroot@buildroot.org > > https://lists.buildroot.org/mailman/listinfo/buildroot > > >
Arnout, All, On 2021-12-07 09:04 +0100, Arnout Vandecappelle spake thusly: > On 06/12/2021 22:57, Yann E. MORIN wrote: > >Guillaume, All, > > > >On 2021-12-06 20:31 +0100, guillaume.bressaix@gmail.com spake thusly: > >>From: "Guillaume W. Bres" <guillaume.bressaix@gmail.com> > >> > >>The python 'setup.py' script needs header files > >>in $(@D)/pybind11 to work since v2.6.1, and these > >>files are generated by an internal minimalist cmake build. > >> > >>Fixes > >> http://autobuild.buildroot.net/results/b89f1de64b308dffa73675f1f31ccb0b7be5a10d > >> http://autobuild.buildroot.net/results/d0287b7f64f206b0f074908c5780a3632e0cb799 > >> http://autobuild.buildroot.net/results/27efb545a5a719a5581c8f746d3a3555ff4216ce > >> http://autobuild.buildroot.net/results/d2f0a0ad8f6c7178517df109e7d885dac9134c3a > >> http://autobuild.buildroot.net/results/b57e9a3279260dae4a590f9421238fcabb2f7cab > >> http://autobuild.buildroot.net/results/515e6f2fc6b5780260d98d6bb52b541ce4bf1afe > >> http://autobuild.buildroot.net/results/d89c4ecc81222d4f80c951da2232d2e393fa1c69 > >> > >>Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com> > >I am sorry, but I fail to see how this actually fixes things. [--SNIP--] > >Also, since pybind is supposed to be a headers'-only library, there is > >nothing supposed to go in target, so it should be; > > PYTHON_PYBIND_INSTALL_TARGET = NO > Note that this is only relevant to make sure that pybind11-config doesn't > appear on target. The headers themselves are removed automatically by > Buildroot. It also installs a buncha stuff in /usr/lib/python3.9/site-packages/pybind11/ which are totally useless because that needed only for the packaging of pybind11, not at runtime (as far as I could understand). > >Furthermore, /usr/bin/pybind11-config is a python script which hardcodes > >its shabang to #!/usr/bin/python, and this is incorrect: > > > > 1. /usr/bin/python might be python2 or python3, we can't know > > 2. anyway, it would not use our host python, so it would miss the > > proper modules search paths. > > Yeah, this is something that needs to be fixed - in particular, there may > not be a /usr/bin/python at all. > > However, I think that is kind of orthogonal to this patch. This patch fixes > a build failure of pybind itself. Your proposed change would be to fix the > build of a dependency of pybind. Well, we can also easily fix it with just this: PYTHON_PYBIND_CONFIGURE_CMDS = true PYTHON_PYBIND_BUILD_CMDS = true PYTHON_PYBIND_INSTALL_TARGET_CMDS = true PYTHON_PYBIND_INSTALL_STAGING_CMDS = true HOST_PYTHON_PYBIND_CONFIGURE_CMDS = true HOST_PYTHON_PYBIND_BUILD_CMDS = true HOST_PYTHON_PYBIND_INSTALL_CMDS = true Of course, this won't be very useful... Fixing the build of pybind in a way that does not make it usable by its dependents is useless, imho. If we fix it, we must fix it so that 1. it indeed builds and installs, and 2. that it is usable by its dependents, which means it should install properly. > Note also that scipy depends on host-pybind, not pybind itself. I'm not > sure how *that* is supposed to work - although, being header-only, it > probably doesn't really matter if you link with the host or the target > version of the library, and the host-installed pybind11-config makes sure > that you pick up the host headers (which are normally not in include path). > So conceptually wrong but works in practice. And if we keep it like that without proper explanations why that is, or if we do not fix it so that it behaves properly, it is going to become a maintenance minefield down the road... Already is, anyway... :-] > In conclusion: I think this patch is going in the right direction (modulo > BR2_CMAKE_HOST_DEPENDENCY). It's still "conceptually wrong but works" > because it doesn't set the toolchain file and other cmake options, but that > happens to work because it's header-only so it doesn't actually need any of > the cmake options. So it's not entirely the correct thing to do, but it does > guarantee to fix things. I'm not sure if that means we should merge it or > not... For me, no. But since we can't continue having build failures, and becasue we have no package that depends on pybind, if we eventualyl do not get a solution, then we can still remove it (it's broken!). > The proper fix, I think, would require patching setup.py so we can pass in > additional cmake options to it. Or as we've been discussing in another mail and IRC, by providing a cmake wrapper in $(@D)/bin/cmake (not unlike some packages for which we do a python symlink in $(@D)/bin/pyhon). But now that the cmake issue has been identified, I think that we are narrowing down on to a proper solution (Famous Last Words™). Regards, Yann E. MORIN.
Hi, On 06/12/2021 23:57, Yann E. MORIN wrote: > So I think we really, really need a package (like scipy) that actually > makes use of pybind, so we can see how it works, and to fix it... Jfyi, there are two relatively simple packages in buildroot that can use pybind: kmsxx and rwmem. Both use meson for building. I can test the next version of this series with those packages. Tomi
On 03/01/2022 09:24, Tomi Valkeinen wrote: > Hi, > > On 06/12/2021 23:57, Yann E. MORIN wrote: > >> So I think we really, really need a package (like scipy) that actually >> makes use of pybind, so we can see how it works, and to fix it... > > Jfyi, there are two relatively simple packages in buildroot that can use pybind: > kmsxx and rwmem. Both use meson for building. > > I can test the next version of this series with those packages. I tried to do that. Unfortunately, it turns out that both of those packages use the CMake way of finding the pybind11 dependency, and apparently our cross-compilation.conf doesn't support cmake yet... So it's not that trivial. I merged the pybind stuff but wasn't able to test the cmake handling, so it's even possible that that is broken as well... Regards, Arnout
On 08/12/2021 12:19, Andreas Naumann wrote: [snip] > By chance I was playing with such a package just yesterday: zxing-cpp. Not the > version buildroot uses (from github glassechidna), but the nu-book one which > claims to be faster. (And is the one where the python bindings in the PyPi > package zxing-cpp originate from). > > Until recently they shipped their own pybind11 version, which throws an error > when included in a cmake project in a crosscompile environment. This is a known > problem, https://github.com/pybind/pybind11/issues/2139 but non of the proposed > fixes has been merged yet. > > However, the unreleased master version of zxing-cpp hast been changed so it can > be configured to use the a system pybind11. So I tried to include the br > packaged one (2.6.1 as well as 2.8.1). They both built on my system (no problem > with the hardcoded host cmake here) but I needed to change three things: > 1. Add PYTHON_PYBIND_INSTALL_STAGING = YES > 2. Set ZXING_CPP_CONF_OPTS += > -Dpybind11_DIR=$(STAGING_DIR)/usr/lib/python$(PYTHON3_VERSION_MAJOR)/site-packages/pybind11/share/cmake/pybind11 > > 3. Apply one of the proposed fixes for the pybind11 crosscompile problem (to > tools/pybind11Tools.cmake) > > So this patch here seems to help my (and possibly others) because 1. is already > included. If interested, I could send a patch for 3. I applied a slightly different approach to master [1]. If you'd like to give that one a go, we'd welcome an updated zxing-cpp package! Regards, Arnout [1] https://git.buildroot.org/buildroot/commit/?id=a19b822239c36cb40a632899bb5c3e848a5fc81e [snip]
On 07/01/2022 23:49, Arnout Vandecappelle wrote: > > > On 03/01/2022 09:24, Tomi Valkeinen wrote: >> Hi, >> >> On 06/12/2021 23:57, Yann E. MORIN wrote: >> >>> So I think we really, really need a package (like scipy) that actually >>> makes use of pybind, so we can see how it works, and to fix it... >> >> Jfyi, there are two relatively simple packages in buildroot that can >> use pybind: kmsxx and rwmem. Both use meson for building. >> >> I can test the next version of this series with those packages. > > I tried to do that. Unfortunately, it turns out that both of those > packages use the CMake way of finding the pybind11 dependency, and > apparently our cross-compilation.conf doesn't support cmake yet... So > it's not that trivial. Yes, I had a look at this also. kmsxx and rwmem use the plain meson dependency() to find the package. Works on Ubuntu, as Ubuntu's pybind11 installs the cmake files. I started testing with a more manual approach to finding pybind11, but didn't get it working yet. Tomi
On 08/01/2022 11:09, Tomi Valkeinen wrote: > On 07/01/2022 23:49, Arnout Vandecappelle wrote: >> >> >> On 03/01/2022 09:24, Tomi Valkeinen wrote: >>> Hi, >>> >>> On 06/12/2021 23:57, Yann E. MORIN wrote: >>> >>>> So I think we really, really need a package (like scipy) that actually >>>> makes use of pybind, so we can see how it works, and to fix it... >>> >>> Jfyi, there are two relatively simple packages in buildroot that can use >>> pybind: kmsxx and rwmem. Both use meson for building. >>> >>> I can test the next version of this series with those packages. >> >> I tried to do that. Unfortunately, it turns out that both of those packages >> use the CMake way of finding the pybind11 dependency, and apparently our >> cross-compilation.conf doesn't support cmake yet... So it's not that trivial. > > Yes, I had a look at this also. kmsxx and rwmem use the plain meson dependency() > to find the package. Works on Ubuntu, as Ubuntu's pybind11 installs the cmake > files. I also made sure that python-pybind installs the cmake files. I just haven't tested if they actually work. I already noticed that there's one of the files that is not cross-compile friendly because it found /usr/bin/python. But maybe that one isn't actually used in practice. > I started testing with a more manual approach to finding pybind11, but didn't > get it working yet. No, the cmake approach should be fine. We just have to update the meson cross-compilation.conf so it points to STAGING_DIR/usr/share/cmake. Regards, Arnout
diff --git a/package/python-pybind/python-pybind.mk b/package/python-pybind/python-pybind.mk index a6a1bdb976..bfd7f6f59a 100644 --- a/package/python-pybind/python-pybind.mk +++ b/package/python-pybind/python-pybind.mk @@ -9,5 +9,12 @@ PYTHON_PYBIND_SITE = $(call github,pybind,pybind11,v$(PYTHON_PYBIND_VERSION)) PYTHON_PYBIND_LICENSE = BSD-3-Clause PYTHON_PYBIND_LICENSE_FILES = LICENSE PYTHON_PYBIND_SETUP_TYPE = setuptools +PYTHON_PYBIND_INSTALL_STAGING = YES + +# every single 'python setup.py' call actually calls cmake +# internally, to populate $(@D)/pybind11 with build requirements. +# If we have a host-cmake env at that moment, +# then the final installation paths are properly defined +PYTHON_PYBIND_DEPENDENCIES = host-cmake $(eval $(python-package))