-
Notifications
You must be signed in to change notification settings - Fork 224
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
Improve how PyGMT finds the GMT library #702
Conversation
In the old codes, PyGMT first checks if the environmental variable `GMT_LIBRARY_PATH` is defined. If yes, it tries to find the GMT library in the specified path. Otherwise, it will search for the GMT library in standard library paths (LD_LIBRARY_PATH on Linux; DYLD_LIBRARY_PATH on macOS; PATH on Windows). This PR improve how PyGMT find the library, by search for all possible paths. The paths from the highest priority to the lowest priority are: 1. the path defined by `GMT_LIBRARY_PATH` 2. the path returned by command `gmt --show-library` 3. On Windows, also check the path returned by `find_library` function 4. the standard system paths Thus, the function `clib_full_names` now returns a list of possible names, for example: ``` ["/path/defined/by/gmtlibrarypath/libgmt.so", "/usr/local/lib/libgmt.so", "libgmt.so"] ``` **Pros**: 1. More ways to find the library, so less "GMTCLibNotFoundError" errors **Cons**: 1. It calls the "gmt" command in a new process 2. A much longer list returned by `clib_full_names`. Maybe slower loading? 3. Difficult to test. Closes #628.
Thanks, @seisman! This is a good thing to have and I agree that it's worth the downsides. A few thoughts on the design of loading the library as a whole (might be out of scope for this PR but worth mentioning):
|
And yes, these are the trickiest things to test. Using pytest's monkeypatch is usually a good way to do it so you can trigger errors yourself. |
Cleaner to use pytest's `monkeypatch.setenv` fixture to set an environment variable, see https://docs.pytest.org/en/4.6.x/monkeypatch.html#monkeypatching-environment-variables
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.
Cons:
1. It calls the "gmt" command in a new process 2. A much longer list returned by `clib_full_names`. Maybe slower loading?
These cons have been mitigated somewhat in commit 5f17f1e, so the gmt
subprocess is only called if necessary (i.e. when looking in GMT_LIBRARY_PATH or the default PATH doesn't work).
3. Difficult to test.
Monkeypatch tests have been added in test_clib_loading.py at commit 243ad13 so there should be full test coverage now. @GenericMappingTools/python-maintainers, give this a review once you have time (particularly on Windows if possible).
@weiji14 Great work! I just made a few commits to this branch (dbfd7e2, 6769dc3 and c3887ce). I tested the branch on macOS for many different cases:
They all work as I expect (e.g., no extra system calling if the library is already found via I think this PR is ready to merge. Do we want to merge it into v0.3.0? |
/test-gmt-master
Yes please! Once the tests pass. I think there's still some missing code coverage? But we can improve on things later in another iteration. |
Searches for all possible GMT library paths to load. The paths from the highest priority to the lowest priority are: 1. the path defined by `GMT_LIBRARY_PATH` 2. the path returned by command `gmt --show-library` 3. On Windows, also check the path returned by `find_library` function 4. the standard system paths * Check library names for FreeBSD * Use generator yield in clib_full_names function * Refactor test_load_libgmt_with_a_bad_library_path to use monkeypatch * Monkeypatch test to check that GMTCLibNotFoundError is raised properly * Check if the GMT shared library exists in GMT_LIBRARY_PATH Co-authored-by: Wei Ji <[email protected]>
In the old codes, PyGMT first checks if the environmental variable
GMT_LIBRARY_PATH
is defined. If yes, it tries to find the GMT library in the specified path.
Otherwise, it will search for the GMT library in standard library paths
(LD_LIBRARY_PATH on Linux; DYLD_LIBRARY_PATH on macOS; PATH on Windows).
This PR improves how PyGMT finds the library, by searching for all possible
paths. The paths from the highest priority to the lowest priority are:
GMT_LIBRARY_PATH
gmt --show-library
find_library
functionThus, the function
clib_full_names
now yields a generator (not a list)of possible paths, for example:
Pros:
gmt
command ifGMT_LIBRARY_PATH
is not definedCons:
1. A much longer list returned byclib_full_names
. Maybe slower loading?2. Difficult to test.TODO
test_clib_loading.py
file.Closes #628.
Reminders
make format
andmake check
to make sure the code follows the style guide.doc/api/index.rst
.Notes
/format
in the first line of a comment to lint the code automatically