`build/pkgs/pillow`: Allow discovery of libjpeg, upgrade to 10.1.0 #36731

mkoeppe · 2023-11-18T00:38:12Z

As discussed in https://groups.google.com/g/sage-devel/c/A4nY3bh1oPI @egourgoulhon

Here we makelibjpeg a new "recommended" dummy package with spkg-configure script -- same category as pandoc, ffmpeg, imagemagick, texlive, so it appears in the installation instructions in https://deploy-preview-36731--sagemath-tobias.netlify.app/html/en/installation/source#debian-ubuntu-package-installation (in the 3rd command line for each of the shown package systems).

Use of libjpeg can be disabled using ./configure --without-system-libjpeg.

Everything done here would also be necessary if we decided to make libjpeg a normal standard/optional package; that is beyond the scope of this PR.

We also refine the "CI Linux incremental" workflow, which determines what packages to uninstall (so that spkg-configure is tested) or to build (important for optional packages), based on changed files. Failures seen in https://github.com/sagemath/sage/actions/runs/6914890950/job/18813211070?pr=36731#step:4:8 came from the workflow trying to build dummy packages. We suppress these now, as seen in https://github.com/sagemath/sage/actions/runs/6915792754/job/18815135590?pr=36731#step:4:8

Tests at https://github.com/mkoeppe/sage/actions/runs/6911535503 (Linux), https://github.com/mkoeppe/sage/actions/runs/6911535500 (macOS)

📝 Checklist

The title is concise, informative, and self-explanatory.
The description explains in detail what this PR is about.
I have linked a relevant issue or discussion.
I have created tests covering the changes.
I have updated the documentation accordingly.

⌛ Dependencies

orlitzky · 2023-11-18T02:26:59Z

Will pillow still build on a system with no libjpeg? Their docs imply that it won't:

Starting with Pillow 3.0.0, libjpeg is required by default, but may be disabled with the --disable-jpeg flag.

Either required or disabled is better than having it be auto-detected, but if it's going to be required we should probably put it in _prereq and not _recommended.

mkoeppe · 2023-11-18T02:47:08Z

Will pillow still build on a system with no libjpeg? Their docs imply that it won't:

Starting with Pillow 3.0.0, libjpeg is required by default, but may be disabled with the --disable-jpeg flag.

Thanks, I'll check

mkoeppe · 2023-11-18T02:51:04Z

I wouldn't like to make it a required dummy package - because as Marc @culler pointed out, it's not available on macOS (without homebrew).

mkoeppe · 2023-11-18T02:55:51Z

Their docs imply that it won't:

Code agrees - https://github.com/python-pillow/Pillow/blob/main/setup.py#L810

orlitzky · 2023-11-18T03:00:43Z

The risk with it being auto-detected is that if you happen to have libjpeg installed temporarily for some unrelated reason, building sage (even with --disable-libjpeg!) will unconditionally build a copy that requires libjpeg to run. Then when you later remove libjpeg, sage will crash.

mkoeppe · 2023-11-18T03:02:08Z

Well, that's true for pretty much any use of shared libs from system packages

orlitzky · 2023-11-18T03:08:49Z

Well, that's true for pretty much any use of shared libs from system packages

Yeah but with a required standard package you can at least avoid the eventual crash with --without-system-foo.

mkoeppe · 2023-11-18T03:10:37Z

That's a fair point. Not sure if a major point, but a fair point.

mkoeppe · 2023-11-18T03:14:37Z

We do have a variable PILLOW_CONFIG_SETTINGS that an ephemeral-system-packages-wary user could use for locking down what libraries pillow uses.

mkoeppe · 2023-11-18T03:21:23Z

And note that Pillow autodetects a bunch of other libraries and we are not forbidding that.

… packages

…packages

github-actions · 2023-11-18T19:47:44Z

Documentation preview for this PR (built with commit 30d5a7e; changes) is ready! 🎉

egourgoulhon

Thank you for implementing this!
LGTM. I've checked it on Ubuntu 22.04, with libjpeg-dev installed as a system package. More precisely, I've built Sage from scratch (starting from a git clone (-> Sage 10.2.rc4) + pull of this branch) and the output of ./configure is

Checking whether SageMath should install SPKG libjpeg...
checking for libjpeg... yes
configure: will use system package and not install SPKG libjpeg
...
libjpeg:                        using system package

Then I've checked that running

from PIL import Image
plot(sin(x)).save("fig.png")
img = Image.open("fig.png")
img

in a Jupyter notebook does not trigger any error (contrary to Sage 10.2.rc4), and that the subsequent code

img.convert("RGB").save("fig.jpg")

properly creates a jpeg image file.
I've also checked that the public notebook
https://nbviewer.org/github/sagemanifolds/SageManifolds/blob/master/Notebooks/SM_black_hole_rendering.ipynb
runs without any error.

mkoeppe · 2023-11-20T16:18:39Z

Thanks for testing!

dimpase · 2023-11-20T17:54:36Z

The risk with it being auto-detected is that if you happen to have libjpeg installed temporarily for some unrelated reason, building sage (even with --disable-libjpeg!) will unconditionally build a copy that requires libjpeg to run. Then when you later remove libjpeg, sage will crash.

In that sense, ./sage --pip install pillow is safer - it will most probably fetch a binary wheel with everything included.

culler · 2023-11-20T18:12:13Z

I agree with Dima. The binary wheel is the simplest way to do this. I wonder whether Sage should perhaps reconsider the policy about never installing binary wheels into the Sage tree. - Marc

…

On Mon, Nov 20, 2023 at 11:54 AM Dima Pasechnik ***@***.***> wrote: The risk with it being auto-detected is that if you happen to have libjpeg installed temporarily for some unrelated reason, building sage (even with --disable-libjpeg!) will unconditionally build a copy that requires libjpeg to run. Then when you later remove libjpeg, sage will crash. In that sense, ./sage --pip install pillow is safer - it will most probably fetch a binary wheel with everything included. — Reply to this email directly, view it on GitHub <#36731 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJ6CP7WAGIW7TJG4U2VA2LYFOKONAVCNFSM6AAAAAA7QRWX6KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJZGU2DMNBVHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

mkoeppe · 2023-11-20T18:14:51Z

In that sense, ./sage --pip install pillow is safer - it will most probably fetch a binary wheel with everything included.

Note that the included libraries in such binary wheels (built using auditwheel or delocate) may cause library conflicts of the same kind that we otherwise avoid using SAGE_SPKG_DEPCHECK files in spkg-configure.m4
Not saying that I'd be overly concerned about this in the particular case of pillow, but it is something that we do not have control over in general.

culler · 2023-11-20T18:20:06Z

In the case of libjpeg, it is not a dynamic library, but just an ar library archive. So I am pretty sure that the Pillow python extension is statically linked with libjpeg. - Marc

…

On Mon, Nov 20, 2023 at 12:15 PM Matthias Köppe ***@***.***> wrote: In that sense, ./sage --pip install pillow is safer - it will most probably fetch a binary wheel with everything included. Note that the included libraries in such binary wheels (built using auditwheel or delocate) may cause library conflicts of the same kind that we otherwise avoid using SAGE_SPKG_DEPCHECK files in spkg-configure.m4 Not saying that I'd be overly concerned about this in the particular case of pillow, but it is something that we do not have control over in general. — Reply to this email directly, view it on GitHub <#36731 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJ6CP34ZQJMMOVWK267PVLYFOM2NAVCNFSM6AAAAAA7QRWX6KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJZGU3TKNRYHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

mkoeppe · 2023-11-20T18:24:55Z

That's true about libjpeg (besides that, we know that Pillow is the only package using it!), but the concern extends to the other libraries bundled in the wheel:

$ unzip Pillow-10.1.0-cp311-cp311-macosx_10_10_x86_64.whl
[...]
  inflating: PIL/.dylibs/liblcms2.2.dylib  
  inflating: PIL/.dylibs/libpng16.16.dylib  
  inflating: PIL/.dylibs/libxcb.1.1.0.dylib  
  inflating: PIL/.dylibs/libbrotlidec.1.1.0.dylib  
  inflating: PIL/.dylibs/libfreetype.6.dylib  
  inflating: PIL/.dylibs/libXau.6.0.0.dylib  
  inflating: PIL/.dylibs/libbrotlicommon.1.1.0.dylib  
  inflating: PIL/.dylibs/libharfbuzz.0.dylib  
  inflating: PIL/.dylibs/libsharpyuv.0.dylib  
  inflating: PIL/.dylibs/libz.1.3.dylib  
  inflating: PIL/.dylibs/libwebp.7.dylib  
  inflating: PIL/.dylibs/libwebpmux.3.dylib  
  inflating: PIL/.dylibs/libopenjp2.2.5.0.dylib  
  inflating: PIL/.dylibs/libwebpdemux.2.dylib  
  inflating: PIL/.dylibs/liblzma.5.dylib  
  inflating: PIL/.dylibs/libtiff.6.dylib  
[...]

orlitzky · 2023-11-20T18:25:37Z

Note that the included libraries in such binary wheels (built using auditwheel or delocate) may cause library conflicts of the same kind that we otherwise avoid using SAGE_SPKG_DEPCHECK files in spkg-configure.m4 Not saying that I'd be overly concerned about this in the particular case of pillow, but it is something that we do not have control over in general.

I think libjpeg is actually pretty high-risk. It's a common system library and lots of math software needs to work with JPEG graphics for e.g. handwriting recognition. You'd have to be able to promise (a) that no other sage dependency would ever want to link to libjpeg, and (b) that the user himself wouldn't want to link a sage program to libjpeg.

mkoeppe · 2023-11-20T18:28:11Z

@culler Of course, in preparing your macOS app, you can simply test that the resulting binary works. But for the unattended from-source builds of Sage on user systems, relying on the published wheels from PyPI introduces a lot of potential for mysterious failures.

culler · 2023-11-20T18:33:37Z

On Mon, Nov 20, 2023 at 12:25 PM Michael Orlitzky ***@***.***> wrote: I think libjpeg is actually pretty high-risk. It's a common system library and lots of math software needs to work with JPEG graphics for e.g. handwriting recognition. You'd have to be able to promise (a) that no other sage dependency would ever want to link to libjpeg, and (b) that the user himself wouldn't want to link a sage program to libjpeg.

I think that concern applies to dynamic libraries, not to statically linked libraries. Nonetheless, Matthias seems to be saying that there are several dynamic libraries packages with the Pillow python extension. I know that delocate embeds .dylib files into Python packages, but they put the libraries inside the package (in a directory named .dylibs) and use relative rpaths. So, if Pillow is using delocate, I don't actually think that any other parts of sage would be able to find the dynamic libraries packaged with Pillow. - Marc

…

— Reply to this email directly, view it on GitHub <#36731 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJ6CP6XGV6WVZMITV4RLKDYFOOC3AVCNFSM6AAAAAA7QRWX6KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJZGU4TAMJUG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

mkoeppe · 2023-11-20T18:43:07Z

I don't actually think that any other parts of sage would be able to find the dynamic libraries packaged with Pillow.

I agree with this; but I think the failure mechanism is through possible global symbol clashes of different versions of the same library.

orlitzky · 2023-11-20T18:48:33Z

I'd have to try it to be sure. The issue isn't with finding the bundled libraries but rather with the conflicting symbol names. All of this stuff eventually gets linked into the same executable, so if two libraries (even transitively) provide the same symbols, you can get a conflict if they're not binary-compatible.

For example, the bundled pillow .so and the one on my system:

$ nm -gD /usr/lib/python3.11/site-packages/PIL/_imaging.cpython-311-x86_64-linux-gnu.so  | grep jpeg
                 U jpeg_CreateCompress@LIBJPEG_6.2
                 U jpeg_CreateDecompress@LIBJPEG_6.2
                 U jpeg_add_quant_table@LIBJPEG_6.2
00000000000375f0 T jpeg_buffer_dest
0000000000037090 T jpeg_buffer_src
                 U jpeg_destroy_compress@LIBJPEG_6.2
                 U jpeg_destroy_decompress@LIBJPEG_6.2
                 U jpeg_finish_compress@LIBJPEG_6.2
                 U jpeg_finish_decompress@LIBJPEG_6.2
                 U jpeg_read_header@LIBJPEG_6.2
                 U jpeg_read_scanlines@LIBJPEG_6.2
                 U jpeg_resync_to_restart@LIBJPEG_6.2
                 U jpeg_set_defaults@LIBJPEG_6.2
                 U jpeg_set_quality@LIBJPEG_6.2
                 U jpeg_simple_progression@LIBJPEG_6.2
                 U jpeg_start_compress@LIBJPEG_6.2
                 U jpeg_start_decompress@LIBJPEG_6.2
                 U jpeg_std_error@LIBJPEG_6.2
                 U jpeg_suppress_tables@LIBJPEG_6.2
                 U jpeg_write_marker@LIBJPEG_6.2
                 U jpeg_write_scanlines@LIBJPEG_6.2
0000000000064ee0 D libjpeg_turbo_version

$ nm -gD PIL/_imaging.pypy310-pp73-x86_64-linux-gnu.so | grep jpeg
                 U jpeg_CreateCompress@LIBJPEG_6.2
                 U jpeg_CreateDecompress@LIBJPEG_6.2
                 U jpeg_add_quant_table@LIBJPEG_6.2
000000000003b1f0 T jpeg_buffer_dest
000000000003ac60 T jpeg_buffer_src
                 U jpeg_destroy_compress@LIBJPEG_6.2
                 U jpeg_destroy_decompress@LIBJPEG_6.2
                 U jpeg_finish_compress@LIBJPEG_6.2
                 U jpeg_finish_decompress@LIBJPEG_6.2
                 U jpeg_read_header@LIBJPEG_6.2
                 U jpeg_read_scanlines@LIBJPEG_6.2
                 U jpeg_resync_to_restart@LIBJPEG_6.2
                 U jpeg_set_defaults@LIBJPEG_6.2
                 U jpeg_set_quality@LIBJPEG_6.2
                 U jpeg_simple_progression@LIBJPEG_6.2
                 U jpeg_start_compress@LIBJPEG_6.2
                 U jpeg_start_decompress@LIBJPEG_6.2
                 U jpeg_std_error@LIBJPEG_6.2
                 U jpeg_suppress_tables@LIBJPEG_6.2
                 U jpeg_write_marker@LIBJPEG_6.2
                 U jpeg_write_scanlines@LIBJPEG_6.2
000000000006b5e0 D libjpeg_turbo_version

In this case they agree, but... I'd have to try it. (This is why there are different wheels for different libc)

@egourgoulhon

…grade to 10.1.0   As discussed in https://groups.google.com/g/sage-devel/c/A4nY3bh1oPI @egourgoulhon Here we make`libjpeg` a new "recommended" dummy package with spkg- configure script -- same category as pandoc, ffmpeg, imagemagick, texlive, so it appears in the installation instructions in https://deploy-preview-36731--sagemath- tobias.netlify.app/html/en/installation/source#debian-ubuntu-package- installation (in the 3rd command line for each of the shown package systems). Use of libjpeg can be disabled using `./configure --without-system- libjpeg`. Everything done here would also be necessary if we decided to make libjpeg a normal standard/optional package; that is beyond the scope of this PR. We also refine the "CI Linux incremental" workflow, which determines what packages to uninstall (so that spkg-configure is tested) or to build (important for optional packages), based on changed files. Failures seen in https://github.com/sagemath/sage/actions/runs/691489095 0/job/18813211070?pr=36731#step:4:8 came from the workflow trying to build dummy packages. We suppress these now, as seen in https://github.c om/sagemath/sage/actions/runs/6915792754/job/18815135590?pr=36731#step:4 :8 Tests at https://github.com/mkoeppe/sage/actions/runs/6911535503 (Linux), https://github.com/mkoeppe/sage/actions/runs/6911535500 (macOS)    ### 📝 Checklist     - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies   URL: sagemath#36731 Reported by: Matthias Köppe Reviewer(s): Eric Gourgoulhon

vbraun · 2023-11-27T20:55:37Z

Imho this needs more testing than cramming it in at the last minute

mkoeppe · 2023-11-27T21:02:12Z

People who use Jupyter seem to consider it a severe regression from the previous release

mkoeppe · 2023-11-27T21:05:05Z

And this was tested on the full set of platforms, see ticket description.

egourgoulhon · 2023-11-27T21:07:40Z

People who use Jupyter seem to consider it a severe regression from the previous release

I confirm...

orlitzky · 2023-11-27T22:52:41Z

The issue wasn't just that you can't play with jpegs, but that some other functionality of pillow happens to be broken at the moment if you compile it without jpeg support, right?

orlitzky · 2023-11-27T22:54:42Z

@culler FYI I noticed that libjpeg has a slightly unusual license, that says,

(2) If only executable code is distributed, then the accompanying documentation must state that "this software is based in part on the work of the Independent JPEG Group".

egourgoulhon · 2023-11-27T23:22:43Z

The issue wasn't just that you can't play with jpegs, but that some other functionality of pillow happens to be broken at the moment if you compile it without jpeg support, right?

Yes indeed. For instance, as reported in https://groups.google.com/g/sage-devel/c/A4nY3bh1oPI/m/0lxrC67HAgAJ, the following code does not manipulate any jpeg but is broken in Sage 10.2.rc4:

from PIL import Image
plot(sin(x)).save("fig.png")
img = Image.open("fig.png")
img

For some reason, it yields

ValueError: Could not save to JPEG for display

while on the user side, we are not asking for any jpeg.
The same piece of code works nicely in Sage 10.1

orlitzky · 2023-11-29T03:00:29Z

while on the user side, we are not asking for any jpeg.
The same piece of code works nicely in Sage 10.1

This is probably fixed in pillow-10.1.0: python-pillow/Pillow#7259

If so, maybe an upgrade to pillow is a less intrusive fix?

build/pkgs/pillow/spkg-install.in: Allow discovery of libjpeg

15f0e90

mkoeppe self-assigned this Nov 18, 2023

build/pkgs/pillow: Update to 10.1.0

565bc0d

mkoeppe changed the title ~~build/pkgs/pillow/spkg-install.in: Allow discovery of libjpeg~~ build/pkgs/pillow: Allow discovery of libjpeg, upgrade to 10.1.0 Nov 18, 2023

mkoeppe added the c: packages: standard label Nov 18, 2023

Matthias Koeppe added 3 commits November 17, 2023 18:07

build/pkgs/_recommended/dependencies: Add libjpeg

b06fbad

build/pkgs/libjpeg: New optional dummy package

13b1115

build/pkgs/libjpeg/SPKG.rst: Explain why useful

a6c4ad4

mkoeppe added the s: needs review label Nov 18, 2023

build/pkgs/libjpeg/spkg-configure.m4: New

49275fc

Matthias Koeppe added 4 commits November 17, 2023 20:52

build/pkgs/pillow/spkg-install.in: Remove outdated comment

1d8f416

Pass SAGE_HAVE_LIBJPEG from configure to pillow spkg-install

c56760c

.github/workflows/ci-linux-incremental.yml: Do not try to build dummy…

0dcbeba

… packages

.github/workflows/ci-linux-incremental.yml: Do not try to build _... …

30d5a7e

…packages

mkoeppe added this to the sage-10.2 milestone Nov 18, 2023

mkoeppe added the p: critical / 2 label Nov 18, 2023

mkoeppe requested review from egourgoulhon, dimpase and orlitzky November 18, 2023 19:45

mkoeppe added p: blocker / 1 and removed p: critical / 2 labels Nov 19, 2023

egourgoulhon approved these changes Nov 20, 2023

View reviewed changes

egourgoulhon added s: positive review and removed s: needs review labels Nov 20, 2023

vbraun merged commit 202a245 into sagemath:develop Nov 30, 2023
32 of 59 checks passed

github-actions bot removed the s: positive review label Nov 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`build/pkgs/pillow`: Allow discovery of libjpeg, upgrade to 10.1.0 #36731

`build/pkgs/pillow`: Allow discovery of libjpeg, upgrade to 10.1.0 #36731

mkoeppe commented Nov 18, 2023 •

edited

Loading

orlitzky commented Nov 18, 2023

mkoeppe commented Nov 18, 2023

mkoeppe commented Nov 18, 2023 •

edited

Loading

mkoeppe commented Nov 18, 2023

orlitzky commented Nov 18, 2023

mkoeppe commented Nov 18, 2023

orlitzky commented Nov 18, 2023

mkoeppe commented Nov 18, 2023

mkoeppe commented Nov 18, 2023

mkoeppe commented Nov 18, 2023

github-actions bot commented Nov 18, 2023

egourgoulhon left a comment

mkoeppe commented Nov 20, 2023

dimpase commented Nov 20, 2023

culler commented Nov 20, 2023 via email

mkoeppe commented Nov 20, 2023

culler commented Nov 20, 2023 via email

mkoeppe commented Nov 20, 2023 •

edited

Loading

orlitzky commented Nov 20, 2023

mkoeppe commented Nov 20, 2023

culler commented Nov 20, 2023 via email

mkoeppe commented Nov 20, 2023

orlitzky commented Nov 20, 2023

vbraun commented Nov 27, 2023

mkoeppe commented Nov 27, 2023

mkoeppe commented Nov 27, 2023

egourgoulhon commented Nov 27, 2023

orlitzky commented Nov 27, 2023

orlitzky commented Nov 27, 2023

egourgoulhon commented Nov 27, 2023

orlitzky commented Nov 29, 2023

build/pkgs/pillow: Allow discovery of libjpeg, upgrade to 10.1.0 #36731

build/pkgs/pillow: Allow discovery of libjpeg, upgrade to 10.1.0 #36731

Conversation

mkoeppe commented Nov 18, 2023 • edited Loading

📝 Checklist

⌛ Dependencies

orlitzky commented Nov 18, 2023

mkoeppe commented Nov 18, 2023

mkoeppe commented Nov 18, 2023 • edited Loading

mkoeppe commented Nov 18, 2023

orlitzky commented Nov 18, 2023

mkoeppe commented Nov 18, 2023

orlitzky commented Nov 18, 2023

mkoeppe commented Nov 18, 2023

mkoeppe commented Nov 18, 2023

mkoeppe commented Nov 18, 2023

github-actions bot commented Nov 18, 2023

egourgoulhon left a comment

Choose a reason for hiding this comment

mkoeppe commented Nov 20, 2023

dimpase commented Nov 20, 2023

culler commented Nov 20, 2023 via email

mkoeppe commented Nov 20, 2023

culler commented Nov 20, 2023 via email

mkoeppe commented Nov 20, 2023 • edited Loading

orlitzky commented Nov 20, 2023

mkoeppe commented Nov 20, 2023

culler commented Nov 20, 2023 via email

mkoeppe commented Nov 20, 2023

orlitzky commented Nov 20, 2023

vbraun commented Nov 27, 2023

mkoeppe commented Nov 27, 2023

mkoeppe commented Nov 27, 2023

egourgoulhon commented Nov 27, 2023

orlitzky commented Nov 27, 2023

orlitzky commented Nov 27, 2023

egourgoulhon commented Nov 27, 2023

orlitzky commented Nov 29, 2023

`build/pkgs/pillow`: Allow discovery of libjpeg, upgrade to 10.1.0 #36731

`build/pkgs/pillow`: Allow discovery of libjpeg, upgrade to 10.1.0 #36731

mkoeppe commented Nov 18, 2023 •

edited

Loading

mkoeppe commented Nov 18, 2023 •

edited

Loading

mkoeppe commented Nov 20, 2023 •

edited

Loading