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

Fix issues for running on Windows #313

Closed
wants to merge 21 commits into from
Closed

Fix issues for running on Windows #313

wants to merge 21 commits into from

Conversation

leouieda
Copy link
Member

Description of proposed changes

Trying to fix the issues for running on Windows now that we have CI.

Fixes #46

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.

for libname in libnames:
try:
libgmt = ctypes.CDLL(os.path.join(libpath, libname))
check_libgmt(libgmt)
Copy link
Member

Choose a reason for hiding this comment

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

If both ctypes.CDLL and check_libgmt don't raise any errors, it means we find the library, then we should break out of the loop. Right?

Suggested change
check_libgmt(libgmt)
check_libgmt(libgmt)
break

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course! Thanks, Dongdong. I stopped coding yesterday because I couldn't even see this basic error anymore :)

Copy link
Member

@weiji14 weiji14 Nov 17, 2019

Choose a reason for hiding this comment

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

I just realized that this change is part of what's been causing our Windows builds to fail... Since we have a list of dll files to check, if one of the dll fails, the variable error turns True and stays True even if a later dll works. Our Linux and MacOS tests work because there's only one item in the list.

@leouieda
Copy link
Member Author

I managed to track down the failures to GMT API session creation causing a crash. It might be something that Windows is a bit more sensitive to. Need to investigate.

@seisman
Copy link
Member

seisman commented Jun 21, 2019

Just provide more information:

I tried pygmt with the latest master branch of GMT. It works well. Running pygmt.test() results in some failed tests, same as what I saw several months ago in #285.

I also tried GMT 6.0.0rc1 provided by conda-forge. GMT itself works well on Windows, but import pygmt crashes.

@leouieda
Copy link
Member Author

Yeah, I'm still scratching my head over this one. I wondered if it's a compiler flag on the conda recipe that is messing things up.

@leouieda
Copy link
Member Author

I just noticed that the conda-forge build log has Build GMT for developers : yes in the cmake log. @seisman do you know if this means we're making debug build by mistake?

@seisman
Copy link
Member

seisman commented Jun 27, 2019

No. It only means installing the header files (*.h) for developers.

@leouieda
Copy link
Member Author

Yeah I figured we wouldn't be this lucky. Thanks

@leouieda
Copy link
Member Author

leouieda commented Jul 3, 2019

Oh good. Now with rc2 the Linux build is failing.

@weiji14
Copy link
Member

weiji14 commented Oct 1, 2019

It's crashing on this line:

session = c_create_session(name.encode(), padding, session_type, print_func)

@seisman
Copy link
Member

seisman commented Oct 1, 2019

Yes, but as I said in previous comments, pygmt works with self-built GMT on Windows, but fails with the GMT provided by conda-forge. So it's really difficult to track down the crash.

@weiji14 weiji14 added this to the 0.1.0 milestone Oct 4, 2019
@leouieda
Copy link
Member Author

@weiji14 😢 yeah, this has bugged me quite a bit. I don't think it's a ctypes issue. I suspect it might be something to do with our conda-forge build. I'm going to work hard on this and some other major issues this week. Hopefully we can get the major blocking points for 0.1 done before AGU 🤞

@vercel vercel bot temporarily deployed to staging November 28, 2019 17:27 Inactive
It doesn't seem to like loading from a clone. It parses the templates
before actually running anything.
@weiji14 weiji14 linked an issue Mar 24, 2020 that may be closed by this pull request
@michaelgrund
Copy link
Member

michaelgrund commented Apr 13, 2020

Hey guys,
after some time, I finally got pygmt properly working on my Windows 10 notebook using Anaconda. However, for this purpose I made use of the Linux subsystem available from the latest Win10 versions. I followed the instructions given here https://gist.github.com/kauffmanes/5e74916617f9993bc3479f401dfec7da to install Anaconda first. Afterwards it was quite easy to install pygmt as outlined in the installing section using the bash available from the Linux subsystem. Putting GMT_LIBRARY_PATH=$HOME/anaconda3/envs/pygmt/lib into .bashrc also works quite well without any errors and warnings. Finally, I successfully tested some of my pygmt jupyter notebooks developed on my "real" Linux machine. I know this is no perfect solution, but probably it may serve as workaround for some of the windows users.

@leouieda
Copy link
Member Author

Hi @michaelgrund thanks for sharing! Yeah, we might just have to stick with that for now. It's not the ideal solution since it wouldn't integrate with a user's existing setup.

@seisman
Copy link
Member

seisman commented May 20, 2020

The issue was fixed in #434.

@seisman seisman modified the milestones: 0.2.x, 0.1.x May 22, 2020
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.

pygmt does not run due to error loading library Support for Windows
4 participants