-
Notifications
You must be signed in to change notification settings - Fork 234
Improve how PyGMT finds the GMT library #440
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
Conversation
|
Ok, just woke up and finished breakfast. Just to make sure we're on the same page, we'll be releasing all of these 'Windows' fixes into |
|
Yes, v0.1.0 is good. Perhaps easier to rename the current 0.2.0 milestone to 0.1.1. |
|
Or we could rename the milestones to |
|
@weiji14 I think we finally have a good solution for the Windows issue. No workaround is needed anymore. Please review the PR. I bet more tests should be added, but I can't see the coverage report. |
|
Awesome, give me a while, there's a lot to review... Not sure what's up with codecov but we can use |
weiji14
left a comment
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.
Cool, this looks good. I think we can also improve the documentation section at https://github.com/GenericMappingTools/pygmt/blob/cdll-path/doc/install.rst#finding-the-gmt-shared-library to add instruction on how to set the GMT_LIBRARY_PATH on MacOS and Windows (instead of just Linux).
| for libname in libnames: | ||
| libfullpath = find_library(libname) | ||
| if libfullpath: | ||
| lib_fullnames.append(libfullpath) |
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.
These lines were missing on my test coverage (since I'm on Linux) so I can't check this properly. But looking at the Azure Pipelines test on Windows, it seems okay.
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 it OK to add a Windows-only test?
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.
The find_library function is cross platform, I wonder if we should remove the sys.platform == 'win32' requirement so that it's easier for Linux/MacOS users too. Comes at the risk of users picking up random libgmt libraries from elsewhere though.
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 if find_library works on macOS. I can import pygmt correctly, which means ctypes.CDLL("libgmt.dylib") works. However find_library` returns nothing for me.
In [1]: import pygmt
In [2]: from ctypes.util import find_library
In [3]: find_library("gmt")
In [4]: find_library("libgmt")
In [5]: find_library("libgmt.dylib")
In [6]: find_library("m")
Out[6]: '/usr/lib/libm.dylib'
In [7]: find_library("libm")
Out[7]: '/usr/lib/libm.dylib'
In [8]: find_library("libm.dylib")
Out[8]: '/usr/lib/libm.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.
Could have sworn someone mentioned this before, but a quick scan through the old issues turn up nothing. Should be good to go then, but will keep it in the back of my mind 😄
Co-authored-by: Wei Ji <[email protected]>
Let's put it in another PR. |
Description of proposed changes
On Windows, ctypes can't find the gmt library in conda's path. The reason is still unclear to me. But ctypes provides a function find_library which search DLLs in the system search path (i.e., the variable PATH).
Thus, on Windows, if GMT_LIBRARY_PATH is not defined, we first call
find_libraryto check if gmt.dll, gmt_w64 or gmt_w32 exists in one of the PATH.find_libraryreturns its full path (e.g.,C:\Miniconda\envs\testing\Library\bin\gmt.dll) if it's found, else returns None.The new function
clib_full_namesreturns the list of full names to the gmt library. When GMT_LIBRARY_PATH is not defined, it returns["libgmt.dylib"]["libgmt.so"]["gmt.dll", "gmt_w64.dll", "gmt_w32.dll", "C:\Miniconda\envs\testing\Library\bin\gmt.dll"].On Windows, the full path returned by
find_libraryis placed at the end of the list, to better mimic the library search order on Windows.With this PR merged, the variables GMT_LIBRARY_PATH is no longer required for Windows users.
Reminders
make formatandmake checkto make sure the code follows the style guide.doc/api/index.rst.