Feature: Don't copy dependencies' DLLs by default

NfNitLoop commented 1 month ago

Existing Functionality

If I'm understanding things correctly, Maturin has this current feature:

If a build results in code that dynamically links to a library (that's not libc?), Maturin:

Copies those DLLs into the resulting python package.
(on Linux) uses patchelf to fix DLL loading (to use those copied DLLs, I assume).

Example maturin build output:

🖨  Copied external shared libraries to package [your-package].libs directory:
    /usr/lib/aarch64-linux-gnu/libcrypto.so.1.1
    /usr/lib/aarch64-linux-gnu/libssl.so.1.1

The Problem

This is really surprising behavior!

My builds had been working fine, until I added some dependency which (somewhere down the transitive dependency chain) introduces dynamic linkage. Then I got surprise errors about patchelf, which wasn't needed before.

Thankfully I took some time to investigate the sudden need for patchelf because I don't want third-party dynamic libraries copied into my python package. It's only an accident of the dependencies I've added.

IANAL but can't there also be legal implications to distributing a DLL vs linking to it?

Requested Improvement

Maybe maturin shouldn't copy DLLs by default. Or, at a minimum, I'd love an option to explicitly disable copying them.

If some people want to allow some DLLs but avoid accidentally including others, maybe an "allow list" for DLLs to be copied would work better for them.

Workaround

For now, I'm working to remove the DLL from my dependency tree. I'm recommending that my team not add the patchelf dependency to our build dependencies/environments, so that it can act as a stopper to keep us from getting into this situation again.

But relying on some dependency being missing is not the most stable way to accomplish this. 😅

Context

This is related to an issue I asked for help with in Discord.

messense commented 1 month ago

Maybe maturin shouldn't copy DLLs by default

I disagree, you'd ship broken wheels to users.

Or, at a minimum, I'd love an option to explicitly disable copying them.

You can already do that by passing --compatibility linux or --skip-auditwheel.

NfNitLoop commented 1 month ago

I disagree, you'd ship broken wheels to users.

I'm not saying that it should succeed in that case. It should fail the build, with something like:

"X depends on shared library Y. To copy it into the python wheel, configure your build with [setting]."

messense commented 1 month ago

"X depends on shared library Y. To copy it into the python wheel, configure your build with [setting]."

That's #2039, so closing this in favor of that.

NfNitLoop commented 1 month ago

No, #2039 is just to add logging, it says nothing about failing the build.

messense commented 1 month ago

It will fail if you don't have patchelf installed.

NfNitLoop commented 1 month ago

Yeah, that's not a very reliable system. What if patchelf was installed for something else? "Just uninstall patchelf to avoid accidentally copying things into your build results" is such a hack. 😞

messense commented 1 month ago

As I have said, you have --compatibility linux or --skip-auditwheel for explicit control.

NfNitLoop commented 1 month ago

Are there docs that I should have read that say "use these options to keep from copying shared libraries into your package"? Because I missed them. And I never would have guessed that from either of those options names. 😅

messense commented 1 month ago

There are some docs in https://www.maturin.rs/distribution#build-wheels but definitely can be improved (esp. for people not familiar with auditwheel).

NfNitLoop commented 1 month ago

Aha! Thanks for the pointer. I'm not familiar with auditwheel.

maturin contains a reimplementation of auditwheel automatically checks the generated library and gives the wheel the proper platform tag.

If your system's glibc is too new, it will assign the linux tag.

If you link other shared libraries, maturin will try to bundle them within the wheel, note that this requires patchelf, it can be installed along with maturin from PyPI: pip install maturin[patchelf].

However, I tried both of your suggestions: --compatibility linux or --skip-auditwheel. They both seem to have the same effect:

No DLLs are copied into the package.
But the build isn't failed. (Unless patchelf is missing.)

So now I'm accidentally distributing a wheel that might not work on users' systems. ⚠️

What I really want is:

I can specify a compatibility target. (ex: before running into this issue, our GitHub workflows were able to target manylinux_2_17.)
The build does not copy DLLs into my package to make it compatible.
Instead, it fails if it is not compatible. (ex: Currently, I get this behavior if I target an older manylinux compatibility, but I'm building on a system that's too new to guarantee compatibility.)
This doesn't rely on whether or not some dependency is present on my system.

messense commented 1 month ago

That sounds reasnable, I think we can extend --skip-auditwheel to add some variants:

--skip-auditwheel: no functional change, deprecate it in favor of --auditwheel=skip
--auditwheel=repair: same as --compability=XXX w/o --skip-auditwheel
--auditwheel=skip: new --skip-auditwheel equivlent option
--auditwheel=check: fail the build if it requires copy DLLs or new GLIBC versions

messense commented 1 month ago

@NfNitLoop What do you think of https://github.com/PyO3/maturin/issues/2038#issuecomment-2050791915

under this proposal, you will use maturin build --compability manylinux_2_17 --auditwheel=check.

NfNitLoop commented 1 month ago

Love it! That would be great!

BTW, I assume you might make --auditwheel=repair the default, for backward compatibility, and that's reasonable. But I would recommend (maybe in the next semver-major release) making "check" the default. I talked w/ a few other rust developers about this issue and they all seemed surprised to hear that copying shared libraries was the default. I think "the least surprising behavior" is a good default.

NfNitLoop commented 1 month ago

Can we re-open this issue, then?

messense commented 1 month ago

I talked w/ a few other rust developers about this issue and they all seemed surprised to hear that copying shared libraries was the default. I think "the least surprising behavior" is a good default.

Though the current state is the least surprising behavior for Python users.

messense commented 1 month ago

Closing in favor of #2045

PyO3 / maturin