Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show recursive dependencies in listdeps command. #118

Merged
merged 7 commits into from
Sep 18, 2021

Conversation

HexDecimal
Copy link
Collaborator

@HexDecimal HexDecimal commented Jul 22, 2021

I finally got around to updating listdeps.

Added the tree_libs_from_directory function instead of duplicating this code. This is an improved version of the tree_libs function.

The wheel_libs function was modified to use tree_libs_from_directory.
This is not fully backwards compatible since the new behavior will now raise an error instead of returning an incomplete lib dict.

The listdeps test now moves the working directory to the test data dir to get more consistent results.

Closes #7

@HexDecimal HexDecimal marked this pull request as draft July 22, 2021 15:17
@HexDecimal HexDecimal force-pushed the listdeps-recursive branch 5 times, most recently from b6c6329 to da602a0 Compare July 22, 2021 18:29
@HexDecimal HexDecimal marked this pull request as ready for review July 22, 2021 18:51
@HexDecimal
Copy link
Collaborator Author

I've changed the way the platform wheel is tested. No more wheel_build_path.txt, now a custom fixture sets up a platform wheel depending on the external library from the test data directory.

The updated API understands that the external library can depend on libraries itself. So tests needed to be edited to include that extra information.

@matthew-brett
Copy link
Owner

Just checking quickly - this doesn't mean that we only test the code built on the platform on which we are testing?

@HexDecimal
Copy link
Collaborator Author

I haven't changed which platforms run tests. This still includes the tests that run without rebuilding the test data. I could now try running these tests in full isolation if you like.

fakepkg1 now starts with the install name libextfunc.dylib instead of the full path. The plat_wheel fixture changes this install name back into the full path of where libextfunc.dylib is located in the test data. This keeps the platform wheel tests like before, but now they're portable.

Copy link
Owner

@matthew-brett matthew-brett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - just some documentation suggestions.

delocate/libsana.py Outdated Show resolved Hide resolved
delocate/libsana.py Outdated Show resolved Hide resolved
delocate/libsana.py Show resolved Hide resolved
delocate/libsana.py Outdated Show resolved Hide resolved
delocate/libsana.py Outdated Show resolved Hide resolved

start_path : iterable of str
Root path of tree to search for libraries depending on other libraries.
lib_filt_func : callable, optional
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment as above about keyword only arguments.

executable_path : None or str, optional
If not None, an alternative path to use for resolving
`@executable_path`.
ignore_missing : bool, default=False
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ignore_missing : bool, default=False
ignore_missing : bool, default=False, optional

delocate/libsana.py Show resolved Hide resolved
delocate/libsana.py Outdated Show resolved Hide resolved
Copy link
Owner

@matthew-brett matthew-brett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few suggestions and comments. Looks like a nice refactor.

delocate/libsana.py Outdated Show resolved Hide resolved
delocate/libsana.py Outdated Show resolved Hide resolved
delocate/libsana.py Outdated Show resolved Hide resolved
delocate/libsana.py Show resolved Hide resolved
delocate/libsana.py Outdated Show resolved Hide resolved
delocate/libsana.py Show resolved Hide resolved
delocate/tests/test_delocating.py Outdated Show resolved Hide resolved
delocate/cmd/delocate_listdeps.py Show resolved Hide resolved
delocate/cmd/delocate_listdeps.py Show resolved Hide resolved
@HexDecimal HexDecimal force-pushed the listdeps-recursive branch 2 times, most recently from 829edae to 99c4673 Compare September 17, 2021 13:38
Copy link
Owner

@matthew-brett matthew-brett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - just a little more discussion.

delocate/cmd/delocate_listdeps.py Show resolved Hide resolved
delocate/cmd/delocate_listdeps.py Show resolved Hide resolved
@HexDecimal HexDecimal marked this pull request as ready for review September 17, 2021 14:12
HexDecimal and others added 7 commits September 17, 2021 18:39
Added tree_libs_from_directory function.  This is an improved version of
the tree_libs function.

The wheel_libs function was modified to use tree_libs_from_directory.
This is not fully backwards compatible since the new behavior
will now raise an error instead of returning an incomplete lib dict.
@codecov-commenter
Copy link

codecov-commenter commented Sep 18, 2021

Codecov Report

Merging #118 (c2b9a61) into master (26074b1) will increase coverage by 0.34%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #118      +/-   ##
==========================================
+ Coverage   94.42%   94.76%   +0.34%     
==========================================
  Files          13       13              
  Lines         986      994       +8     
==========================================
+ Hits          931      942      +11     
+ Misses         55       52       -3     
Impacted Files Coverage Δ
delocate/cmd/delocate_listdeps.py 92.68% <100.00%> (ø)
delocate/delocating.py 97.10% <100.00%> (+1.21%) ⬆️
delocate/libsana.py 97.59% <100.00%> (+0.33%) ⬆️

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 26074b1...c2b9a61. Read the comment docs.

@matthew-brett matthew-brett merged commit 34f1d78 into matthew-brett:master Sep 18, 2021
@matthew-brett
Copy link
Owner

Thanks - in it goes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recursive dependencies not shown in delocate-listdeps
3 participants