-
Notifications
You must be signed in to change notification settings - Fork 49
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
New3rd party libraries #3110
New3rd party libraries #3110
Conversation
Hi Scott, do you mind updating the |
@sgpearse can you please identify which libraries were updated and their version numbers? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we will require users to manually set HDF5_PLUGIN_PATH
to enable ZFP?
@ALL - HDF5 seems to be working on Windows now. I'll request re-reviews once I can verify that the build systems are referencing the new libraries. Answers to your questions are below.
@shaomeng - No problem.
@clyne
@StasJ - From what I've read, this is necessary. From the NetCDF documentation:
There may be a way to use HDF5's API for this but I'm not sure what it entails. |
Installers have been verified on all platforms with the new libraries. If you'd like to try the installers, you can find them in the artifacts on these builds. @StasJ I was able to remove the need for HDF5_PLUGIN_PATH on our UNIX systems. I had no luck building SZlib+HDF5+NetCDF together, so I used Unidata's NetCDF binary installer for Windows. This leaves us without the HDF5 library needed to make the programmatic call for finding the plugin path. @StasJ and @clyne - the programatic call for setting the HDF5 plugin path is currently in apps/vaporgui/main.cpp. Should this be moved elsewhere? |
@sgpearse it looks to me like you are calling H5PLreplace() on Mac and Linux platforms, not windows. I'm a little confused because I thought you only need to set the plug path explicitly on Windows systems? Regardless, setting the path in the vaporgui app. is probably not ideal; our conversion utilities won't work. Any thoughts on this @StasJ? I'm not sure if it is a disaster if we can't convert to/from hdf5 to VDC on Windows platforms. We may just have to live with that. |
Here's an idea: in case that we have to distribute VAPOR with different capabilities among 3 major platforms, or even capabilities that are easy to go wrong within the camp of Linux for example, we provide a utility program that detects those variables and report to the user. It could be some added fields to "vaporversion".
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sgpearse it looks to me like you are calling H5PLreplace() on Mac and Linux platforms, not windows. I'm a little confused because I thought you only need to set the plug path explicitly on Windows systems?
Regardless, setting the path in the vaporgui app. is probably not ideal; our conversion utilities won't work. Any thoughts on this @StasJ? I'm not sure if it is a disaster if we can't convert to/from hdf5 to VDC on Windows platforms. We may just have to live with that.
Couldn’t we just set the plugin path in the VDC API when attempting to load an HDF5 dataset? We could keep track of whether it had been set or the HDF5 API allows you to query the existing plugin paths.
That could be done, but to ensure all of our readers and writers that access NetCDF files (e.g. DCMPAS, DCWRF, VDNetCDF, etc.) are supported we would have to do this in multiple places; there is not one single place where NetCDF is initialized, unfortunately. Another option might be to not touch the code, and to instead set the HDF5_PLUGIN_PATH. It looks like HDF5 will honor that. I don't what is involved in setting an env variable for our Windows installer. |
Good idea, @shaomeng. Could you open an issue? |
@sgpearse we need seem clarification. Does the HDF5 plugin path need to be explicitly set on all platforms? |
HDF5_PLUGIN_PATH only needs to be set on Windows. It also needs to be set on our personal development machines because GetResourcePath() only works on installed versions of Vapor. It does not return /usr/local/VAPOR-Deps/2019-Aug during development builds. |
|
@StasJ I see it now, thank you. HDF5_PLUGIN_PATH is now only required on windows dev/installer builds. |
@sgpearse will look into setting the Windows environment variable within the running executable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@StasJ - Your notes should be addressed. All installers are working except for Windows, #3178. osgl seems to be the problem when we build with the gui. The broken windows build is reported #3178.
Thanks, looks great. There was one comment you didn't respond to:
This change requires lib-common to link against HDF5. It may be more appropriate for the VDC library to handle this since it handles the use of HDF5 files.
@StasJ - Ah, I did miss that. I thought lib common made sense because it handles our resource paths, but no biggie. Fixed. |
Fixes #3179. |
Fixes #3158. |
Fixes #3174 |
Ready for @StasJ to review again |
Fixes #2816 |
@sgpearse this issue has been closed. Presumably everyone should update their own development libraries to be in sync with the DevOps platforms, correct? Where are these libraries available? thanks! |
Yes.
Check out the PR description, which has links to bundles for each platform. |
When I click on the link for Mac OSX I get: Sorry, the file you have requested does not exist. |
This PR updates Vapor to use ZFP enabled HDF5 and an updated version of Ospray. Users must set the HDF5_PLUGIN_PATH to Vapor's /lib directory to allow for ZFP enabled compression.
Since not all libraries were rebuilt, the newly built libraries had to be targeted at /usr/local/VAPOR-Deps/2019-Aug again.
The new libraries can be found on google drive. They have been named 2019-Aug-[OS]-2022.tar.xz
Links here:
Ubuntu
CentOS
OSX
Windows
d4810cb: clangTidyOutput.txt
Fixes #3179
Fixes #3158
Fixes #3174
Fixes #2816