Handle more than one directory returned by pkg-config #6896
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
radarhere
changed the title
|
Hi. We test Debian 10 and 11 in our CI, and there's no problem there using tiff. Looking at https://packages.debian.org/search?searchon=sourcenames&keywords=tiff, I gather from your tiff version that you're using the currently unreleased Debian 12, aka bookworm? Or maybe sid? |
|
tiff (>= 4.5.0-1) is currently in Debian testing (bookworm) and unstable (sid). |
|
If I take our docker images for Debian 10 and 11, and I search for tiff.h, I get So even with 4.1.0, tiff.h is in /usr/include/x86_64-linux-gnu/tiff.h, and it's being found by Pillow. Something else must be going on here. Could you create a Dockerfile demonstrating the problem? |
No, I don't use docker. You can see the issue in the build log of the pillow Debian package which was recently rebuilt and lost the tiff support: The previous build with tiff (4.4.0-6) worked as expected: https://buildd.debian.org/status/fetch.php?pkg=pillow&arch=amd64&ver=9.4.0-1&stamp=1672760314&raw=0 There are not many changes in dependencies between those builds: --- /tmp/pillow_9.4.0-1.build.deps 2023-01-15 07:59:24.330984710 +0100
+++ /tmp/pillow_9.4.0-1+b1.build.deps 2023-01-15 07:59:31.058793479 +0100
@@ -6,10 +6,10 @@
Setting up bsdextrautils (2.38.1-4) ...
Setting up debhelper (13.11.4) ...
Setting up dh-autoreconf (20) ...
-Setting up dh-python (5.20221230) ...
+Setting up dh-python (5.20230109) ...
Setting up dh-strip-nondeterminism (1.13.0-2) ...
Setting up dwz (0.15-1) ...
-Setting up file (1:5.41-4) ...
+Setting up file (1:5.44-1) ...
Setting up fontconfig-config (2.13.1-4.5) ...
Setting up fonts-dejavu-core (2.37-2) ...
Setting up gettext (0.21-10) ...
@@ -36,9 +36,9 @@
Setting up libfontconfig1:amd64 (2.13.1-4.5) ...
Setting up libfontconfig1-dev:amd64 (2.13.1-4.5) ...
Setting up libfontconfig-dev:amd64 (2.13.1-4.5) ...
-Setting up libfreetype6:amd64 (2.12.1+dfsg-3) ...
-Setting up libfreetype6-dev:amd64 (2.12.1+dfsg-3) ...
-Setting up libfreetype-dev:amd64 (2.12.1+dfsg-3) ...
+Setting up libfreetype6:amd64 (2.12.1+dfsg-4) ...
+Setting up libfreetype6-dev:amd64 (2.12.1+dfsg-4) ...
+Setting up libfreetype-dev:amd64 (2.12.1+dfsg-4) ...
Setting up libfribidi0:amd64 (1.0.8-2.1) ...
Setting up libfribidi-dev:amd64 (1.0.8-2.1) ...
Setting up libgirepository-1.0-1:amd64 (1.74.0-2+b1) ...
@@ -65,18 +65,19 @@
Setting up libjs-jquery (3.6.1+dfsg+~3.5.14-1) ...
Setting up libjs-sphinxdoc (5.3.0-2) ...
Setting up libjs-underscore (1.13.4~dfsg+~1.11.4-3) ...
-Setting up liblcms2-2:amd64 (2.13.1-1+b1) ...
-Setting up liblcms2-dev:amd64 (2.13.1-1+b1) ...
+Setting up liblcms2-2:amd64 (2.14-1+b1) ...
+Setting up liblcms2-dev:amd64 (2.14-1+b1) ...
Setting up liblerc4:amd64 (4.0.0+ds-2) ...
Setting up liblerc-dev:amd64 (4.0.0+ds-2) ...
-Setting up liblzma-dev:amd64 (5.4.0-0.1) ...
-Setting up libmagic1:amd64 (1:5.41-4) ...
-Setting up libmagic-mgc (1:5.41-4) ...
+Setting up liblzma5:amd64 (5.4.1-0.0) ...
+Setting up liblzma-dev:amd64 (5.4.1-0.0) ...
+Setting up libmagic1:amd64 (1:5.44-1) ...
+Setting up libmagic-mgc (1:5.44-1) ...
Setting up libmount-dev:amd64 (2.38.1-4) ...
Setting up libmpdec3:amd64 (2.5.1-2) ...
Setting up libncursesw6:amd64 (6.4-1) ...
-Setting up libopenjp2-7:amd64 (2.5.0-1) ...
-Setting up libopenjp2-7-dev:amd64 (2.5.0-1) ...
+Setting up libopenjp2-7:amd64 (2.5.0-1+b1) ...
+Setting up libopenjp2-7-dev:amd64 (2.5.0-1+b1) ...
Setting up libpcre2-16-0:amd64 (10.42-1) ...
Setting up libpcre2-32-0:amd64 (10.42-1) ...
Setting up libpcre2-dev:amd64 (10.42-1) ...
@@ -105,18 +106,17 @@
Setting up libsqlite3-0:amd64 (3.40.1-1) ...
Setting up libsub-override-perl (0.09-4) ...
Setting up libtcl8.6:amd64 (8.6.13+dfsg-1) ...
-Setting up libtiff5:amd64 (4.4.0-6) ...
-Setting up libtiff5-dev:amd64 (4.4.0-6) ...
-Setting up libtiff-dev:amd64 (4.4.0-6) ...
-Setting up libtiffxx5:amd64 (4.4.0-6) ...
-Setting up libtinfo6:amd64 (6.4-1) ...
+Setting up libtiff5-dev:amd64 (4.5.0-3) ...
+Setting up libtiff6:amd64 (4.5.0-3) ...
+Setting up libtiff-dev:amd64 (4.5.0-3) ...
+Setting up libtiffxx6:amd64 (4.5.0-3) ...
Setting up libtk8.6:amd64 (8.6.13-1) ...
Setting up libtool (2.4.7-5) ...
Setting up libuchardet0:amd64 (0.0.7-1) ...
-Setting up libwebp7:amd64 (1.2.2-2+b2) ...
-Setting up libwebpdemux2:amd64 (1.2.2-2+b2) ...
-Setting up libwebp-dev:amd64 (1.2.2-2+b2) ...
-Setting up libwebpmux3:amd64 (1.2.2-2+b2) ...
+Setting up libwebp7:amd64 (1.2.2-2+b3) ...
+Setting up libwebpdemux2:amd64 (1.2.2-2+b3) ...
+Setting up libwebp-dev:amd64 (1.2.2-2+b3) ...
+Setting up libwebpmux3:amd64 (1.2.2-2+b3) ...
Setting up libx11-6:amd64 (2:1.8.3-3) ...
Setting up libx11-data (2:1.8.3-3) ...
Setting up libx11-dev:amd64 (2:1.8.3-3) ...
@@ -135,11 +135,10 @@
Setting up libxrender-dev:amd64 (1:0.9.10-1.1) ...
Setting up libxss1:amd64 (1:1.2.3-1) ...
Setting up libxss-dev:amd64 (1:1.2.3-1) ...
-Setting up m4 (1.4.19-1) ...
-Setting up man-db (2.11.1-1) ...
+Setting up libzstd-dev:amd64 (1.5.2+dfsg2-3) ...
+Setting up m4 (1.4.19-2) ...
+Setting up man-db (2.11.2-1) ...
Setting up media-types (8.0.0) ...
-Setting up ncurses-base (6.4-1) ...
-Setting up ncurses-bin (6.4-1) ...
Setting up pkgconf:amd64 (1.8.0-12) ...
Setting up pkgconf-bin (1.8.0-12) ...
Setting up pkg-config:amd64 (1.8.0-12) ...
@@ -193,4 +192,5 @@
Setting up x11proto-dev (2022.1-1) ...
Setting up xorg-sgml-doctools (1:1.11-1.1) ...
Setting up xtrans-dev (1.4.0-1) ...
+Setting up xz-utils (5.4.1-0.0) ...
Setting up zlib1g-dev:amd64 (1:1.2.13.dfsg-1) ...The changes in this PR resolved the issue in the pillow Debian package as reported in Debian Bug #1028904. The Your setup likely gets the include path from pkg-config which doesn't happen for my builds. The two include paths returned by pkg-config are not used: A single include path is appended like for openjpeg: This is the pkg-config command in question: To get the include path from pkg-config you'll need to loop over the paths returned, e.g.: _add_directory(library_dirs, lib_root)
if include_root is not None and ' ' in include_root:
for include_dir in include_root.split(" "):
_add_directory(include_dirs, include_dir)
else:
_add_directory(include_dirs, include_root)This likely breaks with pkg-config on Windows or if a single path contains a space, so def _pkg_config(name):
command = os.environ.get("PKG_CONFIG", "pkg-config")
for keep_system in (True, False):
try:
command_libs = [command, "--libs-only-L", name]
command_cflags = [command, "--cflags-only-I", name]
stderr = None
if keep_system:
command_libs.append("--keep-system-libs")
command_cflags.append("--keep-system-cflags")
stderr = subprocess.DEVNULL
if not DEBUG:
command_libs.append("--silence-errors")
command_cflags.append("--silence-errors")
libs = re.split(
r'\s*-L',
subprocess.check_output(command_libs, stderr=stderr)
.decode("utf8")
.strip()
)
libs.remove("")
cflags = re.split(
r'\s*-I',
subprocess.check_output(command_cflags, stderr=stderr)
.decode("utf8")
.strip()
)
cflags.remove("")
return libs, cflags
except Exception:
pass
[...]
if lib_root is not None:
for lib_dir in lib_root:
_add_directory(library_dirs, lib_dir)
if include_root is not None:
for include_dir in include_root:
_add_directory(include_dirs, include_dir)This is the patch for --- a/setup.py
+++ b/setup.py
@@ -266,18 +266,20 @@ def _pkg_config(name):
if not DEBUG:
command_libs.append("--silence-errors")
command_cflags.append("--silence-errors")
- libs = (
+ libs = re.split(
+ r'\s*-L',
subprocess.check_output(command_libs, stderr=stderr)
.decode("utf8")
.strip()
- .replace("-L", "")
)
- cflags = (
- subprocess.check_output(command_cflags)
+ libs.remove("")
+ cflags = re.split(
+ r'\s*-I',
+ subprocess.check_output(command_cflags, stderr=stderr)
.decode("utf8")
.strip()
- .replace("-I", "")
)
+ cflags.remove("")
return libs, cflags
except Exception:
pass
@@ -508,8 +510,12 @@ class pil_build_ext(build_ext):
else:
lib_root = include_root = root
- _add_directory(library_dirs, lib_root)
- _add_directory(include_dirs, include_root)
+ if lib_root is not None:
+ for lib_dir in lib_root:
+ _add_directory(library_dirs, lib_dir)
+ if include_root is not None:
+ for include_dir in include_root:
+ _add_directory(include_dirs, include_dir)
# respect CFLAGS/CPPFLAGS/LDFLAGS
for k in ("CFLAGS", "CPPFLAGS", "LDFLAGS"): |
|
The pkg-config change in behaviour comes from this change between libtiff-dev (4.4.0-6) and libtiff-dev (4.5.0-3): diff -ruN /tmp/libtiff-dev_4.4.0-6/usr/lib/x86_64-linux-gnu/pkgconfig/libtiff-4.pc /tmp/libtiff-dev_4.5.0-3/usr/lib/x86_64-linux-gnu/pkgconfig/libtiff-4.pc
--- /tmp/libtiff-dev_4.4.0-6/usr/lib/x86_64-linux-gnu/pkgconfig/libtiff-4.pc 2022-11-24 17:54:18.000000000 +0100
+++ /tmp/libtiff-dev_4.5.0-3/usr/lib/x86_64-linux-gnu/pkgconfig/libtiff-4.pc 2023-01-12 17:45:09.000000000 +0100
@@ -5,8 +5,8 @@
Name: libtiff
Description: Tag Image File Format (TIFF) library.
-Version: 4.4.0
+Version: 4.5.0
Libs: -L${libdir} -ltiff
Libs.private: -lwebp -lzstd -llzma -lLerc -ljbig -ljpeg -ldeflate -lz -lm
Cflags: -I${includedir}
-Requires.private:
+Requires.private: libwebp libzstd liblzma libjpeg libdeflate zlib Without the private dependencies the pkg-config result is just the single directory: With the private dependencies added the result is two include directories: Shall I rebase this PR to use my |
tiff (4.5.0-1) in Debian results in two include directories being returned: ``` -I/usr/include/x86_64-linux-gnu -I/usr/include ```
sebastic
force-pushed
the
tiff-multiarch
branch
from
fb4219e to
04cf5e2
Compare
sebastic
changed the title
|
@radarhere, any chance of merging this? We already carry the patch in the Debian package, but would not like to do that indefinitely. |
radarhere
reviewed
Jan 28, 2023
| libs.remove("") | ||
| cflags = re.split( | ||
| r"\s*-I", | ||
| subprocess.check_output(command_cflags, stderr=stderr) |
There was a problem hiding this comment.
Did you want to briefly explain why you added , stderr=stderr here? It was added to the libs command in #6261 to avoid a Unknown option --keep-system-libs error, but that doesn't apply to the cflags command.
There was a problem hiding this comment.
To be consistent with the other check_output call, since only STDOUT is used to get the paths.
There was a problem hiding this comment.
After discussion, this was later reverted in #7046
This was referenced Mar 30, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
tiff (4.5.0-1) in Debian results in two include directories being returned:
These were not added to the compiler include directories, causing tiff support not be enabled because
tiff.hcould not be found.