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 mmap support for windows #545

Merged
merged 3 commits into from
Apr 23, 2019
Merged

Fix mmap support for windows #545

merged 3 commits into from
Apr 23, 2019

Conversation

tknopp
Copy link
Contributor

@tknopp tknopp commented Apr 17, 2019

This is a test to fix mmap support on windows.

@tknopp tknopp changed the title Try to Fix Mmap support for windows Fix mmap support for windows Apr 17, 2019
@tknopp
Copy link
Contributor Author

tknopp commented Apr 18, 2019

Test is successful. This is a workaround for the mmap issue on windows. Since the file descriptor that we get from HDF5 is not correct, I instead simply open the file again. To get the permissions of the original file open, I needed to wrap the get_intent method.

This closes #89 and #357.

@tknopp
Copy link
Contributor Author

tknopp commented Apr 18, 2019

appveyor on x86 has other issues. The other ones are green.

Ping @musm for review.

@tknopp
Copy link
Contributor Author

tknopp commented Apr 19, 2019

@musm @timholy any objections to merging and making a release? I am requiring this in my package https://github.com/MagneticParticleImaging/MPIFiles.jl which is supposed to also work on windows.

@musm
Copy link
Member

musm commented Apr 19, 2019

I plan to take a closer look this tomorrow.

@musm
Copy link
Member

musm commented Apr 23, 2019

I don't really understand why
h5f_get_vfd_handle(obj.file.id, prop, ret)
gives the wrong result, do you?

@musm
Copy link
Member

musm commented Apr 23, 2019

In any case, since it wasn't working before I plan to merge this soon, since something that works even though not ideal, is better than it not working at all.

@tknopp It would be good to leave some breadcrumbs for future reference, if someone wants to delve more deeply into a proper fix.

@tknopp
Copy link
Contributor Author

tknopp commented Apr 23, 2019

I don't really understand why
h5f_get_vfd_handle(obj.file.id, prop, ret)
gives the wrong result, do you?

No, if I would know that, I would have fixed it ;-)

My guess is that the file handle is not compatible with what Julia requires in the fdio function.

But I actually don't have access to Windows, so I am not really the best to work on the original issue.

I improved the documentation and this then should be good to go from my side. I would appreciate, if this could be integrated into the upcoming release.

@musm musm merged commit c140bd8 into master Apr 23, 2019
@musm
Copy link
Member

musm commented Apr 23, 2019

Fantastic thank you very much @tknopp for working on this !

@musm
Copy link
Member

musm commented Apr 23, 2019

I plan a new release very soon with this.

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.

2 participants