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

Update GMT constant GMT_STR16 to GMT_VF_LEN for GMT API change in 6.1.0 #397

Merged
merged 1 commit into from
Mar 18, 2020

Conversation

seisman
Copy link
Member

@seisman seisman commented Mar 11, 2020

Description of proposed changes

PyGMT fails with the latest GMT master branch with the following error message:

GMTCLibError: Constant 'GMT_STR16' doesn't exits in libgmt.

If you have the GMT master branch installed, you can reproduce the issue by

import pygmt
fig = pygmt.Figure()
fig.basemap(region='0/10/0/10', projection='X10c', frame=True)
fig.plot(x=5, y=5, style='c0.2c')

The error is caused by the recent change of the length of virtual file names
from GMT_STR16 to GMT_VF_LEN in the core GMT
(see GenericMappingTools/gmt#2861).

Changing GMT_STR16 to GMT_VF_LEN is the easiest fix.
To keep compatibility with both GMT 6.0.0 and the upcomming 6.1.0,
this PR checks GMT version and use GMT_STR16 for 6.0.0, otherwise
use GMT_VF_LEN.

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.

PyGMT fails with the latest GMT master branch with the following error message:
```
GMTCLibError: Constant 'GMT_STR16' doesn't exits in libgmt.
```

To reproduce the issue, you can run:
```
import pygmt
fig = pygmt.Figure()
fig.basemap(region='0/10/0/10', projection='X10c', frame=True)
fig.plot(x=5, y=5, style='c0.2c')
```

The error is caused by the recent change of the length of virtual file names
from `GMT_STR16` to `GMT_VF_LEN` in the core GMT
(see GenericMappingTools/gmt#2861).

Changing `GMT_STR16` to `GMT_VF_LEN` is the easiest fix.
To keep compatibility with both GMT 6.0.0 and the upcomming 6.1.0,
this PR checks GMT version and use `GMT_STR16` for 6.0.0, otherwise
use `GMT_VF_LEN`.
@leouieda
Copy link
Member

@seisman thanks for the fix! Sorry I've been away from the project for so long. Hoping to get some time during spring break to do some long delayed refactoring here.

It's a bit bad that we're advertising the GMT API as a stable thing in the papers and then changing it like that. I understand that it probably needed to be done but this type of thing is what makes it hard to work on PyGMT. A better thing to do would have been to recommend using the new variable to keep the old one for compatibility until GMT 7.

Copy link
Member

@leouieda leouieda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs tests but I'm sure how we could go about doing that without a lot of refactoring. Merging it like this for now but we need to rethink a lot of the C interface layer.

@leouieda
Copy link
Member

We really do need to setup CI against GMT master to find these things.

@leouieda
Copy link
Member

Actually, if any one you have any suggesting for possible tests for this that would be great. I'm not keen on disabling the "required" checks for merging since that creates a bad precedent.

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.

We really do need to setup CI against GMT master to find these things.

Yes, testing against GMT master was mentioned before in #345. Let's merge this PR for now and continue the discussion there :)

@seisman
Copy link
Member Author

seisman commented Mar 18, 2020

I don't think it's possible to add any tests to make the coverage checker happy, since we can't simply test two GMT versions in one CI job.

Let's merge it first.

@seisman seisman merged commit 56dfd85 into master Mar 18, 2020
@seisman seisman deleted the gmtapichange branch March 18, 2020 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants