Support for @loader_path.
#94
Conversation
|
After several iterations of refactoring I ended up with the functions:
Travis tests have been difficult to work with. The way asserts are currently handled break pytests error reporting and should be updated to use assert statements directly. The I'm still missing |
|
Some functions from So far my plans to make a dependency tree traversal more strict haven't turned out well due to what I think are optional dependencies. Right now I just log these cases as errors and ignore them. Issues like this were ignored without output before now. I now have a working version with recursive This is my current output. The errors are from missing files on the runner. It might be too verbose to list these system files which aren't going to be copied anyway, but it's weird to me how these files are detected as missing but then the library can still load in an isolated environment afterwards. Only This might be finished, but I'm not certain. I'll at least be using this branch for my own wheels for the moment. |
|
So sorry to be slow to reply to this and your other PR. I'm a university lecturer, and things are really busy at the moment. This is a) very welcome, thank you so much and b) a lot to review. And you've got your other very welcome and large PR over at #93 . I wonder, would you have any interest in taking over the job of maintainer? I'm happy to continue to help, of course, but the code-base is soon going to be more fresh and up to date in your memory than mine. |
I made this PR since my own project suddenly had a requirement for
This PR doesn't change as many files as #93, so merging that one first would make this one easier to review.
I don't have a Mac to test things on. It's hard for me to check the tools or build test libraries and I rely on open-source CI for doing anything. I think I could update the library tests using Actions but I don't think GitHub has an old enough MacOS runner to compile the older x86 binaries. I planned on doing simple maintenance tasks through PRs and maybe adding simple features now that I'm knowledgeable with the library. I lack the experience to solve MacOS specific issues and this PR took more effort because I couldn't perform tasks directly on a Mac. I'd be willing to try and help as a co-maintainer but I'd be overwhelmed on my own. |
|
Right - that's totally fair - I have access to quite a few Macs, and use them all the time, so it's much more reasonable for me to responsible for the library. I'll do my best to get to these PRs quickly. Thanks again. |
|
It's been a very long time since I last checked this PR. I couldn't merge this since I added too many type hints, but I needed those type hints in order to design this PR in the first place. This PR was doomed since #93 couldn't be merged. I can try rebasing this and removing any unnecessary changes if you want. |
|
I've got time now to review this properly. Would you prefer to put this one together with the type hints? We can also drop Python 2, if that would help. |
|
Dropping Python 2 would only help all future work. It won't really affect what that's already been done. I think I can go with whatever plan you want. I feel like I can split my previous PR's up if necessary, or I can just resolve merge conflicts in this one. |
|
Just checking - this PR includes the changes in #93?
|
Yes. |
|
OK - then please do just rebase, and I'll work through it, probably tomorrow. |
Adds type hinting to the delocating and libsana modules.
|
I've already rebased this branch so I'm not sure if it still works until tests pass. The TravisCI jobs are taking a while. |
Test output has been cleaned up. Debug flags no longer escape the wheel tests.
Additional information is printed before the traceback, so this check had to be modified.
|
I've finished the rebase and the tests now pass again. This is ready for review. |
Co-authored-by: Matthew Brett <matthew.brett@gmail.com>
The list copy of lib_dict wasn't needed. Use an alternative type-hint for lib_filt_func. Co-authored-by: Matthew Brett <matthew.brett@gmail.com>
delocate/libsana.py
Outdated
| This must be the directory of the library which is loading `lib_path`. | ||
| executable_path : str, optional | ||
| The path to be used for `@executable_path`. | ||
| Assumed to be the working directory by default. |
There was a problem hiding this comment.
Is this a reasonable assumption? I didn't investigate much, but my guess would have been that it would be the path containing Python - not so? Maybe this should be an input option, from the command line. Should the default be the working directory? Or some signal that we are not raise an error when we see this?
There was a problem hiding this comment.
It isn't a good default. It was hard to imagine what circumstances would lead to having to delocate a Python library using @executable_path, but I put something there anyway.
I can use sys.base_exec_prefix to make @executable_path relative to the Python executable. That seems like a reasonable choice.
There was a problem hiding this comment.
dirname(sys.executable) is now the default for @executable_path, and this can be changed from the command line.
|
Did you have any thoughts on the right way to close #7 ? |
HexDecimal
force-pushed
the
loader_path
branch
6 times, most recently
from
a0de7bd
to
51bc7e7
Compare
This name was given a specific type and can not be reassigned to a plain tuple.
|
A pull request into your branch to fix an error I was getting testing locally: HexDecimal#2 |
I modified the rpath test wheel to add an indirect dependency very similar to what SDL has. I think my wheels should be okay as long as that test passes. |
* MRG: Allow for realpath differences in listdeps I was getting an error locally running the tests, because my code directory realpath differs from the apparent path.
|
@isuruf - we're planning to merge this branch soon - can you test it for your needs and let us know? |
|
I checked this PR and with isuruf@6717410 added on top of this PR, numpy arm64 wheels work correctly. |
|
@HexDecimal - a couple of test failures on Travis - would you mind taking a look? |
Early versions of this fixture could not be used as a path directly. The parameter is now converted into str before use.
|
OK - let's give this a try. Thanks @HexDecimal for all your patient work. |
Added more stuff on top of my previous PR's. This adds a
DependencyTreeclass which uses itself to resolve dependencies recursively and is a little easier to read its attributes than parse thetree_libsreturn value. I tried to use it to updatetree_libsitself but it turns out everything relies on its original behavior.Tests fail unless I remove recursion and remove libraries with
@loader_pathin their install name. That's the current state of things. It might work better if I kept a backwards compatibletree_libsand started switching tools to useDependencyTreeinstead.Closes #91, my previous PR.
Closes #37, closes #62, closes #90, open issues involving missing
@loader_pathsupport.#7 might be solved with additional commits to
delocate_listdeps.py.Still a lot to work on obviously. I just made this PR to report the current status and so that it doesn't clobber #91 which I might need to return to if this PR doesn't turn out well. I've only really gotten familiar with
libsana.pyat the moment.I'll also need to add tests for
DependencyTreeitself. I doubt its__repr__works.