Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed type handling for include and lib directories #7069

Merged
merged 3 commits into from
Apr 11, 2023

Conversation

adisbladis
Copy link
Contributor

@adisbladis adisbladis commented Apr 5, 2023

Recent changes to build script resulted in some type confusion around lib_root and lib_dir which may be either a list of strings or just a string.

Helps #7066

@radarhere
Copy link
Member

For clarity, 'recent changes' refers to #6896

setup.py Outdated Show resolved Hide resolved
Depending on whether these are created by pkg_config or not they might
be a list of directories or just a string with a single directory.
@sebastic
Copy link
Contributor

sebastic commented Apr 5, 2023

Just for the record, lib_root and include_root are strings when using environment variables to set the prefix (e.g. ZLIB_ROOT=/opt) instead of using pkg-config because _pkg_config() always returns a list.

You can also make that case use a tuple of lists to match the _pkg_config() return values:

diff --git a/setup.py b/setup.py
index 07d6c66d6..ed5c92841 100755
--- a/setup.py
+++ b/setup.py
@@ -454,7 +454,7 @@ class pil_build_ext(build_ext):
 
             if root is None and root_name in os.environ:
                 prefix = os.environ[root_name]
-                root = (os.path.join(prefix, "lib"), os.path.join(prefix, "include"))
+                root = ([os.path.join(prefix, "lib")], [os.path.join(prefix, "include")])
 
             if root is None and pkg_config:
                 if isinstance(lib_name, tuple):

@radarhere
Copy link
Member

I almost suggested just using the code from @sebastic, but then found that there are these variables.

Pillow/setup.py

Lines 30 to 38 in 943a7a8

FREETYPE_ROOT = None
HARFBUZZ_ROOT = None
FRIBIDI_ROOT = None
IMAGEQUANT_ROOT = None
JPEG2K_ROOT = None
JPEG_ROOT = None
LCMS_ROOT = None
TIFF_ROOT = None
ZLIB_ROOT = None

I presume the intention is to allow users to modify setup.py and insert values here, and it would be good to allow those values to be strings.

@radarhere
Copy link
Member

I've created adisbladis#1 with a suggestion.

@radarhere radarhere changed the title Include dir type confusion Fixed type handling for include and lib directories Apr 10, 2023
@adisbladis
Copy link
Contributor Author

Proposed changes by @radarhere LGTM.

@radarhere radarhere merged commit 0ec0a89 into python-pillow:main Apr 11, 2023
sigprof added a commit to sigprof/nix-devenv-qmk that referenced this pull request May 5, 2023
There is a bug in the `pillow` 9.5.0 release that breaks the build:

  nix-community/poetry2nix#1139
  python-pillow/Pillow#7069

Apply the patch with the upstream fix when building version 9.5.0.

The old override is still kept, even though the main part of that
override was included in nix-community/poetry2nix#689 (the problem is
that `buildInputs` in the poetry2nix override does not include all
packages that are in the nixpkgs package for `pillow`, and therefore
`preConfigure` might add references to nonexistent packages).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants