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

Feature: Don't copy dependencies' DLLs by default #2038

Closed
NfNitLoop opened this issue Apr 11, 2024 · 16 comments
Closed

Feature: Don't copy dependencies' DLLs by default #2038

NfNitLoop opened this issue Apr 11, 2024 · 16 comments
Labels
enhancement New feature or request

Comments

@NfNitLoop
Copy link

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.

@NfNitLoop NfNitLoop added the enhancement New feature or request label Apr 11, 2024
@messense
Copy link
Member

messense commented Apr 11, 2024

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

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

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

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

@messense
Copy link
Member

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

@NfNitLoop
Copy link
Author

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

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

@NfNitLoop
Copy link
Author

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

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

NfNitLoop commented Apr 12, 2024

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.

  1. If your system's glibc is too new, it will assign the linux tag.
  2. 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
Copy link
Member

messense commented Apr 12, 2024

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

@NfNitLoop What do you think of #2038 (comment)

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

@NfNitLoop
Copy link
Author

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

Can we re-open this issue, then?

@messense messense reopened this Apr 13, 2024
@messense
Copy link
Member

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

Closing in favor of #2045

@messense messense closed this as not planned Won't fix, can't repro, duplicate, stale Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants