pyvips icon indicating copy to clipboard operation
pyvips copied to clipboard

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

Open kleisauke opened this issue 1 year ago • 12 comments

See the individual commits for more details.

kleisauke avatar Jan 05 '24 14:01 kleisauke

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

jcupitt avatar Jan 05 '24 16:01 jcupitt

Quick thoughts:

  • it seems to work well!
  • since this drops py2 support, we should probably do a major version bump to pyvips 3.0.0
  • hahah no more py2 nice
  • should we add automatic libvips binary downloading too, perhaps if no libvips install is found? or do you think that should be a separate PR?

jcupitt avatar Jan 05 '24 17:01 jcupitt

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 avatar Jan 07 '24 11:01 kleisauke

... 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:.

kleisauke avatar Jan 07 '24 11:01 kleisauke

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.

SteveHawk avatar Jan 09 '24 08:01 SteveHawk

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.

jcupitt avatar Jan 09 '24 08:01 jcupitt

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 avatar Jan 09 '24 17:01 kleisauke

... 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

kleisauke avatar Jan 09 '24 18:01 kleisauke

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.:

  • we're passing uint64_t
  • that's a typedef for unsigned long long
  • to a function that takes a GType
  • which is a typedef for unsigned long
  • both unsigned long long and unsigned long are 64 bits on macos, but clang still wants to complain, sigh

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 :(

jcupitt avatar Mar 22 '24 14:03 jcupitt

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;
}

kleisauke avatar Mar 22 '24 14:03 kleisauke

Oh great! Yes, that should fix it too.

jcupitt avatar Mar 22 '24 14:03 jcupitt

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

kleisauke avatar Apr 01 '24 08:04 kleisauke

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 avatar Sep 21 '24 10:09 jcupitt

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!

jcupitt avatar Sep 21 '24 12:09 jcupitt

No problem! Thanks for merging.

kleisauke avatar Sep 21 '24 12:09 kleisauke

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.

kleisauke avatar Sep 21 '24 13:09 kleisauke