Raise an exception on dependency resolution failures. #91

HexDecimal · 2020-12-31T12:33:28Z

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

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.

matthew-brett

Thanks for this - I agree, it seems better to raise here, despite back-compatibility.

delocate/libsana.py

matthew-brett · 2020-12-31T12:49:56Z

delocate/libsana.py

@@ -24,6 +31,9 @@ def tree_libs(start_path, filt_func=None):
        If None, inspect all files for library dependencies. If callable,
        accepts filename as argument, returns True if we should inspect the
        file, False otherwise.
+    ignore_missing : bool


Do we want to ignore or warn?

Currently tree_libs prints out ignored dependencies. I don't want it to be silent since it seems very important when they get ignored. Maybe 'skip_missing' would be a better term?

matthew-brett · 2020-12-31T12:52:02Z

delocate/libsana.py

@@ -24,6 +31,9 @@ def tree_libs(start_path, filt_func=None):
        If None, inspect all files for library dependencies. If callable,
        accepts filename as argument, returns True if we should inspect the
        file, False otherwise.
+    ignore_missing : bool
+        Determines if dependency resolution failures are considered a critical
+        error to be raised.  Otherwise ignored libraries are only printed out.


Could you also add a Raises section at the end for the DependencyNotFound error? https://numpydoc.readthedocs.io/en/latest/format.html

I almost added a raises section but I didn't find anything else in the module to reference when I looked and didn't notice this was using the NumPy docstring style guide. I'll add one now.

A contributing file might have helped.

matthew-brett · 2020-12-31T12:52:42Z

delocate/tests/test_libsana.py

@@ -182,5 +183,5 @@ def test_resolve_rpath():
    lib_rpath = pjoin('@rpath', lib)
    # Should skip '/nonexist' path
    assert_equal(resolve_rpath(lib_rpath, ['/nonexist', path]), realpath(LIBA))
-    # Should return the given parameter as is since it can't be found
-    assert_equal(resolve_rpath(lib_rpath, []), lib_rpath)
+    # Should raise DependencyNotFound if the dependency can not be resolved.


And test for warning, if that seems useful?

The skip missing behavior is in the tree_libs function. I changed the feedback from using the warnings module to just print, since it'd be expected behavior by that point.

I have it back to using warnings now.

@HexDecimal
I checked out this branch to compare the PR code with the released 0.8.2, after hitting this output when running delocate-listdeps. I have to say, it doesn't feel particularly right to me, in either the previous or current form. (Though it's definitely much improved, now that it's no longer outputting module source code as the warning message!)

Even with the fixed code, though, it doesn't feel like the right sort of output when running an interactive utility. (Meaning, in the context of delocate-listdeps specifically.) In that context, in fact, it feels a bit like a violation of the "When to use logging" chart from the Basic logging tutorial, which advises:

Task you want to perform The best tool for the task

Display console output for ordinary usage of a command line script or program print()

Issue a warning regarding a particular runtime event warnings.warn() in library code if the issue is avoidable and the client application should be modified to eliminate the warning

...The primary implication here being that the warning signals an application error, that "the client application should be modified to eliminate the warning". There's no change that can be made to delocate-listdeps to correct unresolvable dependency paths in a native binary. Shouldn't that just be reported in the normal print() output, like the rest of the dependency listing?

I've been having the same feeling and I wish the logging module itself was being used. There's a lot of stuff I wished I could print for debugging without it being in the main output.

Would it be valid to import the logging module and start using it for this kind of thing? Or should I just stick with print?

It's a bit tricky, because of the usual tension between libraries and command line utilities. Printing from libraries is usually very bad style, and delocate can be used as a library. So, what I'd like to find is some way of making the output behave as expected for a command line utilty - with ordinary, expected output going to stdout, and warnings / errors going to stderr - while also making it behave sensibly when used as a library.

I think, precisely because of that multi-purpose utility, no decision about how to respond to a missing dependency can really be made at that low a level -- not to the point of informing the user, at least. Not by any method.

Because, if the caller id delocate-listdeps, then an unresolvable path is merely a point of information, no more or less bad than a resolved path, and the tool should handle informing the user of both through normal means. There's no error of any kind, in that situation.

But if the caller is delocate-wheel trying to fix up package's external dependencies, not being able to resolve one is a four-alarm failure condition. So then it needs to be shouted about, in a way that user can't possibly miss.

That's a decision that can only be made higher up the call chain, with benefit of the proper context to make the right call. Any action taken way down in the inner workings of the library other than kicking the problem upstairs will invariably be the wrong one, for some subset of all the possible scenarios.

Added missing raises sections and fixed documented type hints.

HexDecimal · 2021-01-01T07:09:39Z

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 · 2021-01-01T12:36:49Z

Yes, please go ahead ... thanks.

Ignore the lack of type-hinting from the wheel package.

Everything in the delocate directory with the exception of `_version.py` has been type annotated. Code has been converted to use Unicode more often, this is enforced better in Python 2 via type hinting. Six is used in some places to ensure compatibility with Python 2. `back_tick` was too polymorphic to gracefully type hint so it's been split into two calls. `check_call` which is like `back_tick`'s default behavior and `run_call` which returns the error output instead of raising an exception. In addition both functions take and return Unicode strings exclusively. Tests and code have been updated with the new functions. This is the best I can do without dropping Python 2.7 or adding `subprocess32`. InWheelCtx violated its supertype by overriding __enter__ with a different return type. I've changed this class into a function with similar behavior. InWheelCtx tests have not been changed. MyPy doesn't like the tests but the current code will work well in practice. This new function can be turned into a method of InWheel if you really wanted support for class inheritance. InWheelCtx can also be a class that encapsulates InWheel, it just can't sub-class it.

The function was brought back as it was before, but has now been deprecated. I had to guess the documented deprecated version number. Normally I'd suppress the warnings for pytest, but I'm not sure what the pytest_tools module is doing exactly.

This still isn't completely backwards compatible. A sub-class of InWheelCtx no longer returns itself in a context.

The `ret_self` parameter has been removed from InWheel. This already did nothing before I started working on these classes and it never had any associated tests. This breaks MyPy's implicit union in the tests for some reason. InWheelCtx returns itself again and implements its previous behavior using properties. This is the most backward compatible option.

This only works if the loader_path parameter is good. The one given by the tree_libs function is technically correct but will fail because it's often working on already moved libraries. resolve_rpath's loader_path parameter is required, which is a breaking change.

matthew-brett · 2021-01-11T12:37:14Z

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

ferdnyc · 2021-01-12T03:46:35Z

@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 · 2021-01-20T11:00:41Z

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.

matthew-brett reviewed Dec 31, 2020

View reviewed changes

HexDecimal added 2 commits December 31, 2020 05:36

Update documentation, rename ignore_missing to skip_missing.

c85c63c

Added missing raises sections and fixed documented type hints.

Change skipped dependencies to use the warnings module.

4f83cd2

HexDecimal added 4 commits January 2, 2021 17:34

Add a GitHub Action to enforce Flake8.

e7b5910

Resolve all remaining Flake8 issues,

1c75d55

Add MyPy checks to GitHub Actions.

048973e

Ignore the lack of type-hinting from the wheel package.

HexDecimal marked this pull request as draft January 4, 2021 07:35

HexDecimal added 5 commits January 4, 2021 08:33

Revert removing the back_tick function.

8c286d0

The function was brought back as it was before, but has now been deprecated. I had to guess the documented deprecated version number. Normally I'd suppress the warnings for pytest, but I'm not sure what the pytest_tools module is doing exactly.

InWheelCtx is now a class which encapsulates InWheel.

1537dab

This still isn't completely backwards compatible. A sub-class of InWheelCtx no longer returns itself in a context.

Merge in type hinting branch.

021fdfd

HexDecimal mentioned this pull request Jan 11, 2021

Support for @loader_path. #94

Merged

HexDecimal closed this Jan 20, 2021

HexDecimal deleted the error-on-dep-resolution-failure branch July 15, 2021 12:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raise an exception on dependency resolution failures. #91

Raise an exception on dependency resolution failures. #91

HexDecimal commented Dec 31, 2020

matthew-brett left a comment

matthew-brett Dec 31, 2020

HexDecimal Dec 31, 2020

matthew-brett Dec 31, 2020

HexDecimal Dec 31, 2020

matthew-brett Dec 31, 2020

HexDecimal Dec 31, 2020

HexDecimal Jan 1, 2021

ferdnyc Jan 10, 2021

HexDecimal Jan 10, 2021

matthew-brett Jan 10, 2021

ferdnyc Jan 11, 2021

HexDecimal commented Jan 1, 2021

matthew-brett commented Jan 1, 2021 via email

matthew-brett commented Jan 11, 2021 via email

ferdnyc commented Jan 12, 2021

HexDecimal commented Jan 20, 2021

Task you want to perform	The best tool for the task
Display console output for ordinary usage of a command line script or program	`print()`
Issue a warning regarding a particular runtime event	`warnings.warn()` in library code if the issue is avoidable and the client application should be modified to eliminate the warning

Raise an exception on dependency resolution failures. #91

Raise an exception on dependency resolution failures. #91

Conversation

HexDecimal commented Dec 31, 2020

matthew-brett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HexDecimal commented Jan 1, 2021

matthew-brett commented Jan 1, 2021 via email

matthew-brett commented Jan 11, 2021 via email

ferdnyc commented Jan 12, 2021

HexDecimal commented Jan 20, 2021