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

WIP: Enable Windows build #2

Closed
wants to merge 7 commits into from
Closed

WIP: Enable Windows build #2

wants to merge 7 commits into from

Conversation

seisman
Copy link
Contributor

@seisman seisman commented May 17, 2020

Although pygmt doesn't work with the gmt conda package on Windows, it works well with the GMT official installers. See GenericMappingTools/pygmt#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

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

Although pygmt doesn't work with the gmt conda package on Windows, it
works well with the GMT official installers.
See GenericMappingTools/pygmt#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-linter
Copy link

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
Copy link

moorepants commented May 17, 2020

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
Copy link
Member

isuruf commented May 17, 2020

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

@moorepants
Copy link

There is this issue: conda-forge/gmt-feedstock#90

@isuruf
Copy link
Member

isuruf commented May 17, 2020

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
Copy link

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
Copy link
Contributor Author

seisman commented May 19, 2020

We finally find out why PyGMT doesn't work with conda on Windows, see GenericMappingTools/pygmt#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 GenericMappingTools/pygmt#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.

@seisman
Copy link
Contributor Author

seisman commented May 19, 2020

@isuruf Great! Thanks.

@weiji14
Copy link
Member

weiji14 commented May 19, 2020

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
Copy link
Member

weiji14 commented May 19, 2020

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

@moorepants
Copy link

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.

@seisman seisman changed the title Enable Windows build WIP: Enable Windows build May 20, 2020
@conda-forge-linter
Copy link

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

@seisman seisman closed this May 20, 2020
@seisman seisman reopened this May 20, 2020
@seisman seisman closed this May 20, 2020
@seisman seisman reopened this May 20, 2020
@weiji14
Copy link
Member

weiji14 commented May 20, 2020

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.

@seisman
Copy link
Contributor Author

seisman commented May 21, 2020

Close and reopen to trigger the build again.

@seisman seisman closed this May 21, 2020
@seisman seisman reopened this May 21, 2020
@seisman
Copy link
Contributor Author

seisman commented May 21, 2020

@conda-forge-admin, please rerender

@seisman
Copy link
Contributor Author

seisman commented May 21, 2020

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.

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.

5 participants