matthew-brett / delocate

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

Raise an exception on dependency resolution failures. #91

Closed HexDecimal closed 3 years ago

HexDecimal commented 3 years ago

This only affects libraries with @rpath dependencies. It's also me fixing my own bad code when I wrote resolve_rpath. At least I knew this could be a problem back then and added a warning for it.

Previously this issue was warned about using Python's warning module, but these errors are more often critical and will affect the compatibility of the wheel. A failure here means that either the library actually is missing, delocate is not being correct, or the library was built incorrectly.

The more I considered it the less I was able to justify the ability to ignore these errors as an option since it's too implicit and could easily ignore unintended dependencies. I did add a parameter to tree_libs which can be enabled to ignore these errors, but there's no new command line options yet.

This is what the new error looks like in practice, compared to the example in #90:

delocate-wheel -v dist/*.whl
Fixing: dist/tcod-11.19.2.dev2-cp35-abi3-macosx_10_9_x86_64.whl
Traceback (most recent call last):
  File "/Users/runner/work/python-tcod/python-tcod/venv/bin/delocate-wheel", line 8, in <module>
    sys.exit(main())
  File "/Users/runner/work/python-tcod/python-tcod/venv/lib/python3.7/site-packages/delocate/cmd/delocate_wheel.py", line 72, in main
    check_verbose=opts.verbose)
  File "/Users/runner/work/python-tcod/python-tcod/venv/lib/python3.7/site-packages/delocate/delocating.py", line 378, in delocate_wheel
    lib_filt_func, copy_filt_func)
  File "/Users/runner/work/python-tcod/python-tcod/venv/lib/python3.7/site-packages/delocate/delocating.py", line 290, in delocate_path
    return copy_recurse(lib_path, copy_filt_func, copied)
  File "/Users/runner/work/python-tcod/python-tcod/venv/lib/python3.7/site-packages/delocate/delocating.py", line 146, in copy_recurse
    _copy_required(lib_path, copy_filt_func, copied_libs)
  File "/Users/runner/work/python-tcod/python-tcod/venv/lib/python3.7/site-packages/delocate/delocating.py", line 202, in _copy_required
    lib_dict = tree_libs(lib_path)
  File "/Users/runner/work/python-tcod/python-tcod/venv/lib/python3.7/site-packages/delocate/libsana.py", line 103, in tree_libs
    raise DependencyNotFound(error_msg)
delocate.libsana.DependencyNotFound: Could not find these dependencies:
@rpath/hidapi.framework/Versions/A/hidapi not found:
  Needed by: /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/tmpabvk0pzn/wheel/tcod/.dylibs/SDL2
  Search path:
    @executable_path/Frameworks
    @loader_path/Frameworks

This error reveals what happened. SDL2 now has a Frameworks directory embedded in its own framework and delocate doesn't support @loader_path yet. This will need to be fixed so I can update SDL to the latest version, but I'm not a hurry. Anyone else might be able to use DYLD_LIBRARY_PATH as a workaround.

As mentioned before this could possibly be a breaking change. I've given my reasons for why this is acceptable but tree_libs can be told to ignore errors if you can pass the option down to it.

Resolves #90

HexDecimal commented 3 years ago

I'm thinking of working on the @loader_path issue in another PR. Would it be okay if I setup GitHub Actions for Flake8 and MyPy? I'll need MyPy so that I can more easily work on larger refactors.

matthew-brett commented 3 years ago

Yes, please go ahead ... thanks.

matthew-brett commented 3 years ago

But - isn't that what try : catch is for?

ferdnyc commented 3 years ago

@matthew-brett

That's a fair question. ...Maybe?

Problem with raising is, it breaks you out of the current operation. That's fine if the current operation is "report the status of this individual dependency", but certainly nobody'd want delocate-listdeps to give up at the first sign of an unresolved dep.

And the other side of that coin is, if a dependency resolution failure is going to raise an exception, then it needs to be raised immediately, each time there's a failure, but without preventing the rest of the lookups from completing.

HexDecimal commented 3 years ago

I had issues with raising errors in practice and had to stop. PR #94 continues this branch, but with the topic of @loader_path support instead.