Skip to content
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

Merged
merged 23 commits into from
Feb 12, 2021
Merged

Improve how PyGMT finds the GMT library #702

merged 23 commits into from
Feb 12, 2021

Conversation

seisman
Copy link
Member

@seisman seisman commented Nov 24, 2020

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:

  1. the path defined by GMT_LIBRARY_PATH
  2. the path returned by the 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 yields a generator (not a list)
of possible paths, 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
  2. Only calls the gmt command if GMT_LIBRARY_PATH is not defined

Cons:

1. A much longer list returned by clib_full_names. Maybe slower loading?
2. Difficult to test.

TODO

  • Need to rewrite some tests in the test_clib_loading.py file.

Closes #628.

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If adding new functionality, add an example to docstrings or tutorials.

Notes

  • You can write /format in the first line of a comment to lint the code automatically

@seisman seisman changed the title Improve how PyGMT find the GMT library Improve how PyGMT finds the GMT library Nov 24, 2020
@seisman seisman requested a review from a team November 24, 2020 03:48
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.
@seisman seisman added the enhancement Improving an existing feature label Nov 29, 2020
@seisman seisman added this to the 0.3.0 milestone Nov 29, 2020
@seisman seisman added this to In progress in Release v0.3.x Nov 29, 2020
@seisman seisman marked this pull request as ready for review November 29, 2020 22:42
@seisman seisman requested review from a team and removed request for a team November 29, 2020 22:42
@seisman seisman requested review from a team and removed request for a team November 30, 2020 19:44
pygmt/clib/loading.py Outdated Show resolved Hide resolved
@seisman seisman marked this pull request as draft December 1, 2020 20:26
@leouieda
Copy link
Member

leouieda commented Dec 2, 2020

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

  • I initially made the Session class load the library when it's used to avoid doing this work at import time. Mostly so we wouldn't have GMTLibNotFoundError at import.
  • That clearly didn't work because we need to call begin at import for this whole thing to work.
  • So it might be time to rethink loading libgmt in the class instead of as a global instead to avoid searching for it every time.
  • Or we figure out a way to not call begin at import time.

@leouieda
Copy link
Member

leouieda commented Dec 2, 2020

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.

Copy link
Member

@weiji14 weiji14 left a 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).

@seisman seisman marked this pull request as ready for review February 12, 2021 07:14
@seisman
Copy link
Member Author

seisman commented Feb 12, 2021

@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:

  • GMT_LIBRAYR_PATH defined
  • PATH defined, GMT_LIBRARY_PATH undefined
  • both PATH and GMT_LIBRARY_PATH defined
  • both PATH and GMT_LIBRARY_PATH undefined, but DYLD_LIBRARY_PATH defined

They all work as I expect (e.g., no extra system calling if the library is already found via GMT_LIBRARY_PATH.

I think this PR is ready to merge. Do we want to merge it into v0.3.0?

@weiji14
Copy link
Member

weiji14 commented Feb 12, 2021

/test-gmt-master

I think this PR is ready to merge. Do we want to merge it into v0.3.0?

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.

@weiji14 weiji14 merged commit c869531 into master Feb 12, 2021
Release v0.3.x automation moved this from In progress to Done Feb 12, 2021
@weiji14 weiji14 deleted the gmt-library branch February 12, 2021 09:24
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
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]>
@weiji14 weiji14 mentioned this pull request Oct 26, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improving an existing feature
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Find the GMT library by calling "gmt --show-library"
3 participants