[Buildroot] [PATCH 1/1] package/python-pillow: fix pkg-config search paths
James Hilliard
james.hilliard1 at gmail.com
Tue Mar 22 18:33:14 UTC 2022
On Mon, Mar 21, 2022 at 5:10 PM Angelo Compagnucci
<angelo at amarulasolutions.com> wrote:
>
>
>
> On Mon, Mar 21, 2022 at 11:35 PM Angelo Compagnucci <angelo at amarulasolutions.com> wrote:
>>
>>
>>
>> On Mon, Mar 21, 2022 at 11:10 PM Angelo Compagnucci <angelo at amarulasolutions.com> wrote:
>>>
>>>
>>>
>>> On Mon, Mar 21, 2022 at 10:56 PM James Hilliard <james.hilliard1 at gmail.com> wrote:
>>>>
>>>> On Mon, Mar 21, 2022 at 3:32 PM Arnout Vandecappelle <arnout at mind.be> wrote:
>>>> >
>>>> > Hi James,
>>>> >
>>>> > On 16/03/2022 07:02, James Hilliard wrote:
>>>> > > Currently pillow doesn't correctly search pkg-config system paths
>>>> > > for some libraries which can prevent some libraries from being
>>>> > > properly detected/enabled in pillow.
>>>> > >
>>>> > > Signed-off-by: James Hilliard <james.hilliard1 at gmail.com>
>>>> > > ---
>>>> > > ...Search-pkg-config-system-libs-cflags.patch | 33 +++++++++++++++++++
>>>> > > package/python-pillow/python-pillow.mk | 23 +++----------
>>>> > > 2 files changed, 37 insertions(+), 19 deletions(-)
>>>> > > create mode 100644 package/python-pillow/0001-Search-pkg-config-system-libs-cflags.patch
>>>> > >
>>>> > > diff --git a/package/python-pillow/0001-Search-pkg-config-system-libs-cflags.patch b/package/python-pillow/0001-Search-pkg-config-system-libs-cflags.patch
>>>> > > new file mode 100644
>>>> > > index 0000000000..9f979b048f
>>>> > > --- /dev/null
>>>> > > +++ b/package/python-pillow/0001-Search-pkg-config-system-libs-cflags.patch
>>>> > > @@ -0,0 +1,33 @@
>>>> > > +From 89c9c347bb7437d5ea559930e315f42a0236761f Mon Sep 17 00:00:00 2001
>>>> > > +From: James Hilliard <james.hilliard1 at gmail.com>
>>>> > > +Date: Tue, 15 Mar 2022 23:31:59 -0600
>>>> > > +Subject: [PATCH] Search pkg-config system libs/cflags.
>>>> > > +
>>>> > > +We need to search the system paths as well from pkg-config for
>>>> > > +some packages to be found properly.
>>>> > > +
>>>> > > +Signed-off-by: James Hilliard <james.hilliard1 at gmail.com>
>>>> > > +[Upstream status:
>>>> > > +https://github.com/python-pillow/Pillow/pull/6138]
>>>> > > +---
>>>> > > + setup.py | 4 ++--
>>>> > > + 1 file changed, 2 insertions(+), 2 deletions(-)
>>>> > > +
>>>> > > +diff --git a/setup.py b/setup.py
>>>> > > +index 3468b260..59d65ce2 100755
>>>> > > +--- a/setup.py
>>>> > > ++++ b/setup.py
>>>> > > +@@ -252,8 +252,8 @@ def _cmd_exists(cmd):
>>>> > > + def _pkg_config(name):
>>>> > > + try:
>>>> > > + command = os.environ.get("PKG_CONFIG", "pkg-config")
>>>> > > +- command_libs = [command, "--libs-only-L", name]
>>>> > > +- command_cflags = [command, "--cflags-only-I", name]
>>>> > > ++ command_libs = [command, "--libs-only-L", "--keep-system-libs", name]
>>>> > > ++ command_cflags = [command, "--cflags-only-I", "--keep-system-cflags", name]
>>>> >
>>>> > You gave an additional explanation why this is needed in the upstream PR:
>>>> >
>>>> > Zlib and anything that has headers in the normal system libs/include folders
>>>> > seems to not get their headers picked up without this change.
>>>> >
>>>> > I think this issue is mostly going to be something people hit when cross
>>>> > compiling since the prefix based system libs/include folder would probably work
>>>> > in most of the usual cases(searching in sys.prefix will not work when cross
>>>> > compiling since it points to the host toolchain prefix rather than the target
>>>> > toolchain sysroot prefix):
>>>> >
>>>> >
>>>> > However, that implies that either we have to make sure that sys.prefix is set
>>>> > correctly (i.e. point it to staging instead of host), or pillow is using
>>>> > sys.prefix incorrectly.
>>>>
>>>> I think pillow is using sys.prefix in a way that is not really cross compilation
>>>> compatible, however using pkg-config with sysm libs/cflags seems to be
>>>> sufficient
>>>> for it to pass its non-standard header checks.
>>>
>>>
>>> I remember having added the --disable-platform-guessing exactly to overcome this problem. All the setting should be provided by buildroot. Probably, the logic behind this is slightly changed, and the mechanism doesn't work anymore. I'll try to have a look.
>>
>>
>> Yes, basically the paths leaks some host directories:
>>
>> [...]
>> Checking for include file %s in %s ('libimagequant.h', '/usr/local/include')
>> Checking for include file %s in %s ('libimagequant.h', '/usr/include')
>>
>> Looking forward for a fix.
>
>
> I came to the conclusion that James setup.py patch is necessary, pillow doesn't use sys.prefix but builds the paths from the pkg-config output. Withouth the patch, include and libraries directories are wrong.
> It misses a piece anyway
>
> +PYTHON_PILLOW_BUILD_OPTS = build_ext --disable-platform-guessing
> +PYTHON_PILLOW_INSTALL_TARGET_OPTS = $(PYTHON_PILLOW_BUILD_OPTS) install
https://patchwork.ozlabs.org/project/buildroot/patch/20220322182952.935255-1-james.hilliard1@gmail.com/
The install part isn't needed here since it will still get passed by
_BASE_INSTALL_TARGET_CMD.
See:
https://github.com/buildroot/buildroot/blob/eaa8fcf546d84e38990566e49f4730c530d2577c/package/pkg-python.mk#L199
https://github.com/buildroot/buildroot/blob/eaa8fcf546d84e38990566e49f4730c530d2577c/package/pkg-python.mk#L285
>
> target install options should match the build ones to have a correct library installation.
>
>>
>>>
>>>>
>>>>
>>>> >
>>>> > Before that, still, I don't understand how this can be an issue. Unless if
>>>> > pillow also passes something like -nostdinc, our toolchain wrapper should make
>>>> > sure that staging/usr/include is in the search path.
>>>>
>>>> It's due to pillow having custom header checks like this:
>>>> https://github.com/python-pillow/Pillow/blob/9.0.1/setup.py#L633
>>>>
>>>> >
>>>> > There also doesn't seem to be an autobuilder failure due to this... But maybe
>>>> > it builds successfully, just without some optional dependency? Please provide
>>>> > more details about the failure in the commit message.
>>>>
>>>> The failure was getting hidden by the non-standard build/install cmd
>>>> overrides it
>>>> would appear, it seemed pillow was getting built for the host instead
>>>> of the target with
>>>> those, I didn't investigate in too much detail as those custom
>>>> build/install overrides
>>>> are not actually needed.
>>>>
>>>> >
>>>> >
>>>> >
>>>> > > + if not DEBUG:
>>>> > > + command_libs.append("--silence-errors")
>>>> > > + command_cflags.append("--silence-errors")
>>>> > > +--
>>>> > > +2.35.1
>>>> > > +
>>>> > > diff --git a/package/python-pillow/python-pillow.mk b/package/python-pillow/python-pillow.mk
>>>> > > index 901876e0ee..ef677855b2 100644
>>>> > > --- a/package/python-pillow/python-pillow.mk
>>>> > > +++ b/package/python-pillow/python-pillow.mk
>>>> > > @@ -12,7 +12,10 @@ PYTHON_PILLOW_LICENSE_FILES = LICENSE
>>>> > > PYTHON_PILLOW_CPE_ID_VENDOR = python
>>>> > > PYTHON_PILLOW_CPE_ID_PRODUCT = pillow
>>>> > > PYTHON_PILLOW_SETUP_TYPE = setuptools
>>>> > > -PYTHON_PILLOW_BUILD_OPTS = --disable-platform-guessing
>>>> > > +PYTHON_PILLOW_ENV = MAX_CONCURRENCY="$(PARALLEL_JOBS)"
>>>> >
>>>> > This change isn't explained in the commit message, and seems unrelated.
>>>> >
>>>> > > +
>>>> > > +PYTHON_PILLOW_DEPENDENCIES = host-pkgconf
>>>> > > +PYTHON_PILLOW_BUILD_OPTS = build_ext --disable-platform-guessing
>>>> >
>>>> > The need for these two changes is also not clear at all from the commit message.
>>>> >
>>>> >
>>>> > I've marked the patch as Changes Requested.
>>>> >
>>>> > Regards,
>>>> > Arnout
>>>> >
>>>> > >
>>>> > > ifeq ($(BR2_PACKAGE_FREETYPE),y)
>>>> > > PYTHON_PILLOW_DEPENDENCIES += freetype
>>>> > > @@ -68,22 +71,4 @@ else
>>>> > > PYTHON_PILLOW_BUILD_OPTS += --disable-webp --disable-webpmux
>>>> > > endif
>>>> > >
>>>> > > -define PYTHON_PILLOW_BUILD_CMDS
>>>> > > - cd $(PYTHON_PILLOW_BUILDDIR); \
>>>> > > - PKG_CONFIG_ALLOW_SYSTEM_CFLAGS=1 \
>>>> > > - $(PYTHON_PILLOW_BASE_ENV) $(PYTHON_PILLOW_ENV) \
>>>> > > - $(PYTHON_PILLOW_PYTHON_INTERPRETER) setup.py build_ext \
>>>> > > - $(PYTHON_PILLOW_BASE_BUILD_OPTS) $(PYTHON_PILLOW_BUILD_OPTS)
>>>> > > -endef
>>>> > > -
>>>> > > -define PYTHON_PILLOW_INSTALL_TARGET_CMDS
>>>> > > - cd $(PYTHON_PILLOW_BUILDDIR); \
>>>> > > - PKG_CONFIG_ALLOW_SYSTEM_CFLAGS=1 \
>>>> > > - $(PYTHON_PILLOW_BASE_ENV) $(PYTHON_PILLOW_ENV) \
>>>> > > - $(PYTHON_PILLOW_PYTHON_INTERPRETER) setup.py build_ext \
>>>> > > - $(PYTHON_PILLOW_BUILD_OPTS) install \
>>>> > > - $(PYTHON_PILLOW_BASE_INSTALL_TARGET_OPTS) \
>>>> > > - $(PYTHON_PILLOW_INSTALL_TARGET_OPTS)
>>>> > > -endef
>>>> > > -
>>>> > > $(eval $(python-package))
>>>> _______________________________________________
>>>> buildroot mailing list
>>>> buildroot at buildroot.org
>>>> https://lists.buildroot.org/mailman/listinfo/buildroot
>>>
>>>
>>>
>>> --
>>>
>>> Angelo Compagnucci
>>>
>>> Software Engineer
>>>
>>> angelo at amarulasolutions.com
>>> __________________________________
>>> Amarula Solutions SRL
>>>
>>> Via le Canevare 30, 31100 Treviso, Veneto, IT
>>>
>>> T. +39 (0)42 243 5310
>>> info at amarulasolutions.com
>>>
>>> www.amarulasolutions.com
>>>
>>> [`as] https://www.amarulasolutions.com|
>>
>>
>>
>> --
>>
>> Angelo Compagnucci
>>
>> Software Engineer
>>
>> angelo at amarulasolutions.com
>> __________________________________
>> Amarula Solutions SRL
>>
>> Via le Canevare 30, 31100 Treviso, Veneto, IT
>>
>> T. +39 (0)42 243 5310
>> info at amarulasolutions.com
>>
>> www.amarulasolutions.com
>>
>> [`as] https://www.amarulasolutions.com|
>
>
>
> --
>
> Angelo Compagnucci
>
> Software Engineer
>
> angelo at amarulasolutions.com
> __________________________________
> Amarula Solutions SRL
>
> Via le Canevare 30, 31100 Treviso, Veneto, IT
>
> T. +39 (0)42 243 5310
> info at amarulasolutions.com
>
> www.amarulasolutions.com
>
> [`as] https://www.amarulasolutions.com|
More information about the buildroot
mailing list