search

matthew-brett / delocate

Find and copy needed dynamic libraries into python wheels
BSD 2-Clause "Simplified" License
262 stars 59 forks source link

allow copying files into an already existing directory #199

Open mattip opened 4 months ago

mattip commented 4 months ago

Describe the bug A clear and concise description of what the bug is.

I would like to ship a build of OpenBLAS as a wheel. On posix, the build ships a libscipy-openblas.so which depends on libgfortran.so and libquadmath.so. The scipy-openblas shared object is the target bundled in the wheel under lib, then auditwheel/delocate is used to copy libgfortran and libquadmath into the lib directory. On auditwheel I can do this using --lib-sdir /lib. But in delocate, using delocate-wheel -L /lib raises an error here (note there is also a typo in the use of f-string, the f should be outside the quotes): https://github.com/matthew-brett/delocate/blob/eaba43dc531d0af348cb9b949a6c3f0790d207b2/delocate/delocating.py#L684-L687

To Reproduce Steps used to reproduce the behavior, such as the delocate commands used.

Expected behavior A clear and concise description of what you expected to happen.

Wheels used If a wheel is involved then consider attaching the original wheel (before being delocated) or linking to the repository where the original wheel can be created.

Platform (please complete the following information):

Additional context Add any other context about the problem here.

What was the reasoning for wanting a clean directory?

mattip commented 4 months ago

here is the code in question

HexDecimal commented 4 months ago

If I recall correctly it was a safety check since I wasn't sure how to handle a situation where a library references an external dependency but that dependency already exists inside the package. Without this check the internal file would've been silently clobbered.

What does auditwheel do in this situation? Does it reuse or clobber the internal file?

HexDecimal commented 4 months ago

At the very least I could probably change this into a warning instead of an error.

mattip commented 4 months ago

Does it reuse or clobber the internal file?

I'm not sure. That would be a strange situation, but then maybe the check should be more fine-grained. If I understand correctly the code currently will error out if the directory pre-exists. I could make a PR to check my assumptions, if you think it makes sense.

HexDecimal commented 4 months ago

You're right. It seems it'd delete the folders contents and start over if this check didn't stop it. I'm wondering if that's actually desirable or what the edge cases are.

Looking into it more, I did not write this other than to mess up converting it to an f-string. This check was here for decades. Original code for comparision.

I suspect it's surviving multiple refactors because everyone including me has been too afraid to touch it.