dcwatson / deflate

Python extension wrapper for libdeflate.
MIT License
25 stars 6 forks source link

Use system installed deflate library #29

Closed bket closed 2 years ago

bket commented 2 years ago

Use pkgconfig if available to locate a libdeflate system library.

If a specific library prefix is given by LIBDEFLATE_PREFIX=somepath, use that. Otherwise if pkgconfig is installed and the library can be located via that, use that one. Otherwise (or if enforced by USE_BUNDLED_DEFLATE=yes), fall back to bundled code.

Build tested, with and without deflate library installed, on OpenBSD amd64 current. Tests pass.

ThomasWaldmann commented 2 years ago

also minor pep8 issue:

setup.py:55:21: missing whitespace around operator
ThomasWaldmann commented 2 years ago

BTW, I edited the top post a bit to reflect the current behaviour.

Could also just be a c&p of the commit comment.

ThomasWaldmann commented 2 years ago

BTW, our tests of course only test the default behaviour and do not set any of these env vars yet - but I suspect you tried all modes manually?

bket commented 2 years ago

Fixed :-)

bket commented 2 years ago

BTW, our tests of course only test the default behaviour and do not set any of these env vars yet - but I suspect you tried all modes manually?

Yes, tried all modes manually. So far, behavioud is as expected.

ThomasWaldmann commented 2 years ago

@dcwatson I think this is a good and very flexible improvement to deflate and it especially enables dist package maintainers to not use the bundled code (or even remove it) if they rather prefer to use the libdeflate library from another dist package.

Avoids duplication and eases maintenance, especially in case of critical bug or security fixes, they then only need to get fixed at one place.

Merge?

ThomasWaldmann commented 2 years ago

@bket If you don't mind, I'll fix it now according to the review comments of @dcwatson ...

bket commented 2 years ago

@bket If you don't mind, I'll fix it now according to the review comments of @dcwatson ...

@ThomasWaldmann, thank you for taking care of this! Changes lgtm, and make sense.

ThomasWaldmann commented 2 years ago

@bket if this gets merged and released, it will unblock some borg PRs, that's why I am trying to speed this up a bit. :-)

ThomasWaldmann commented 2 years ago

trying pytest within cibuildwheel now vvv.

ThomasWaldmann commented 2 years ago

yay! all except pypy tested OK.

Guess pypy breakage is not new and unrelated to this PR?

  ImportError while importing test module '/project/tests/test_compress.py'.
  Hint: make sure your test modules/packages have valid Python names.
  Traceback:
  /opt/python/pp37-pypy37_pp73/lib-python/3/importlib/__init__.py:127: in import_module
      return _bootstrap._gcd_import(name[level:], package, level)
  /project/tests/test_compress.py:6: in <module>
      import deflate
  E   ImportError: /tmp/tmp.Dh9bFArfRD/venv/site-packages/deflate.pypy37-pp73-x86_64-linux-gnu.so: undefined symbol: Py_Initialize
ThomasWaldmann commented 2 years ago

@dcwatson have a look! before merging i need to remove that temporary commit.

https://github.com/dcwatson/deflate/runs/5489155309?check_suite_focus=true

ThomasWaldmann commented 2 years ago

@dcwatson considering this works and wheels are even tested now, i'ld like to merge this if you don't object.

this will remove the pypy wheel build (which likely was broken before also, just nobody noticed).

i'll remove that temporary commit now, which was just for demonstration purposes, but we still have the test results linked above.

ThomasWaldmann commented 2 years ago

temporary commit removed, rebased on current master.

ThomasWaldmann commented 2 years ago

As there were no more objections, I merged this now.

In case we find more to do / to fix, it can be done in a later PR.

Next thing we need is a new release. @dcwatson can you do that or give me permissions on pypi.org to do that? #26