Fix: Loading library on Aarch64 fails because pylink attempts to load 32-bit library

[FletcherD]

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.

[CLAassistant]

All committers have signed the CLA.

[hkpeprah]

Can we add a unit test for this as well? Capturing the scenario you're looking at?

[hkpeprah]

Is there a specific error we could catch here instead of a raw except?

[FletcherD]

Let's try catching OSError, that's what we get in this scenario

[hkpeprah]

Suggested change

	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():

[hkpeprah]

Should this check actually occur after the OS check?

[FletcherD]

Yes, should only be checked on 64 bit OSes. Changed

[FletcherD]

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.

[hkpeprah]

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 mock to fake the architecture. There's some examples of this in the unit test files: https://github.com/square/pylink/blob/master/tests/unit/test_library.py#L953

[markahinkle]

Super helpful fix, thanks ! Hopefully this gets merged soon

[hkpeprah]

Since this has been sitting for awhile, I'll try to add tests, and get this merged in.

[hkpeprah]

Here's a diff with working unit tests:

diff --git a/pylink/library.py b/pylink/library.py
index 6361abf..ca4d33f 100644
--- a/pylink/library.py
+++ b/pylink/library.py
@@ -143,10 +143,11 @@ class Library(object):
         """Test whether a library is the correct architecture to load.
 
         Args:
-          dllpath: A path to a library.
+          cls (Library): the ``Library`` class
+          dllpath (str): A path to a library.
 
         Returns:
-          True if the library could be successfully loaded, False if not.
+          ``True`` if the library could be successfully loaded, ``False`` if not.
         """
         try:
             ctypes.CDLL(dllpath)
diff --git a/tests/unit/test_library.py b/tests/unit/test_library.py
index fb92f92..e12f967 100644
--- a/tests/unit/test_library.py
+++ b/tests/unit/test_library.py
@@ -880,13 +880,16 @@ class TestLibrary(unittest.TestCase):
     @mock.patch('tempfile.NamedTemporaryFile', new=mock.Mock())
     @mock.patch('ctypes.util.find_library')
     @mock.patch('ctypes.cdll.LoadLibrary')
+    @mock.patch('ctypes.CDLL')
     @mock.patch('pylink.library.os')
-    def test_linux_6_10_0_64bit(self, mock_os, mock_load_library, mock_find_library, mock_open, mock_is_os_64bit):
+    def test_linux_6_10_0_64bit(self, mock_os, mock_cdll, mock_load_library,
+                                mock_find_library, mock_open, mock_is_os_64bit):
         """Tests finding the DLL on Linux through the SEGGER application for V6.0.0+ on 64 bit linux.
 
         Args:
           self (TestLibrary): the ``TestLibrary`` instance
           mock_os (Mock): a mocked version of the ``os`` module
+          mock_cdll (Mock): a mocked version of the `cdll.CDLL` class constructor
           mock_load_library (Mock): a mocked version of the library loader
           mock_find_library (Mock): a mocked call to ``ctypes`` find library
           mock_open (Mock): mock for mocking the call to ``open()``
@@ -896,6 +899,7 @@ class TestLibrary(unittest.TestCase):
           ``None``
         """
         mock_find_library.return_value = None
+        mock_cdll.return_value = None
         directories = [
             '/opt/SEGGER/JLink_Linux_V610d_x86_64/libjlinkarm_x86.so.6.10',
             '/opt/SEGGER/JLink_Linux_V610d_x86_64/libjlinkarm.so.6.10',
@@ -918,6 +922,49 @@ class TestLibrary(unittest.TestCase):
         lib.unload = mock.Mock()
         self.assertEqual(None, lib._path)
 
+    @mock.patch('sys.platform', new='linux2')
+    @mock.patch('pylink.util.is_os_64bit', return_value=True)
+    @mock.patch('pylink.library.open')
+    @mock.patch('os.remove', new=mock.Mock())
+    @mock.patch('tempfile.NamedTemporaryFile', new=mock.Mock())
+    @mock.patch('ctypes.util.find_library')
+    @mock.patch('ctypes.cdll.LoadLibrary')
+    @mock.patch('ctypes.CDLL')
+    @mock.patch('pylink.library.os')
+    def test_linux_64bit_no_x86(self, mock_os, mock_cdll, mock_load_library,
+                                mock_find_library, mock_open, mock_is_os_64bit):
+        """Tests finding the DLL on Linux when no library name contains 'x86'.
+
+        Args:
+          self (TestLibrary): the ``TestLibrary`` instance
+          mock_os (Mock): a mocked version of the ``os`` module
+          mock_cdll (Mock): a mocked version of the `cdll.CDLL` class constructor
+          mock_load_library (Mock): a mocked version of the library loader
+          mock_find_library (Mock): a mocked call to ``ctypes`` find library
+          mock_open (Mock): mock for mocking the call to ``open()``
+          mock_is_os_64bit (Mock): mock for mocking the call to ``is_os_64bit``, returns True
+
+        Returns:
+          ``None``
+        """
+        def on_cdll(name):
+            if '_arm' in name:
+                raise OSError
+
+        mock_find_library.return_value = None
+        mock_cdll.side_effect = on_cdll
+        directories = [
+            '/opt/SEGGER/JLink_Linux_V610d_x86_64/libjlinkarm_arm.so.6.10',
+            '/opt/SEGGER/JLink_Linux_V610d_x86_64/libjlinkarm.so.6.10',
+        ]
+
+        self.mock_directories(mock_os, directories, '/')
+
+        lib = library.Library()
+        lib.unload = mock.Mock()
+        load_library_args, load_libary_kwargs = mock_load_library.call_args
+        self.assertEqual(directories[1], lib._path)
+
     @mock.patch('sys.platform', new='linux')
     @mock.patch('pylink.library.open')
     @mock.patch('os.remove', new=mock.Mock())
@@ -960,8 +1007,9 @@ class TestLibrary(unittest.TestCase):
     @mock.patch('pylink.platform.libc_ver', return_value=('libc', '1.0'))
     @mock.patch('ctypes.util.find_library', return_value='libjlinkarm.so.7')
     @mock.patch('pylink.library.JLinkarmDlInfo.__init__')
+    @mock.patch('ctypes.CDLL')
     @mock.patch('ctypes.cdll.LoadLibrary')
-    def test_linux_glibc_unavailable(self, mock_load_library, mock_dlinfo_ctr, mock_find_library,
+    def test_linux_glibc_unavailable(self, mock_load_library, mock_cdll, mock_dlinfo_ctr, mock_find_library,
                                      mock_libc_ver, mock_is_os_64bit, mock_os,  mock_open):
         """Confirms the whole JLinkarmDlInfo code path is not involved when GNU libc
         extensions are unavailable on a Linux system, and that we'll successfully fallback
@@ -974,6 +1022,7 @@ class TestLibrary(unittest.TestCase):
           to the "search by file name" code path, aka find_library_linux()
         - and "successfully load" a mock library file from /opt/SEGGER/JLink
         """
+        mock_cdll.side_effect = None
         directories = [
             # Library.find_library_linux() should find this.
             '/opt/SEGGER/JLink/libjlinkarm.so.6'
@@ -999,8 +1048,9 @@ class TestLibrary(unittest.TestCase):
     @mock.patch('pylink.util.is_os_64bit', return_value=True)
     @mock.patch('pylink.platform.libc_ver', return_value=('glibc', '2.34'))
     @mock.patch('ctypes.util.find_library')
+    @mock.patch('ctypes.CDLL')
     @mock.patch('ctypes.cdll.LoadLibrary')
-    def test_linux_dl_unavailable(self, mock_load_library, mock_find_library, mock_libc_ver,
+    def test_linux_dl_unavailable(self, mock_load_library, mock_cdll, mock_find_library, mock_libc_ver,
                                   mock_is_os_64bit, mock_os,  mock_open):
         """Confirms we successfully fallback to the "search by file name" code path when libdl is
         unavailable despite the host system presenting itself as POSIX (GNU/Linux).
@@ -1012,6 +1062,7 @@ class TestLibrary(unittest.TestCase):
           to the "search by file name" code path, aka find_library_linux()
         - and "successfully load" a mock library file from /opt/SEGGER/JLink
         """
+        mock_cdll.side_effect = None
         mock_find_library.side_effect = [
             # find_library('jlinkarm')
             'libjlinkarm.so.6',

@FletcherD, could you apply it to your PR?

[FletcherD]

Sorry, just saw this. I've incorporated that diff, everything should be good to go.

[hkpeprah]

LGTM. I'm trying to figure out the issue with the CLA. Once I get that fixed, you'll have to sign the CLA, then I'll merge in the change and cut a release. Hopefully by EOD.

[hkpeprah]

Re-kicked the CLA Assistant. All looks good to me. Thanks for the patch. Will merge ASAP.