libvips / pyvips

python binding for libvips using cffi
MIT License
649 stars 50 forks source link

Drop support for Python 2.7, migrate to `pyproject.toml` #445

Closed kleisauke closed 2 months ago

kleisauke commented 10 months ago

See the individual commits for more details.

jcupitt commented 10 months ago

Wow! Very nice Kleis, I'll have a poke about.

jcupitt commented 10 months ago

Quick thoughts:

kleisauke commented 10 months ago

Major bump to 3.0.0 sounds good to me. I had a quick go building wheels for use on the most common platforms, see: https://github.com/kleisauke/pyvips/commit/615969f910406ca40e8dc9002908e801c25d04ce (I'll probably open a separate PR for this)

It seems to work well, see for example the produced wheels listed at https://github.com/kleisauke/pyvips/actions/runs/7437832687. However, I'm uncertain about how to distribute this without affecting users who installed libvips globally. NetVips has a special NetVips.Native NuGet package for this reason. I'm still figuring out the best way to do something similar in Python.

kleisauke commented 10 months ago

... one idea is to just distribute it along with the PyPI package and users who want to use a globally installed libvips should install pyvips with --no-binary :all:.

SteveHawk commented 10 months ago

Nice work!

For shipping libvips binary, another idea is to create a separate PyPI package for libvips, and mark that as pyvips's optional dependency. If user wants to install pyvips along with provided libvips, they can type something like pip install pyvips[libvips]. And pip install pyvips still only installs pyvips without libvips, which doesn't break existing behavior.

As an example, PyMuPDF, a PDF library that I used, have been doing this lately, putting C/C++ MuPDF shared libraries into a separate package called PyMuPDFb, as per the comment in their build script. Maybe we can do this in a similar way?

But I'm not sure if it's possible to achieve cffi API mode this way, or are we stuck in ABI mode. PyMuPDF uses swig rather than cffi for binding, which I'm not familiar with.

jcupitt commented 10 months ago

PyMuPDF uses swig rather than cffi for binding, which I'm not familiar with.

I used to use SWIG -- it's a nice thing, but it's a C++ compile at install time, so it's more like API mode.

kleisauke commented 10 months ago

For shipping libvips binary, another idea is to create a separate PyPI package for libvips, and mark that as pyvips's optional dependency.

Indeed, this is a clever idea. I had another attempt with commit https://github.com/kleisauke/pyvips/commit/0c0921511f19f1ff9558426052341cd573545e2a which adds a pyvips-prebuilt package. This allows users to install both pyvips and a pre-built libvips, along with its dependencies, by using:

$ pip install --user pyvips[prebuilt]

This also seems to work well, I tested this by downloading wheels-ubuntu-latest.zip from: https://github.com/kleisauke/pyvips/actions/runs/7464702812

And by issuing the following commands:

$ pkg-config --exists --print-errors vips
Package vips was not found in the pkg-config search path.
Perhaps you should add the directory containing `vips.pc'
to the PKG_CONFIG_PATH environment variable
Package 'vips', required by 'virtual:world', not found
$ pip install --user pyvips
$ python -c "import pyvips; print(pyvips.API_mode)" 
ImportError: libvips.so.42: cannot open shared object file: No such file or directory
[...]
$ unzip wheels-ubuntu-latest.zip
Archive:  wheels-ubuntu-latest.zip
  inflating: pyvips_prebuilt-8.15.0-cp37-abi3-manylinux_2_28_aarch64.whl  
  inflating: pyvips_prebuilt-8.15.0-cp37-abi3-manylinux_2_28_x86_64.whl  
  inflating: pyvips_prebuilt-8.15.0-cp37-abi3-musllinux_1_2_aarch64.whl  
  inflating: pyvips_prebuilt-8.15.0-cp37-abi3-musllinux_1_2_x86_64.whl  
$ pip install --user pyvips_prebuilt-8.15.0-cp37-abi3-manylinux_2_28_x86_64.whl
Processing ./pyvips_prebuilt-8.15.0-cp37-abi3-manylinux_2_28_x86_64.whl
Installing collected packages: pyvips-prebuilt
Successfully installed pyvips-prebuilt-8.15.0
$ python -c "import pyvips; print(pyvips.API_mode)"
True
kleisauke commented 10 months ago

... also seems to work on Windows.

PS> pip install --user pyvips
PS> python -c "import pyvips; print(pyvips.API_mode)"
[...]
ModuleNotFoundError: No module named '_libvips'
[...]
OSError: cannot load library 'libgobject-2.0-0.dll': error 0x7e. [...]
PS> Expand-Archive wheels-windows-latest.zip
PS> pip install --user wheels-windows-latest/pyvips_prebuilt-8.15.0-cp37-abi3-win_amd64.whl
Processing c:\users\kleisauke\downloads\wheels-windows-latest\pyvips_prebuilt-8.15.0-cp37-abi3-win_amd64.whl
Installing collected packages: pyvips-prebuilt
Successfully installed pyvips-prebuilt-8.15.0
PS> python -c "import pyvips; print(pyvips.API_mode)"
True
jcupitt commented 8 months ago

I tried on macos and it's failing with this error:

/var/folders/pz/gm0pbl0d03x481qvb2dqppb40000gn/T/tmpgc1_hsm5.build-temp/_libvips.c:5713:28:
error: incompatible function pointer types 
passing 'void *(*)(uint64_t, void *, void *)' (aka 'void *(*)(unsigned long long, void *, void *)') 
to parameter of type 'VipsTypeMap2Fn' (aka 'void *(*)(unsigned long, void *, void *)') [-Wincompatible-function-pointer-types]
        return vips_type_map(x0, x1, x2, x3);

ie.:

Changing vdecls.py to do this:

    # we need the glib names for these types
    code = '''
        typedef unsigned int guint32;
        typedef int gint32;
        typedef unsigned long guint64;
        typedef long gint64;
    '''

rather than using uint64_t etc. fixes it, though I don't know if that's correct on all platforms :(

kleisauke commented 8 months ago

Has that been tested with GLib 2.80.0 from Homebrew? If so, I think PR https://github.com/Homebrew/homebrew-core/pull/166473 might fix that. That is, it ensures that (u)int64_t and g(u)int64 are aligned.

#include <cstdint>
#include <type_traits>
#include <glib.h>

int main()
{
    static_assert(std::is_same<int64_t, gint64>::value == true, "gint64 should match int64_t");
    static_assert(std::is_same<uint64_t, guint64>::value == true, "guint64 should match uint64_t");
    return 0;
}
jcupitt commented 8 months ago

Oh great! Yes, that should fix it too.

kleisauke commented 7 months ago

(temporarily marking as draft to split some commits to separate PRs)

jcupitt commented 2 months ago

Oooof I'm finally back on this, sorry for the huge delay.

Shall we merge and do any more small fixes in the build-up to pyvips 3.0?

jcupitt commented 2 months ago

Ah let's just do it, and do any fixing up necessary in subsequent PRs as we head to pyvips 3.0. Thank you again for doing this work, Kleis!

kleisauke commented 2 months ago

No problem! Thanks for merging.

kleisauke commented 2 months ago

As for the subsequent fixes, I wasn't entirely sure if this PR could cause any regressions. It seems straightforward, though I could be mistaken.

I've opened PR #506 as an improvement now that we require Python >= 3.7.

For shipping libvips binary, another idea is to create a separate PyPI package for libvips, and mark that as pyvips's optional dependency.

FWIW, I opened PR #507 for this. Previously this was called pyvips-prebuilt, but I think -binary is more common in Python-land (see e.g. the psycopg-binary package), at least it's shorter to write.