Add support for 64bit Python on Windows #25
Conversation
Standard set of tools delivered by Segger is targetted for 32 bit platform, however there is a DLL for 64 bit application named `JLink_x64.dll`. This change takes that into account and checks the Python version before selecting the DLL to load.
… the sake of further generations :-)
…K_SDK_NAME` uses in the first place. Now sorted.
|
@hkpeprah Sorry, I stopped understanding why these tests fail at this point. Any clue? |
|
This sounds like a good idea. Before we can accept your submission though, you have to sign the CLA here: Individual Contributor License Agreement. I'll take a look at the test failure shortly to give you a hand. |
There was a problem hiding this comment.
The tests are failing because they DLL isn't being found. If you look in the tests, you'll see a variable called directories as:
directories = [
'C:\\Program Files (x86)\\SEGGER\\JLinkARM\\JLinkARM.dll',
'C:\\Program Files (x86)\\SEGGER\\JLink_V500l\\JLinkARM.dll'
]
They're failing since the 64-bit DLL isn't there. You should add a separate test for 64-bit which mocks the platform as 64-bit, and mock the existing Windows library tests so that they think they're running on a 32-bit platform. I'd also update the tests for Windows so they contain both the 32-bit and 64-bit DLLs so that we verify we choose the right one.
CONTRIBUTORS.md
Outdated
| | Charles Nicholson | @charlesnicholson | | ||
| | Marek Novak | @MarekNovakNXP | | ||
| | Michał Fita | @michalfita | | ||
|
|
There was a problem hiding this comment.
NIT: remove this extra trailing newline.
| WINDOWS_64_JLINK_SDK_NAME = 'JLink_x64' | ||
|
|
||
| @classmethod | ||
| def get_appropriate_windows_sdk_name(cls): |
There was a problem hiding this comment.
I wouldn't create a new function to do this. Just have the call in find_library_windows() have an if-statement. Also, sys.maxsize is the correct one I think, so that always returns the interpreter's size.
if sys.maxsize > 2 ** 32:
dll = WINDOWS_64_JLINK_SDK_NAME + '.dll'
else:
dll = WINDOWS_32_JLINK_SDK_NAME + '.dll'
There was a problem hiding this comment.
I'm surprised somebody refuse a function for duplicated logic - we need that in at least two places in the code.
There was a problem hiding this comment.
Ah, I see now. I think it would be better to just get rid of the _sdk attribute, and have the find_library_{platform} methods call the ctypes_util.find_library method themselves on the SDK name, then fallback to the path searching. That is probably the correct way to begin with.
There was a problem hiding this comment.
@hkpeprah I don't think I understand what do you want to have done. And I'm deeply sorry, but I don't have more resources to work on refactoring of this PR. I made it working for our QA engineers, but I'm currently very busy on something completely unrelated.
You current code doesn't support 64 bit Python, so it's going to affect most Windows 10 users - probably not very many. Most people probably will not understand the error on attempt, so they'll just surrender the use. I think it's better to have this change as 64 bit support than don't and wait 10 years until I have time to return to this.
tests/unit/test_library.py
Outdated
| @@ -246,7 +248,10 @@ def test_initialize_windows(self, mock_ctypes, mock_find_library, mock_open): | |||
| lib = library.Library() | |||
| lib.unload = mock.Mock() | |||
|
|
|||
| mock_find_library.assert_called_once_with(library.Library.WINDOWS_JLINK_SDK_NAME) | |||
| if platform.architecture()[0] == '64bit': | |||
There was a problem hiding this comment.
There should be a separate test for 64-bit where we mock and force the system to be 64-bit. The existing tests should be mocked to treat the system as 32-bit.
|
@michalfita Sorry about the wait on this. When I initially reviewed, I thought I would have time to look at a way to have this change work for everything (and not just Windows), but the time hasn't come up for it (maybe over the holidays). Will merge as is, as there's quite a few Windows users running into the issue on 10. Thanks for the patch! |
Standard set of tools delivered by Segger is targetted for 32 bit platform, however there is a DLL for 64 bit application named
JLink_x64.dll. This change takes that into account and checks the Python version before selecting the DLL to load.This addresses closed issue #10 as I figured out the problem basing on the same error observed.