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 #440

Merged
merged 8 commits into from
May 21, 2020
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions .azure-pipelines.yml
Original file line number Diff line number Diff line change
Expand Up @@ -179,8 +179,6 @@ jobs:
CONDA_REQUIREMENTS: requirements.txt
CONDA_REQUIREMENTS_DEV: requirements-dev.txt
CONDA_INSTALL_EXTRA: "codecov gmt=6.0.0"
# ctypes.CDLL cannot find conda's libraries
GMT_LIBRARY_PATH: 'C:\Miniconda\envs\testing\Library\bin'

strategy:
matrix:
Expand Down
53 changes: 39 additions & 14 deletions pygmt/clib/loading.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,24 +7,19 @@
import os
import sys
import ctypes
from ctypes.util import find_library

from ..exceptions import GMTOSError, GMTCLibError, GMTCLibNotFoundError


def load_libgmt(env=None):
def load_libgmt():
"""
Find and load ``libgmt`` as a :py:class:`ctypes.CDLL`.

By default, will look for the shared library in the directory specified by
the environment variable ``GMT_LIBRARY_PATH``. If it's not set, will let
ctypes try to find the library.

Parameters
----------
env : dict or None
A dictionary containing the environment variables. If ``None``, will
default to ``os.environ``.

Returns
-------
:py:class:`ctypes.CDLL` object
Expand All @@ -37,22 +32,21 @@ def load_libgmt(env=None):
couldn't access the functions).

"""
if env is None:
env = os.environ
libnames = clib_name(os_name=sys.platform)
libpath = env.get("GMT_LIBRARY_PATH", "")
lib_fullnames = clib_full_names()
error = True
for libname in libnames:
for libname in lib_fullnames:
try:
libgmt = ctypes.CDLL(os.path.join(libpath, libname))
libgmt = ctypes.CDLL(libname)
check_libgmt(libgmt)
error = False
break
except OSError as err:
error = err
if error:
raise GMTCLibNotFoundError(
seisman marked this conversation as resolved.
Show resolved Hide resolved
"Error loading the GMT shared library '{}':".format(", ".join(libnames))
"Error loading the GMT shared library '{}':".format(
", ".join(lib_fullnames)
)
)
return libgmt

Expand Down Expand Up @@ -84,6 +78,37 @@ def clib_name(os_name):
return libname
seisman marked this conversation as resolved.
Show resolved Hide resolved


def clib_full_names(env=None):
"""
Return the full path of GMT's shared library for the current OS.

Parameters
----------
env : dict or None
A dictionary containing the environment variables. If ``None``, will
default to ``os.environ``.

Returns
-------
lib_fullnames: list of str
List of possible full names of GMT's shared library.

"""
if env is None:
env = os.environ
libnames = clib_name(os_name=sys.platform) # e.g. libgmt.so, libgmt.dylib, gmt.dll
libpath = env.get("GMT_LIBRARY_PATH", "") # e.g. $HOME/miniconda/envs/pygmt/lib

lib_fullnames = [os.path.join(libpath, libname) for libname in libnames]
# Search for DLLs in PATH if GMT_LIBRARY_PATH is not defined [Windows only]
if not libpath and sys.platform == "win32":
for libname in libnames:
libfullpath = find_library(libname)
if libfullpath:
lib_fullnames.append(libfullpath)
Comment on lines +105 to +108
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member Author

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'

Copy link
Member

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 😄

return lib_fullnames


def check_libgmt(libgmt):
"""
Make sure that libgmt was loaded correctly.
Expand Down
7 changes: 0 additions & 7 deletions pygmt/tests/test_clib_loading.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,6 @@ def test_load_libgmt():
check_libgmt(load_libgmt())


def test_load_libgmt_fail():
"Test that loading fails when given a bad library path."
env = {"GMT_LIBRARY_PATH": "not/a/real/path"}
with pytest.raises(GMTCLibNotFoundError):
load_libgmt(env=env)


seisman marked this conversation as resolved.
Show resolved Hide resolved
def test_clib_name():
"Make sure we get the correct library name for different OS names"
for linux in ["linux", "linux2", "linux3"]:
Expand Down