Avoid duplication when library has soft links and been used in the same wheels.

[sighingnow]

Resolves issue #133.

I have managed to reproduce the issue by the following minimal case:

We have:

  liba.dylib
  links/liba.dylib, a soft link to liba.dylib

and

  libb.dylib, depends on liba.dylib via `@rpath/liba.dylib`
  links/libb.dylib, depends on links/liba.dylib and been found via `DYLD_LIBRARY_PATH`

During resolving dependencies, the first is found via resolving from @rpath by resolve_dynamic_paths, and the later is resolved from DYLD_LIBRARY_PATH by search_environment_for_lib, where the soft link path is returned, thus the conflicit happens.

For more details, see the minimal reproduce case in test_tree_libs_from_directory_with_links, and we have verified the patch in our real-world wheels that triggerred #133 before.

[sighingnow]

This pull request generates copied files in the test case rather than put it into tests/data to avoid affecting other test cases.

[codecov-commenter]

Codecov Report

Merging #134 (43bd372) into master (9c2c6a4) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #134   +/-   ##
=======================================
  Coverage   95.28%   95.28%           
=======================================
  Files          13       13           
  Lines        1018     1018           
=======================================
  Hits          970      970           
  Misses         48       48           

Impacted Files	Coverage Δ
delocate/libsana.py	97.64% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9c2c6a4...43bd372. Read the comment docs.

[sighingnow]

The fix itself looks good.

You should add a note for this fix in the changelog.

I'd accept the test if it was at least using a try..finally block when working with os.environ.

I have applied the suggested change.

[sighingnow]

Hi @HexDecimal could we get this one merge? and it it is possible, could we have a new release on pypi to include this fix?

Thanks!

[HexDecimal]

@matthew-brett, would you look at the test in case I've missed anything obvious? Or should I merge PR's without your input?

I can make a bug fix release immediately once this is merged.

[matthew-brett]

Thanks both. Yes, will have a look at this tomorrow morning, but @HexDecimal - feel free to merge without me if I'm being slow, or a merge needs doing.

[sighingnow]

Thanks for this - just a few suggestions.

Thanks for reviewing, all comments have been addressed.

[sighingnow]

Lint error fixed.

[matthew-brett]

Thanks very much @sighingnow for the PR, and @HexDecimal for the review.

[HexDecimal]

You're welcome. I've also released a version with this fix.

[sighingnow]

You're welcome. I've also released a version with this fix.

Thanks a lot.