auditwheel icon indicating copy to clipboard operation
auditwheel copied to clipboard

Add runtime dependency on patchelf-pypi

Open lkollar opened this issue 2 years ago • 5 comments

The patchelf-pypi project ships up to date patchelf binaries. Adding this to install_requires would mean that users don't have to track down and install the package with a different package manager.

Any thoughts @mayeut? Any reason not to make this a dependency?

lkollar avatar Jul 22 '22 10:07 lkollar

Any reason not to make this a dependency?

There are a few reasons:

  • If I'm not mistaken, pipx installed auditwheel wouldn't work out-of-the-box without care (need to expose the entry point of a dependency which is not the default behavior). This could be solved by reworking the Patchelf class.
  • patchelf is only required for the repair command => this means a auditwheel show or a future auditwheel check could be run on any platform (not only linux for a check command) without patchelf which will probably never have macOS or Windows wheels.
  • OS packagers will want to remove this dependency and use their own version of patchelf (like they are doing today e.g. in Fedora)
  • Someone using a custom/system version of patchelf will need to go the extra mile to use it

Maybe a good first middle ground would be to add the dependency as an extra and update the documentation accordingly ? e.g. pip install auditwheel[patchelf] or even just pip install auditwheel patchelf with no extra.

mayeut avatar Jul 30 '22 08:07 mayeut

  • patchelf is only required for the repair command => this means a auditwheel show or a future auditwheel check could be run on any platform (not only linux for a check command) without patchelf which will probably never have macOS or Windows wheels.

This could be solved by making patchelf a platform-specific dependency when/if we add support for other platforms.

  • OS packagers will want to remove this dependency and use their own version of patchelf (like they are doing today e.g. in Fedora)

Good point. I agree that this is problematic.

Maybe a good first middle ground would be to add the dependency as an extra and update the documentation accordingly ? e.g. pip install auditwheel[patchelf] or even just pip install auditwheel patchelf with no extra.

Good idea. And this is pretty simple to implement. We could also note this in the error message if auditwheel cannot find patchelf.

lkollar avatar Aug 16 '22 18:08 lkollar

patchelf-pypi do not provide wheels for Alpine at all and for debian\alpine with armv7 CPU. will adding those dependency will try to install patchelf-pypi from source even if patchelf was installed with package manager or builded from source?

bigcat88 avatar Nov 24 '22 09:11 bigcat88

patchelf-pypi do not provide wheels for Alpine

yes it does

patchelf-pypi do not provide for debian\alpine with armv7 CPU

you're right, it doesn't

will adding those dependency will try to install patchelf-pypi from source even if patchelf was installed with package manager or builded from source?

Yes it would and that's one reason I'm not in favor of adding this dependency and proposed something else.

mayeut avatar Nov 27 '22 13:11 mayeut

FWIW I implemented a pure-python patchelf replacement that I use in my repairwheel tool (which wraps auditwheel and the other two related delocate and delvewheel). One caveat is that strip doesn't like the result, as I took a different, less invasive approach than patchelf. But one could always strip prior to patching.

jvolkman avatar Jun 26 '23 18:06 jvolkman