Dynamically link FriBiDi instead of Raqm #5062

nulano · 2020-11-25T19:58:03Z

This is likely to fix issues #3066 and #4225.

Includes Raqm and a FriBiDi shim in src/thirdparty. Raqm is modified to use the shim ~~and for compatibility with C89~~ (edit: reverted because Raqm is no longer built by default).

I've checked that Raqm/FriBiDi/HarfBuzz are being loaded correctly in all of the Windows, Ubuntu, and Docker GHA jobs.
~~I'm not sure what is missing to get HarfBuzz detected on macOS~~ edit: merging master fixed this somehow. ~~Travis is failing to compile Raqm because it has HarfBuzz 1.0.1 which is too old~~ edit: disabled dynamic loading by default.

Is this the way forward to resolving the Raqm/FreeType crashes? I haven't tried building wheels to see if the issues are actually resolved.

This effectively removes dynamic loading as the default option. It now needs to be explicitly enabled with --vendor-raqm --vendor-fribidi, e.g. when building wheels. A side effect of these changes is that Windows users now need fribidi.dll instead of libraqm.dll.

~~TODO:~~

~~I intended to add a detailed message for which Raqm version is selected in setup.py.~~
~~PR in pillow-wheels adding HarfBuzz and enabling dynamic linking.~~ Update config for FriBiDi dynamic linking pillow-wheels#181

wiredfool · 2021-01-02T10:02:42Z

@nulano is there a way that this can go into the current release?

setup.py

nulano · 2021-01-02T11:05:45Z

setup.py

+                    _dbg("--enable-raqm implies --enable-freetype")
+                    self.feature.required.add("freetype")
+        for x in ("raqm", "fribidi"):
+            if getattr(self, f"system_{x}"):


This PR is currently set up to default to building vendored libraries. Since this is not fully functional yet (e.g. macOS), I think the best way to get this into the release would be to invert this for now (perhaps using --_vendored-...) and re-evaluate later. Thoughts?

nulano · 2021-01-02T11:14:14Z

I've triggered a run of pillow-wheels on this branch to make sure this actually fixes the issue: https://github.com/nulano/pillow-wheels/actions/runs/457534488

wiredfool · 2021-01-02T12:27:37Z

We're getting seemingly unrelated errors with a bunch of the runs on that pillow-wheels push. It looks like that might be a tiff 4.2.0 issue?

radarhere · 2021-01-02T12:30:29Z

If you're talking about 'RuntimeError: Error setting from dictionary', then that should have been fixed by 6b21a96

nulano · 2021-01-02T12:31:40Z

We're getting seemingly unrelated errors with a bunch of the runs on that pillow-wheels push. It looks like that might be a tiff 4.2.0 issue?

If you're talking about 'RuntimeError: Error setting from dictionary', then that should have been fixed by 6b21a96

I changed the repo url to my fork which has a slightly outdated master branch.

wiredfool · 2021-01-02T12:34:11Z

Ah, ok.

nulano · 2021-01-02T13:20:54Z

I think this is (mostly) ready for review, but it will require a second PR in pillow-wheels, and I'm having trouble getting HarfBuzz to compile there.

wiredfool · 2021-01-02T13:22:27Z

These wheels are looking good against my earlier repro. (3.8-manylinux1 x86_64) Valgrind is giving a clean report to 10k iterations, where it used to fail quickly.

nulano · 2021-01-02T13:24:08Z

These wheels are looking good against my earlier repro. (3.8-manylinux1 x86_64) Valgrind is giving a clean report to 10k iterations, where it used to fail quickly.

If you used the wheels from my link above, I'm pretty sure they are missing HarfBuzz and hence also Raqm:

... will require a second PR in pillow-wheels, and I'm having trouble getting HarfBuzz to compile there.

wiredfool · 2021-01-02T13:26:20Z

ah, not a good test then.

nulano · 2021-01-02T16:15:05Z

I finally got it to compile! (tests failed because it detected Raqm which was not on the expected feature list)

nulano · 2021-01-02T17:50:05Z

I've tested the wheels and they do appear to have Raqm support, and I can no longer reproduce a crash with 50k iterations.

I'll open a PR in pillow-wheels shortly. Edit: python-pillow/pillow-wheels#181

wiredfool · 2021-01-03T13:23:04Z

Looks good to me:

--------------------------------------------------------------------
Pillow 8.1.0.dev0
Python 3.8.5 (default, Jul 28 2020, 12:59:40)
       [GCC 9.3.0]
--------------------------------------------------------------------
Python modules loaded from /home/ubuntu/vpy38-dbg/lib/python3.8/site-packages/PIL
Binary modules loaded from /home/ubuntu/vpy38-dbg/lib/python3.8/site-packages/PIL
--------------------------------------------------------------------
--- PIL CORE support ok, compiled for 8.1.0.dev0
--- TKINTER support ok
--- FREETYPE2 support ok, loaded 2.10.4
--- LITTLECMS2 support ok, loaded 2.10
--- WEBP support ok, loaded 1.1.0
--- WEBP Transparency support ok
--- WEBPMUX support ok
--- WEBP Animation support ok
--- JPEG support ok, compiled for 9.0
--- OPENJPEG (JPEG2000) support ok, loaded 2.4.0
--- ZLIB (PNG/ZIP) support ok, loaded 1.2.11
--- LIBTIFF support ok, loaded 4.2.0
--- RAQM (Bidirectional Text) support ok, loaded 0.7.1, fribidi 1.0.8, harfbuzz 2.7.4
*** LIBIMAGEQUANT (Quantization method) support not installed
--- XCB (X protocol) support ok
--------------------------------------------------------------------
Running selftest:
--- 58 tests passed.

All ordinary tests pass, as well as the valgrind run against the test case in #4225. As a comparison, the valgrind case still fails against the shipped 8.1.0 binaries.

…atibility" This reverts commit b3cfe73.

setup.py

wiredfool · 2021-03-15T22:14:34Z

Does this need to merge before the wheels PR goes in? Is there anything that needs to happen on this?

nulano · 2021-03-25T01:23:01Z

Does this need to merge before the wheels PR goes in? Is there anything that needs to happen on this?

Missed the GitHub notification for this...

This PR needs to be merged before the wheels PR to make that one pass.

Looking over the changes again I noticed and fixed a few small issues, but I don't think there is anything else needed (other than release notes).

wiredfool · 2021-03-27T16:28:57Z

OK, Merging.

Still need release notes: #5287

nulano · 2021-03-29T15:59:31Z

OK, Merging.

Still need release notes: #5287

In #5365.

nulano mentioned this pull request Nov 25, 2020

Crash with an error: munmap_chunk(): invalid pointer #4225

Closed

hugovk force-pushed the master branch from 747e44b to 9e3ad5e Compare December 18, 2020 20:12

radarhere added the Needs Rebase label Dec 24, 2020

radarhere removed the Needs Rebase label Jan 2, 2021

radarhere reviewed Jan 2, 2021

View reviewed changes

setup.py Outdated Show resolved Hide resolved

nulano commented Jan 2, 2021

View reviewed changes

radarhere mentioned this pull request Jan 2, 2021

Release Pillow 8.1.0 on January 2, 2021 #4997

Closed

21 tasks

This comment has been minimized.

Sign in to view

Zverik mentioned this pull request Jan 2, 2021

OSError: invalid face handle #3066

Closed

nulano mentioned this pull request Jan 2, 2021

Update config for FriBiDi dynamic linking python-pillow/pillow-wheels#181

Merged

nulano marked this pull request as ready for review January 2, 2021 18:02

nulano added 6 commits January 4, 2021 23:57

add raqm to thirdparty directory

e5e5761

use FriBiDi shim in Raqm

8bc1ff3

add Raqm license and readme

9e5fc13

add option to statically link fribidi, version info

5cd688f

GHA: fix windows build for dynamic fribidi

d4403be

replace tabs in thirdparty libs

3386a9c

nulano added 7 commits January 5, 2021 00:07

support FriBiDi<1.0

b4a57d6

raqm: fix FriBiDi<1 support

9c17843

test

db0dad9

Revert "raqm: avoid declaring variables in for statement for C89 comp…

8c02e38

…atibility" This reverts commit b3cfe73.

disable Raqm/FriBiDi vendoring by default, except in Windows tests

43bde01

can't use underscore prefix for distutils options

0488a27

lint

aae9411

radarhere force-pushed the fribidi-link branch from 1b05926 to aae9411 Compare January 4, 2021 13:07

dlukes mentioned this pull request Jan 28, 2021

Memory errors on Fedora 29 amueller/word_cloud#516

Closed

Merge branch 'master' into fribidi-link

9b56833

radarhere reviewed Feb 27, 2021

View reviewed changes

setup.py Outdated Show resolved Hide resolved

nulano added 2 commits February 27, 2021 15:14

fix merge

70fb148

add Raqm build configuration info to build summary

e4cc422

radarhere mentioned this pull request Mar 20, 2021

draw.text((50,5),str(d['类型']), font=font25bd, fill='#eaca67') #5346

Closed

nulano added 2 commits March 25, 2021 00:41

fix support for old versions of Raqm

5e61c18

avoid unused variable warnings

c718cc6

wiredfool merged commit 3addd7d into python-pillow:master Mar 27, 2021

nulano mentioned this pull request Mar 29, 2021

Document Raqm/FriBiDi linking changes #5365

Merged

MatthiasValvekens mentioned this pull request Apr 14, 2021

Coredump when importing pyhanko in pdfarranger MatthiasValvekens/pyHanko#13

Closed

nulano mentioned this pull request Jul 1, 2021

Release Pillow 8.3.0 on July 1, 2021 #5413

Closed

23 tasks

radarhere mentioned this pull request Jul 6, 2021

Release 8.3.1 on July 6, 2021 #5578

Closed

12 tasks

nulano deleted the fribidi-link branch September 18, 2021 21:18

nulano mentioned this pull request Sep 18, 2021

Remove raqm.cmake #5722

Merged

nulano mentioned this pull request Nov 5, 2021

Use the Windows method to get TCL functions on Cygwin #5807

Merged

nulano mentioned this pull request Mar 31, 2022

Fix loading FriBiDi on Alpine #6165

Merged

radarhere mentioned this pull request May 30, 2022

Crash when importing ImageFont #6314

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamically link FriBiDi instead of Raqm #5062

Dynamically link FriBiDi instead of Raqm #5062

nulano commented Nov 25, 2020 •

edited

wiredfool commented Jan 2, 2021

nulano Jan 2, 2021

nulano commented Jan 2, 2021

wiredfool commented Jan 2, 2021

radarhere commented Jan 2, 2021

nulano commented Jan 2, 2021

wiredfool commented Jan 2, 2021

nulano commented Jan 2, 2021

wiredfool commented Jan 2, 2021

nulano commented Jan 2, 2021 •

edited

wiredfool commented Jan 2, 2021

This comment has been minimized.

nulano commented Jan 2, 2021

nulano commented Jan 2, 2021 •

edited

wiredfool commented Jan 3, 2021

wiredfool commented Mar 15, 2021

nulano commented Mar 25, 2021

wiredfool commented Mar 27, 2021

nulano commented Mar 29, 2021

Dynamically link FriBiDi instead of Raqm #5062

Dynamically link FriBiDi instead of Raqm #5062

Conversation

nulano commented Nov 25, 2020 • edited

wiredfool commented Jan 2, 2021

nulano Jan 2, 2021

Choose a reason for hiding this comment

nulano commented Jan 2, 2021

wiredfool commented Jan 2, 2021

radarhere commented Jan 2, 2021

nulano commented Jan 2, 2021

wiredfool commented Jan 2, 2021

nulano commented Jan 2, 2021

wiredfool commented Jan 2, 2021

nulano commented Jan 2, 2021 • edited

wiredfool commented Jan 2, 2021

This comment has been minimized.

nulano commented Jan 2, 2021

nulano commented Jan 2, 2021 • edited

wiredfool commented Jan 3, 2021

wiredfool commented Mar 15, 2021

nulano commented Mar 25, 2021

wiredfool commented Mar 27, 2021

nulano commented Mar 29, 2021

nulano commented Nov 25, 2020 •

edited

nulano commented Jan 2, 2021 •

edited

nulano commented Jan 2, 2021 •

edited