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

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

Merged
merged 5 commits into from
Sep 21, 2024

Conversation

kleisauke
Copy link
Member

@kleisauke kleisauke commented Jan 5, 2024

See the individual commits for more details.

@jcupitt
Copy link
Member

jcupitt commented Jan 5, 2024

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

@jcupitt
Copy link
Member

jcupitt commented Jan 5, 2024

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?

@kleisauke
Copy link
Member Author

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:
kleisauke@615969f
(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
Copy link
Member Author

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

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
Copy link
Member

jcupitt commented Jan 9, 2024

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
Copy link
Member Author

kleisauke commented Jan 9, 2024

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 kleisauke@0c09215 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
Copy link
Member Author

... 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
Copy link
Member

jcupitt commented Mar 22, 2024

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

@kleisauke
Copy link
Member Author

Has that been tested with GLib 2.80.0 from Homebrew? If so, I think PR Homebrew/homebrew-core#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
Copy link
Member

jcupitt commented Mar 22, 2024

Oh great! Yes, that should fix it too.

@kleisauke
Copy link
Member Author

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

@kleisauke kleisauke marked this pull request as ready for review April 19, 2024 06:26
@jcupitt
Copy link
Member

jcupitt commented Sep 21, 2024

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 jcupitt merged commit 4e7582b into libvips:master Sep 21, 2024
6 checks passed
@jcupitt
Copy link
Member

jcupitt commented Sep 21, 2024

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
Copy link
Member Author

No problem! Thanks for merging.

@kleisauke kleisauke deleted the pyproject-toml branch September 21, 2024 12:33
@kleisauke
Copy link
Member Author

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.

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 this pull request may close these issues.

3 participants