-
Notifications
You must be signed in to change notification settings - Fork 125
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
Fix: Loading library on Aarch64 fails because pylink attempts to load 32-bit library #182
base: master
Are you sure you want to change the base?
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.
Can we add a unit test for this as well? Capturing the scenario you're looking at?
pylink/library.py
Outdated
try: | ||
ctypes.CDLL(dllpath) | ||
return True | ||
except: |
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.
Is there a specific error we could catch here instead of a raw except
?
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.
Let's try catching OSError
, that's what we get in this scenario
pylink/library.py
Outdated
if not cls.can_load_library(fpath): | ||
continue | ||
if util.is_os_64bit(): |
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.
if not cls.can_load_library(fpath): | |
continue | |
if util.is_os_64bit(): | |
if not cls.can_load_library(fpath): | |
continue | |
elif util.is_os_64bit(): |
pylink/library.py
Outdated
@@ -202,6 +218,8 @@ def find_library_linux(cls): | |||
|
|||
for fname in fnames: | |||
fpath = os.path.join(directory_name, fname) | |||
if not cls.can_load_library(fpath): |
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.
Should this check actually occur after the OS check?
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.
Yes, should only be checked on 64 bit OSes. Changed
Not sure how to write a unit test for this - seems to require a ARM64 architecture system to test? Unless there's some way to emulate that. |
You can use |
Super helpful fix, thanks ! Hopefully this gets merged soon |
Since this has been sitting for awhile, I'll try to add tests, and get this merged in. |
Here's a diff with working unit tests:
@FletcherD, could you apply it to your PR? |
The ARM64 version of the JLink software contains two library files, libjlinkarm.so (64 bit) and libjlinkarm_arm.so (32 bit). Pylink attempts to load libjlinkarm_arm.so first, which fails on ARM64 due to the architecture mismatch and causes an error. This change simply performs a 'test load' of each library file found, skipping if it fails, fixing this issue on ARM64.