WIP: Enable Windows build

seisman commented 4 years ago

Although pygmt doesn't work with the gmt conda package on Windows, it works well with the GMT official installers. See https://github.com/GenericMappingTools/pygmt/issues/431 for details.

Thus, we can still provide pygmt package on Windows, but

should not install gmt provided by conda-forge
can't test

@conda-forge-admin, please rerender

Checklist

[x] Used a fork of the feedstock to propose changes
[x] Bumped the build number (if the version is unchanged)
[ ] Reset the build number to 0 (if the version changed)
[x] Re-rendered with the latest conda-smithy (Use the phrase code>@<space/conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
[x] Ensured the license file is being packaged.

conda-forge-linter commented 4 years ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

moorepants commented 4 years ago

I'm not sure doing this is the best idea. It also might not follow conda forge's protocol.

The main issue I see with doing this, is that Windows users will install this conda package and it will not work if they don't have GMT installed by some other method and it is available to the pygmt conda package. Secondly, if GMT is built against dependencies that are incompatible with the equivalent conda forge packages, the user may get errors associated with that. Lastly, there is no way to communicate the special install instructions via conda forge.

I'm -1 to adding this unless @conda-forge/core says that this is an appropriate practice on conda recipes.

isuruf commented 4 years ago

Agree with @moorepants. What's the issue with gmt from conda-forge?

moorepants commented 4 years ago

There is this issue: https://github.com/conda-forge/gmt-feedstock/issues/90

isuruf commented 4 years ago

I see lots of mentions of crashes and fails and not how exactly. If there is more information, we could help, but without more details, there's nothing we can do.

moorepants commented 4 years ago

Thanks @isuruf, the maintainers of pygmt will have to report the specifics of the issues. I am not familiar with the package. My cluster users needed it, so my only skin in the game is getting it running on linux.

seisman commented 4 years ago

We finally find out why PyGMT doesn't work with conda on Windows, see https://github.com/GenericMappingTools/pygmt/pull/434 for details if anyone is interested in it.

PyGMT calls ctypes.CDLL to find the GMT library "gmt.dll". ctypes.CDLL can't find the library file, perhaps because conda doesn't/can't change the DLL search paths on Windows. Thus, we have to set the environmental variable GMT_LIBRARY_PATH to the path of the gmt library, e.g., C:\Miniconda\envs\pygmt\Library\bin. Is there anything conda can do here?

We also find out a potential bug in GMT (still need more time to confirm where the GMT bug is), and the workaround is setting an environmental variable GMT_SHAREDIR to the path of GMT's share directory, e.g., C:\Miniconda\envs\pygmt\Library\share\gmt.

After merging https://github.com/GenericMappingTools/pygmt/pull/434 and releasing a new PyGMT version, we should be able to provide the pygmt conda package on Windows, but we need to set the two variables in the meta.yaml file to let the tests pass. I checked the metadata documentation but didn't find how to do that.

Any help is appreciated.

isuruf commented 4 years ago

See https://docs.conda.io/projects/conda-build/en/latest/resources/activate-scripts.html for docs. See https://github.com/conda-forge/openjdk-feedstock/tree/master/recipe/scripts for an example.

seisman commented 4 years ago

@isuruf Great! Thanks.

weiji14 commented 4 years ago

So like this?

recipe/scripts/activate.bat

@echo off
set "GMT_LIBRARY_PATH=%CONDA_PREFIX%\Library\bin"
set "GMT_SHAREDIR=%CONDA_PREFIX%\Library\share\gmt"

recipe/scripts/deactivate.bat

@echo off
set "GMT_LIBRARY_PATH="
set "GMT_SHAREDIR="

weiji14 commented 4 years ago

Actually, should those *.bat activation scripts be placed here in the pygmt-feedstock or upstream in gmt-feedstock?

moorepants commented 4 years ago

Actually, should those *.bat activation scripts be placed here in the pygmt-feedstock or upstream in gmt-feedstock?

If this fixes things for any dependency of GMT on Windows, then having it in the GMT feedstock sounds helpful. We can also add them here and you can open and issue on the GMT feedstock.

conda-forge-linter commented 4 years ago

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipe) and found some lint.

Here's what I've got...

For recipe:

When defining a source/url please add a sha256, sha1 or md5 checksum (sha256 preferably).

weiji14 commented 4 years ago

Ok, we're fixing these directly on PyGMT now instead of in the feedstock, so that we wouldn't need to set the environment variables.

[x] GMT_SHAREDIR isn't needed after https://github.com/GenericMappingTools/pygmt/pull/437
[x] GMT_LIBRARY_PATH isn't needed after https://github.com/GenericMappingTools/pygmt/pull/440

seisman commented 4 years ago

Close and reopen to trigger the build again.

seisman commented 4 years ago

@conda-forge-admin, please rerender

seisman commented 4 years ago

The master branch of pygmt now works well on all platforms. Closing this PR. We'll release pygmt v0.1.1 in the next few days.

conda-forge / pygmt-feedstock

WIP: Enable Windows build #2