fonttools / skia-pathops

Python bindings for the Skia library's Path Ops
https://skia.org/docs/dev/present/pathops/
BSD 3-Clause "New" or "Revised" License
47 stars 14 forks source link

Add support for finding skia using pkg-config #54

Closed naveen521kk closed 2 years ago

naveen521kk commented 2 years ago

This will allow building skia-pathops using system skia or skia compiled externally. This will not fail/show an error message when either pkg-config isn't available or skia isn't available through pkg-config. See https://github.com/msys2/MINGW-packages/pull/10195 where I have tried to build skia separately and provided a pkg-config file for flags necessary.

anthrotype commented 2 years ago

Thanks for this. I'm open to modify the setup to allow for this use case. However, have you noticed that we already have a couple of environment variables that control whether to BUILD_SKIA_FROM_SOURCE and pass a custom SKIA_LIBRARY_DIR with the path to precompiled libskia? I wonder if, instead of calling pkg-config from setup.py as subprocess like you do here, you could instead try to reuse these env vars, perhaps defining additional ones for cflags and include directories; so you'd call pkg-config yourself externally before building skia-pathops to obtain the compiler/linker flags and then export the respective env vars?

anthrotype commented 2 years ago

actually, you know that setuptools' python setup.py build_ext already has -L, -l and -I options that do exactly that? Maybe it could be enough to set BUILD_SKIA_FROM_SOURCE=0 (to prevent having the embedded libskia rebuilt from source) and then pass the right combination of -L, -I and -l options to build_ext.

naveen521kk commented 2 years ago

However, have you noticed that we already have a couple of environment variables that control whether to BUILD_SKIA_FROM_SOURCE and pass a custom SKIA_LIBRARY_DIR with the path to precompiled libskia? I wonder if, instead of calling pkg-config from setup.py as subprocess like you do here, you could instead try to reuse these env vars, perhaps defining additional ones for cflags and include directories; so you'd call pkg-config yourself externally before building skia-pathops to obtain the compiler/linker flags and then export the respective env vars?

Yeah, I have seen them. I just wanted a cleaner way rather than using environment variables and pkg-config is one of the common ones to find libraries with cflags etc. That way I would be able to pip install directly and not worry about setting env vars.

With the current setup, I think setting BUILD_SKIA_FROM_SOURCE=0 and CFLAGS environment variable to add necessary include libraries should do I guess.

naveen521kk commented 2 years ago

LGTM, thanks. Let me know when you're done and I'll merge

Thanks for the review. This PR is ready :). (I have tested this with https://github.com/msys2/MINGW-packages/pull/10195)

naveen521kk commented 2 years ago

Thanks 😄

anthrotype commented 2 years ago

Thank you! noob qustions, just out of curiousity, how common it is to have libskia available system-wide? Do you know of other distros, i.e. besides msys2, that do that?

naveen521kk commented 2 years ago

noob qustions, just out of curiousity, how common it is to have libskia available system-wide? Do you know of other distros, i.e. besides msys2, that do that?

Dunno, I haven't seen any other distro with libskia available system-wide but there exists an arch package for skia-sharp (which I guess is a different project). I think MSYS2 may be the first one to do so 😄 (which I guess should be fine).

anthrotype commented 2 years ago

I just tagged a new release including this https://github.com/fonttools/skia-pathops/releases/tag/v0.7.2

naveen521kk commented 2 years ago

Thanks.

khaledhosny commented 2 years ago

AFAIK, Skia does not have a stable API or ABI, which is probably why no distribution packages it.