-
Notifications
You must be signed in to change notification settings - Fork 59
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
Handle C extensions not inside a package directory (at the root) #39
Conversation
0dcf21e
to
0667367
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - and sorry to be so slow to review, the start of term got the better of me.
delocate/tools.py
Outdated
@@ -382,8 +382,8 @@ def dir2zip(in_dir, zip_fname): | |||
z.close() | |||
|
|||
|
|||
def find_package_dirs(root_path): | |||
""" Find python package directories in directory `root_path` | |||
def find_packages(root_path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry - I'm a little sensitive about changing the API without warning.
Would you mind making and deprecating something like:
def find_package_dirs(root_path):
return [p for p, is_dir in find_packages(root) if is_dir]
and importing into delocating
?
delocate/tools.py
Outdated
@@ -393,14 +393,17 @@ def find_package_dirs(root_path): | |||
Returns | |||
------- | |||
package_sdirs : set | |||
Set of strings where each is a subdirectory of `root_path`, containing | |||
an ``__init__.py`` file. Paths prefixed by `root_path` | |||
Set of strings where each is a subdirectory of `root_path` containing an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually a set of (string, is_dir) tuples no?
delocate/tools.py
Outdated
""" | ||
package_sdirs = set() | ||
for entry in os.listdir(root_path): | ||
fname = entry if root_path == '.' else pjoin(root_path, entry) | ||
if isdir(fname) and exists(pjoin(fname, '__init__.py')): | ||
package_sdirs.add(fname) | ||
package_sdirs.add((fname, True)) | ||
elif isfile(fname) and ((fname.endswith('.so') or fname.endswith('.dylib'))): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use lib_filt_func
here? See delocate_wheel
docstring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to use lib_filt_func
here - here we're not checking the dependencies of found objects, but only whether a member of root_path
is a module. What would lib_filt_func is None
mean in this context? It can't mean that every directory/file found is a package, nor should lib_filt_func == "dylibs-only"
cause the function to ignore packages in subdirs.
Perhaps I should move _dylibs_only()
to tools
and use it here (and import it in delocating
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure sure I understand, I might have been too hasty but lib_filt_func
is to decide what files to consider when looking for libraries. I guess it's fine print. Yes, moving to tools
sounds like a reasonable thing to do.
eac59ce
to
aa5c4ab
Compare
looks like this has a conflict now -- I'm very interested in using this branch |
@natefoo - OK to rebase? I promise to review quickly. |
the conflict is pretty trivial -- it's just mad about imports -- (can compare against https://github.com/asottile/delocate/tree/top-level-fix-squash_asottile) |
aa5c4ab
to
58106c6
Compare
@matthew-brett ping, I rebased this (also feel free to modify directly). |
FWIW, I'm using this patch to build |
@matthew-brett Anything I help with to get this over the line? |
reaching for this again -- unfortunately the conflict is pretty messy now so I'm having trouble rebasing this :( |
- Incorporate 9f042a8 into pieces of PR matthew-brett#39 - Delegate to _analyze_path()
@bsolomon1124, thank you for continue working on this. BTW recently per pypa/auditwheel#90, auditwheel has just adopted the new naming scheme for dynamic libraries ( |
Superseded by #123. |
@matthew-brett how's this look for an implementation? It's working for me but needs a few tests.
Root packages end up with a
.dylib<pkg>
directory for delocated libs, this mirrors what auditwheel does.