GenericMappingTools / pygmt

A Python interface for the Generic Mapping Tools.
https://www.pygmt.org
BSD 3-Clause "New" or "Revised" License
747 stars 216 forks source link

test_load_libgmt_with_a_bad_library_path fails with GMT dev versions on Linux and macOS #874

Closed seisman closed 3 years ago

seisman commented 3 years ago

Description of the problem

In recent runs (https://github.com/GenericMappingTools/pygmt/actions/runs/560287188), test_load_libgmt_with_a_bad_library_path fails with GMT dev on Linux and macOS, but pass on Windows, possibly because we merged #702. However, it passed when @weiji14 triggered the dev tests before merging (https://github.com/GenericMappingTools/pygmt/actions/runs/560286224).

It's unclear to me what happens. We can wait for today's scheduled runs and see if it works or not.

See the error messages below:

___________________ test_load_libgmt_with_a_bad_library_path ___________________

monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x7f462205dca0>

    def test_load_libgmt_with_a_bad_library_path(monkeypatch):
        """
        Test that loading still works when given a bad library path.
        """
        # Set a fake "GMT_LIBRARY_PATH"
        monkeypatch.setenv("GMT_LIBRARY_PATH", "/not/a/real/path")
>       assert check_libgmt(load_libgmt()) is None

/usr/share/miniconda3/envs/test/lib/python3.9/site-packages/pygmt/tests/test_clib_loading.py:48: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

    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.

        Returns
        -------
        :py:class:`ctypes.CDLL` object
            The loaded shared library.

        Raises
        ------
        GMTCLibNotFoundError
            If there was any problem loading the library (couldn't find it or
            couldn't access the functions).
        """
        lib_fullnames = []
        error = True
        for libname in clib_full_names():
            lib_fullnames.append(libname)
            try:
                libgmt = ctypes.CDLL(libname)
                check_libgmt(libgmt)
                error = False
                break
            except OSError as err:
                error = err
        if error:
>           raise GMTCLibNotFoundError(
                "Error loading the GMT shared library "
                f"{', '.join(lib_fullnames)}.\n {error}."
            )
E           pygmt.exceptions.GMTCLibNotFoundError: Error loading the GMT shared library libgmt.so.
E            libgmt.so: cannot open shared object file: No such file or directory.
seisman commented 3 years ago

Here is what happens. For GMT dev builds,

Windows: We use the conda-forge's GMT dev pacakges because we can't (don't) build GMT source codes on Windows. Obviously, conda adds the bin path to PATH so that we can call gmt --show-library to find the library, and the test passes.

Linux/macOS: We build GMT source codes and install it to ${{ github.workspace }}/gmt-install-dir, and we set GMT_LIBRARY_PATH to ${{ github.workspace }}/gmt-install-dir/lib but don't add it's bin path to PATH, so PyGMT can't find the GMT library.

We have two ways to fix the problem:

  1. On Linux/macOS, add GMT's bin path to PATH
  2. Remove the test, because we're going to add more comprehensive tests in #872
seisman commented 3 years ago
  • Remove the test, because we're going to add more comprehensive tests in #872

Maybe no, because the new tests in #872 need GMT's bin in PATH.

  • On Linux/macOS, add GMT's bin path to PATH

This is the preferred way.

Thoughts?

weiji14 commented 3 years ago

Linux/macOS: We build GMT source codes and install it to ${{ github.workspace }}/gmt-install-dir, and we set GMT_LIBRARY_PATH to ${{ github.workspace }}/gmt-install-dir/lib but don't add it's bin path to PATH, so PyGMT can't find the GMT library.

I don't get it, so how did pygmt/clib/loading.py find the GMT bin before #702 even without us setting it? This seems like a regression bug to me.

seisman commented 3 years ago

The test is new in #702, no?

weiji14 commented 3 years ago

Well, that test (test_load_libgmt_fails) was actually modified from a previous test. Before, the library loading would just fail when a fake "GMT_LIBRARY_PATH" is set because we didn't have extra options. After, the library loading should pass because we check for other paths (using gmt --show-library and standard system paths). I think you're right that we still need to set GMT's bin in the system PATH.