-
Notifications
You must be signed in to change notification settings - Fork 145
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
Check external dependencies versioned symbols #150
Conversation
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! I tested this with pygame, and it appears to be working as expected. It's nicer than my solution in #148, because it fails before producing the erroneous manylinux wheel.
However, I'm having a hard time reading the code. I've put some suggestions inline for how I think it could be more readable. Or if someone smarter than me can read the code and sign off on it, I'm happy to defer to them. :-)
5c0f3ab
to
5c47630
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 - it's definitely getting clearer. I still struggle to follow it at the moment, but that's the peril of reviewing when I'm tired. I'll try to have a look tomorrow when I'm hopefully a bit more awake.
@takluyver, I pushed 2 commits that should answer your comments |
Thanks, the comments really do help me understand what's going on. There are some rather long lines there now. I'm not picky about any particular column limit, but some of these lines are definitely longer than is comfortably readable. Do you have time to reformat the code a bit? Or I can have a go at this if you prefer. |
Now #158 added a linter to the test suite, so we will need to stick to some column limit to satisfy that. |
I’ll rebase next week in order to get the linter checks |
I've rebased this PR on master, it now passes the lint check but I'm not quite happy with the type description of some variables. I don't know how to improve the readability of those comments, so feel free to comment on these. |
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.
Conditionally approved for another round of review.
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 did a quick review, mostly on style and structure. I think refactoring the main logic into separate functions would also help with your type comments as you could move them into the doctstrings as example inputs, e.g:
def foo_func(thing: dict):
"""This function does X and Y.
Parameters
----------
thing : dict
a thing to do stuff with
Example input: {'/path/to/external_ref.so.1.2.3': 'external_ref.so.1'}
"""
to be squashed
@auvipy I was going to review the changes. Why did you merge this? In the future could you wait until the person who requested changes approves the final version? |
OK sure. |
This should fix #146
Uses a different mechanism compared to #148, it checks before producing the wheel.
It checks external dependencies versioned symbols when first analyzing the wheel ABI.