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

Re-evaluate bundling dynamic library dependencies in manylinux wheels. #284

Closed
theacodes opened this issue Nov 26, 2024 · 16 comments · May be fixed by #285
Closed

Re-evaluate bundling dynamic library dependencies in manylinux wheels. #284

theacodes opened this issue Nov 26, 2024 · 16 comments · May be fixed by #285

Comments

@theacodes
Copy link

Overview

At the moment, skia-python includes several dynamic library dependencies in its manylinux wheel:

libbz2-a273e504.so.1.0.6
libfontconfig-42c558d2.so.1.11.1
libfreetype-c0e61f0c.so.6.14.0
libpng15-ce838cd1.so.15.13.0
libuuid-f64cda11.so.1.3.0

This is convenient since it reduces the number of system packages that a user needs to install, but it has some serious drawbacks:

  1. These libraries are significantly out of date compared to versions on modern systems (see table below), they likely contain bugs, vulnerabilities (see Vulnerable shared libraries might make skia-python vulnerable. Can you help upgrade to patch versions? #175), or compatibility issues with Skia.
  2. Skia explicitly recommends against static linking or bundling dynamic libraries for production builds.
  3. This can cause issues with libraries like libfontconfig that depend on configuration files found in the system. For example, modern Debian installs a config file at /usr/share/fontconfig/conf.avail/05-reset-dirs-sample.conf that is incompatible with the older version of libfontconfig bundled with skia-python, causing a warning:

Fontconfig warning: "/usr/share/fontconfig/conf.avail/05-reset-dirs-sample.conf", line 6: unknown element "reset-dirs"

How these get bundled

The present build system for skia-python causes this through two location. First, Skia is told not to use system packages when being built:

skia_use_system_libjpeg_turbo=false
skia_use_system_libwebp=false
skia_use_system_libpng=false
skia_use_system_icu=false
skia_use_system_harfbuzz=false
skia_use_system_freetype2=false

Second, cibuildwheel will run auditwheel on any manylinux wheel. This where these dependencies get grafted into the wheel.

Possible solutions

  1. Do nothing, possibly document that manylinux wheels will use outdated versions of these libraries.
  2. Drop manylinux2014 in favor of a newer PEP600 version based on glibc, possibly manylinux_2_36 (which covers contemporary distros like Debian 12). This would result in newer version being bundled, but wouldn't completely resolve the issue.
  3. Change the configuration to dynamically link to system versions of these libraries. This would require users to ensure these packages are installed (which is already done in the docs). This would lead to some users running into issues if they blindly install skia-python without the necessary packages.

Version comparison

Library Bundled Debian Trixie
libbz2 1.0.6 1.0.8-6
libfontconfig 1.11.1 2.15.0-1.1
libfreetype 6.14.0 6.20.2
libpng 1.5.13.0 1.6.44-2
libuuid 1.3.0 2.40.2-11
@HinTak
Copy link
Collaborator

HinTak commented Nov 26, 2024

Item 3 already exists, I personally uses the releases from https://github.com/HinTak/skia-building-fun when I switch skia upward. It still needs be static - as skia-python's pybind11 code can't cross dynamic library boundaries, but there is already an answer of NOT bundling any system libraries at all - it is just that AFAIK only I am building skia that way. (It is quicker for me to switch skia when they are properly separate).

2 is a possibility, but it is a matter of what's the oldest we want to support - there are people still asking for 87.x you know, 4 years after Google stopped supporting it.

As I comment in #175 I am not sure the vulnerability is real - the vulnerability refers to packages, which consists of multiple libraries, and not all of them are applicable. I.e. while package X is known to be vulnerable, package x have 10 libraries, and we are only bundling one of 10 (say). The vulnerability may well be one or two of the other 9, within the same package.

I am tempted to close this as duplicate of #175, or the other way round.

@HinTak
Copy link
Collaborator

HinTak commented Nov 26, 2024

Also, we don't actually use the bundled old freetype nor the bundled old libpng - we use some of the latest features in freetype so actually use
our old up-to-date freetype and libpng from skia submodules.
skia_use_system_libpng=false
skia_use_system_freetype2=false. The old libraries are indirect from the old fontconfig, and not themselves used.

@HinTak
Copy link
Collaborator

HinTak commented Nov 26, 2024

Apparently it is just adding icuuc, webp, jpeg, webpdemux, webpmux, harfbuzz-subset and adding freetypr header locations, to build against a "everything from system" static skia.

@HinTak
Copy link
Collaborator

HinTak commented Nov 27, 2024

See also #257 . I think the issue covered here is a combination of #257 and #175 . Please feel free to improve #257 , and adding (alternative from system) of "icuuc, webp, jpeg, webpdemux, webpmux, harfbuzz-subset and adding freetypr header locations, to build against a "everything from system" static skia." to setup.py. I have a semi-permanent diff in my private branch (hence all my pulls are called m1xxx public) and I can post that, if it helps .

@HinTak HinTak closed this as completed Nov 27, 2024
@theacodes
Copy link
Author

Thanks for the detailed explanation of why things are this way, I really appreciate it!

Also, we don't actually use the bundled old freetype nor the bundled old libpng - we use some of the latest features in freetype so actually use our old up-to-date freetype and libpng from skia submodules. skia_use_system_libpng=false skia_use_system_freetype2=false. The old libraries are indirect from the old fontconfig, and not themselves used.

Interesting! If it's using system freetype anyway, why not use system fontconfig instead of bundling it?

Item 3 already exists, I personally uses the releases from https://github.com/HinTak/skia-building-fun when I switch skia upward.

This repo is super interesting and useful, thank you.

2 is a possibility, but it is a matter of what's the oldest we want to support - there are people still asking for 87.x you know, 4 years after Google stopped supporting it.

Haha I don't doubt it! Maybe for Python 3.13+ wheels we could step up the manylinux version?

@HinTak
Copy link
Collaborator

HinTak commented Nov 27, 2024

Your understanding of what is and what isn't bundled is in the opposite sense, I think. Skia/Chrome uses freetype on all platforms (and Google contributes to freetype development) to use freetype to load fonts on mac os which apple does not support, and load fonts on windows which Microsoft does not support. Hence skia has a build option to bundle freetype ("use system freetype false" means use the private up-to-date static copy from google repo as a submodule - it is sometimes ahead of "latest freetype release", and definitely ahead of most linux's distro's shipped freetype) within skia. The dependency on fontconfig is only on Linux. There is 2nd and different bundling process by auditwheel, which is what you see. Basically the 2nd bundling bundles what is NOT bundled by the first. What you observe is the 2nd bundling; what is written in script/build_* is about the first.

libpng is a dependency of freetype (to load apple emoji's on MS platforms, if you want a specific usage). Hence both are/can be bundled by the first bundle, and their usage are up to date in current skia-python, although it does not look like it. Fontconfig is an optional dependency applies only to Linux, so it is bundled by the 2nd stage (and together with what it depends, the older copy of freetype & libpng).

@HinTak
Copy link
Collaborator

HinTak commented Nov 27, 2024

@kyamagu switching manylinux for the new python builds is an interesting idea - since we broke into two build stanza in m132 - one for 3.8 + 3.9, another for 3.10,11,12,13 (due to github's build machinery not being able to cope with both new and old in one), it isn't too hard to split out say 3.13 - in fact we can probably go with the oldest Linux box that has 3.10 for the latter group of builds, since they are now separate?

@HinTak
Copy link
Collaborator

HinTak commented Nov 27, 2024

Python 3.10 dev began in May 2020, released 2021 Oct.

2.30 August 2019
2.31 February 2020
2.32 August 2020
2.33 February 2021
2.34 August 2021

We should be able to say python 3.10 is accompanied by at least glibc 2.31? @kyamagu - it is of course possible to put extremely new python on extremely old glibc system... but we are trying to guess what distro combinations are on initial shipping, I think.

@HinTak
Copy link
Collaborator

HinTak commented Nov 27, 2024

2_28 will be the default in 6 months - https://cibuildwheel.pypa.io/en/stable/options/#linux-image - so do nothing, this and #175 will probably fix itself.

@HinTak
Copy link
Collaborator

HinTak commented Nov 27, 2024

@kyamagu considering how long #175 been around, and that cibuildwheel is switching its default in 6 months, we can switch a little earlier? 2.28 is still 6 years old. See #285 pull.

@HinTak
Copy link
Collaborator

HinTak commented Nov 27, 2024

By the time we merge for the next release (134 possibly), it would be only about 3 months ahead.

@kyamagu
Copy link
Owner

kyamagu commented Nov 27, 2024

@HinTak Thanks for the discussion. Yes, I think switching to 2_28 earlier is a reasonable approach to take at this point. Go with the PR #285

@theacodes
Copy link
Author

Thanks for explaining, @HinTak!

I'm still confused on one aspect - if the manylinux builds are indeed using system libraries, then why is it seemingly using the wheel's libfontconfig instead of the systems? If it were using the system version, the warning about incompatible configuration files wouldn't happen:

Fontconfig warning: "/usr/share/fontconfig/conf.avail/05-reset-dirs-sample.conf", line 6: unknown element "reset-dirs"

Also, if the wheel libraries aren't being used, it would probably be worthwhile to not include them since they make the .whl larger.

@HinTak
Copy link
Collaborator

HinTak commented Dec 1, 2024

@theacodes "up to date" != "system". There are 3 sets of libraries, one from skia's submodules, which is always up to date and occasionally have extra google-specific stuff being upstreamed, and sometimes ahead of releases - google has a forked freetype repo and forked libpng repo, but not fontconfig; (2) the build host, which is manylinux2014 and soon to be manylinux2_28, (3) the runtime host, I.e. your computer.

Fontconfig is from (2), which is why it isn't 100% happy working in (3). Freetype and libpng used to be from (2) too, but switched to (1) about a year ago. What you see them bundled is indirect from fontconfig. Since the build host's copy of fontconfig does not know it will have (1) eventually, the fontconfig library on the build host holds a reference to the build host's freetype, which then get bundled.

The benefit of switching from (2) to (1) is that things are more up to date - but it makes larger binaries, because the skia-python...so file itself contains a hidden, private, and up to date copy of freetype and libpng which you don't see.

@HinTak
Copy link
Collaborator

HinTak commented Dec 1, 2024

@theacodes also you probably don't realise: (2) is not built on the build host, but just taken from system locations of the build host. That's why they exist as separate files as seen on (3). (1) is built and included in skia-....so and not visible.

@theacodes
Copy link
Author

Ah, gotcha. That makes sense. Thank you. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants