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

Add explicit testing for hdf5dll.dll on Windows to improve diagnostics #146

Closed
wants to merge 3 commits into from

Conversation

mwiebe
Copy link
Contributor

@mwiebe mwiebe commented Apr 18, 2012

Ran into this because of a bug in the HDF5 windows installer.

@avalentino
Copy link
Member

Sorry @mwiebe, I'm not an expert of ctypes, but it is not enough just using something like

if not ctypes.util.find_libraty('hdf5dll.dll'):
    raise ImportError('Could not load "hdf5dll.dll", please ensure'
                              ' that it can be found in the system path')

or

try:
    l = ctypes.CDLL('hdf5dll.dll')
except OSError:
    raise ImportError('Could not load "hdf5dll.dll", please ensure'
                              ' that it can be found in the system path')
else:
    del l

?

@mwiebe
Copy link
Contributor Author

mwiebe commented May 1, 2012

Yeah, those probably would work too, and be simpler. I'm not really an expert on ctypes either.

@scopatz
Copy link
Member

scopatz commented May 6, 2012

Either way, I think that we should merge this fix in. @avalentino's is cleaner and would test it if I had a windows machine....

@ghost ghost assigned avalentino May 7, 2012
@avalentino
Copy link
Member

@mwiebe can you please confirm if at least one of the simplified version of the patch works for you?

@mwiebe
Copy link
Contributor Author

mwiebe commented May 8, 2012

I've switched it to use the find_library function, which works on my machine. I also modified setup.py to automatically find the HDF5 directory if it's in the PATH, because I don't like having to set that manually.

@scopatz
Copy link
Member

scopatz commented May 8, 2012

@mwiebe Much agreed. This looks good to me now. @avalentino Do you want to merge it in or should I?

@avalentino
Copy link
Member

Done. It is now in the develop branch.
Thanks again @mwiebe

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