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

Longint fixes #625

Merged
merged 4 commits into from Oct 5, 2015
Merged

Longint fixes #625

merged 4 commits into from Oct 5, 2015

Conversation

gpwright
Copy link
Contributor

Between version 1.9.178 and 1.9.222 of HDF5 the type hid_t was changed from int to long int. h5py now handles this appropriately by modifying it's own hid_t cython declaration based on the HDF5 version number.

Note: this changeset fixes issue #599 and issue #616

…is was exposed by the typedef for hid_t changing from int to long int.
…ros with a reference counting mechanism. If we don't explicitly copy them here they become invalid.
@gpwright
Copy link
Contributor Author

Btw I'm pushing this to master but can we cherry-pick it onto the 2.5 branch since that's what I'm actually developing on? Should I create a separate pull request or can it be done as part of this one?

@andrewcollette
Copy link
Contributor

I have some lingering concerns about this. Specifically, as I recall we never relied on the exact type involved in the "ctypedef int hid_t" declaration; in fact, this bit from the Cython docs (number 3 in the second list) specifically states that the exact type should not matter:

http://docs.cython.org/src/userguide/external_C_code.html#referencing-c-header-files

In other words, "hid_t" should always be what shows up in the Cython output so the "int" or "long int" should not be visible. Then it will map to whatever int/long int is declared by HDF5.

Any idea of how to address this? One approach might be to run Cython with the "int" definition, and then the "long int" definition, and simply diff the output files. Then we could see where the exact type was "leaking" into the Cython output.

@gpwright
Copy link
Contributor Author

I understand your point, particularly the section in the cython docs that says "will work okay whatever the actual size of a word is (provided the header file defines it correctly)".

Moreover I just ran the diff as you suggested on builds with and without the typedef change, and the diff showed no changes to the builds except for the typedef and some unknown changes to the binary data inside the egg file.

I just verified for my own sanity that by reverting back to typedef int and trying to "import h5py" I am back to the original problem "Value Error: Not a property list class".

I have little or no knowledge of cython but my explanation would be that at runtime, python is instantiating int objects rather than long objects based on the cython type. The python int object is then assigned a long and overflows.

@andrewcollette
Copy link
Contributor

Interesting! So what happens if we simply use "ctypedef long int" on both HDF5 1.8 and 1.10?

@andrewcollette
Copy link
Contributor

(ctypedef long int hid_t, that is)

@gpwright
Copy link
Contributor Author

gpwright commented Oct 1, 2015

I built h5py against HDF5 1.8 with ctypedef long int, and it seems fine. I can import h5py, and run some simple tests of my own, and all the built-in tests are passing.

Version 1.10 I couldn't find online... has this been released yet?

So are you thinking to skip the version check and just always use long?

@andrewcollette
Copy link
Contributor

Sorry, 1.9 is the branch that will become 1.10.

Since the choice of type evidently doesn't affect the Cython-generated C files, I would suggest removing the version check and just using long int everywhere.

@gpwright
Copy link
Contributor Author

gpwright commented Oct 2, 2015

I removed the version check

@andrewcollette
Copy link
Contributor

Looks good. Thanks again for tracking this down!

andrewcollette added a commit that referenced this pull request Oct 5, 2015
@andrewcollette andrewcollette merged commit c29d5be into h5py:master Oct 5, 2015
nevion pushed a commit to nevion/h5py that referenced this pull request Oct 18, 2015
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.

None yet

2 participants