matthew-brett / delocate

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

DelocationError: Already planning to copy library with same basename as: ... #133

Closed sighingnow closed 2 years ago

sighingnow commented 2 years ago

Describe the bug

Hi @matthew-brett,

It seems that delocate cannot process brew-installed libraries correct, e.g., if we have two binary target in a wheel package, one of it links to /usr/local/lib/libabsl_status.2103.0.1.dylib and another links to /usr/local/Cellar/abseil/20210324.2_1/lib/libabsl_status.2103.0.1.dylib, delocate will raise exception:

2021-11-16T05:00:04.8377350Z Fixing: dist/gs_lib-0.8.0-py2.py3-none-macosx_10_9_x86_64.whl
2021-11-16T05:00:04.8378500Z   File "/usr/local/bin/delocate-wheel", line 8, in <module>
2021-11-16T05:00:04.8379030Z     sys.exit(main())
2021-11-16T05:00:04.8380080Z   File "/usr/local/lib/python3.9/site-packages/delocate/cmd/delocate_wheel.py", line 91, in main
2021-11-16T05:00:04.8380810Z     copied = delocate_wheel(
2021-11-16T05:00:04.8381920Z   File "/usr/local/lib/python3.9/site-packages/delocate/delocating.py", line 598, in delocate_wheel
2021-11-16T05:00:04.8382640Z     copied_libs = delocate_path(
2021-11-16T05:00:04.8383750Z   File "/usr/local/lib/python3.9/site-packages/delocate/delocating.py", line 429, in delocate_path
2021-11-16T05:00:04.8384830Z     return delocate_tree_libs(lib_dict, lib_path, tree_path)
2021-11-16T05:00:04.8386340Z   File "/usr/local/lib/python3.9/site-packages/delocate/delocating.py", line 80, in delocate_tree_libs
2021-11-16T05:00:04.8387230Z     libraries_to_copy, libraries_to_delocate = _analyze_tree_libs(
2021-11-16T05:00:04.8388510Z   File "/usr/local/lib/python3.9/site-packages/delocate/delocating.py", line 121, in _analyze_tree_libs
2021-11-16T05:00:04.8389720Z     raise DelocationError('Already planning to copy library with '
2021-11-16T05:00:04.8390860Z delocate.delocating.DelocationError: Already planning to copy library with same basename as: libabsl_status.2103.0.1.dylib
2021-11-16T05:00:04.8516800Z make: *** [graphscope-darwin-py3] Error 1

However they are exactly the same library, as the former is just a soft link of the alter. The same issue happens to other libraries that are installed using brew too. I'm wondering is there any reason that we don't use the realpath of libraries in _tree_libs_from_libraries?

It if is ok to fix that I would like to submit a pull request.

To Reproduce

See the full log in https://pipelines.actions.githubusercontent.com/gy2OJWVp8kQU2Am4QAuhjA1IN6fEAQVuP8lbcuQFN2B1NgOLpQ/_apis/pipelines/1/runs/11189/signedlogcontent/2?urlExpires=2021-11-16T05%3A28%3A27.4207285Z&urlSigningMethod=HMACV1&urlSignature=m9fbjkcv2VGzyKSyCYst2qN8r7dxgNA90%2BGF88vCy6M%3D

Expected behavior

The softlink should be resolved before copying libraries.

Platform (please complete the following information):

Additional context Add any other context about the problem here.

HexDecimal commented 2 years ago

You can submit a PR and I'll review it. You'll need to write a test for this.

The install names are kept as-is since the exact names are used to modify the libraries which depend on their required libraries by using those install names. If you call realpath on them then that information is lost and they can no longer be edited to point to the copied library.

It might be better to edit _analyze_tree_libs and skip the exception when duplicate required paths point to the same library.

HexDecimal commented 2 years ago

Looking at it again, the required part could be the realpath since that isn't the part containing the install name. So it's likely that a call to realpath is missing somewhere in the code near where you originally suggested.

sighingnow commented 2 years ago

Looking at it again, the required part could be the realpath since that isn't the part containing the install name. So it's likely that a call to realpath is missing somewhere in the code near where you originally suggested.

Thanks for the quick analysis. I will submit the PR later.

sighingnow commented 2 years ago

Hi @HexDecimal, I have submitted a PR in #134, with a minimal test case to reproduce the bug.

sighingnow commented 2 years ago

Fixed by #134.