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

Why not use H5_DEFAULT_PLUGINDIR to set the plugin directory? #2428

Closed
opoplawski opened this issue Jun 25, 2022 · 9 comments
Closed

Why not use H5_DEFAULT_PLUGINDIR to set the plugin directory? #2428

opoplawski opened this issue Jun 25, 2022 · 9 comments

Comments

@opoplawski
Copy link
Contributor

hdf5 defines H5_DEFAULT_PLUGINDIR in H5pubconf.h, why not use it to set the plugin path by default?

@edwardhartnett
Copy link
Contributor

Because you are the first to suggest it! ;-)

I did not even know it existed. Are you suggesting we take whatever value of H5_DEFAULT_PLUGINDIR is in h5pubconf.h and install plugins there?

@opoplawski
Copy link
Contributor Author

That would be my suggestion for a default value. That is where hdf5 will look in the absence of HDF5_PLUGIN_PATH.

@DennisHeimbigner
Copy link
Collaborator

I looked at the code. The flag H5_DEFAULT_PLUGINDIR is intended as an HDF5 CMake build option of the form:

-DH5_DEFAULT_PLUGINDIR:PATH=location

It does not to be ever used as an environment variable.

@opoplawski
Copy link
Contributor Author

I never said it was an environment variable.

@sebastic
Copy link
Contributor

The H5_DEFAULT_PLUGINDIR value in /usr/include/hdf5/serial/H5pubconf.h is also exposed through pkg-config:

$ grep H5_DEFAULT_PLUGINDIR /usr/include/hdf5/serial/H5pubconf.h 
#define H5_DEFAULT_PLUGINDIR "/usr/lib/x86_64-linux-gnu/hdf5/serial/plugins"

$ pkg-config --variable=PluginDir hdf5
/usr/lib/x86_64-linux-gnu/hdf5/serial/plugins

Although this is a customization in the hdf5 Debian package.

Extracting the value from the include file is most reliable, e.g.:

$ cat hdf5-plugin-dir.c
#include <stdio.h>
#include "H5pubconf.h"

int main () {
    printf("%s\n", H5_DEFAULT_PLUGINDIR);
}
$ gcc -I /usr/include/hdf5/serial hdf5-plugin-dir.c && ./a.out 
/usr/lib/x86_64-linux-gnu/hdf5/serial/plugins

@DennisHeimbigner
Copy link
Collaborator

I do not understand your point. Even if exported as an environment variable,
it is never used in the code at run-time. It is only used when hdf5 is built.

@sebastic
Copy link
Contributor

As mentioned in #2429:

The default path (/usr/local/hdf5/lib/plugin) is also incorrect when using -DCMAKE_INSTALL_PREFIX=/usr and -DCMAKE_INSTALL_LIBDIR=lib/x86_64-linux-gnu. A better default would be ${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_LIBDIR}/hdf5/plugin.

/usr/local/hdf5/lib/plugin is a bad default value:

IF(ENABLE_PLUGIN_INSTALL AND PLUGIN_INSTALL_DIR STREQUAL "YES")
    # Default to last dir (lowest search priority) in HDF5_PLUGIN_PATH
    IF(DEFINED ENV{HDF5_PLUGIN_PATH})
      SET(PLUGIN_PATH "$ENV{HDF5_PLUGIN_PATH}")
    ELSE()
        IF(ISMSVC OR ISMINGW)
          SET(PLUGIN_PATH "$ENV{ALLUSERSPROFILE}\\hdf5\\lib\\plugin")
        ELSE()
          SET(PLUGIN_PATH "/usr/local/hdf5/lib/plugin")
        ENDIF()
    ENDIF()
ENDIF()
# If user wants, then install selected standard filters
AC_MSG_CHECKING([whether and where we should install plugins])
AC_ARG_WITH([plugin-dir], [AS_HELP_STRING([--with-plugin-dir=<absolute directory>|no|--without-plugin-dir],
              [Install selected standard filters in specified or default directory])],
              [],[with_plugin_dir=no])
AC_MSG_RESULT([$with_plugin_dir])
if test "x$with_plugin_dir" = xno ; then # option missing|disabled
    with_plugin_dir=no
    with_plugin_dir_setting="N.A."
    enable_plugin_dir=no
elif test "x$with_plugin_dir" = xyes ; then # --with-plugin-dir, no argument
    # Default to last dir (lowest search priority) in HDF5_PLUGIN_PATH
    PLUGIN_PATH="$HDF5_PLUGIN_PATH"
    if test "x${PLUGIN_PATH}" = x ; then
        if test "x$ISMSVC" = xyes || test "x$ISMINGW" = xyes; then
            PLUGIN_PATH="${ALLUSERSPROFILE}\\hdfd5\\lib\\plugin"
        else
            PLUGIN_PATH="/usr/local/hdf5/lib/plugin"
        fi
    fi
    # Use the lowest priority dir in the path
    if test "x$ISMSVC" = xyes || test "x$ISMINGW" = xyes; then
      PLUGIN_DIR=`echo "$PLUGIN_PATH" | tr ';' ' '`
    else
      PLUGIN_DIR=`echo "$PLUGIN_PATH" | tr ':' ' '`
    fi
    for pp in ${PLUGIN_DIR} ; do last="$pp"; done
    PLUGIN_DIR="$last"
    with_plugin_dir_setting="$PLUGIN_DIR"
    # canonical form is all forward slashes
    with_plugin_dir=`echo "$PLUGIN_DIR" | tr '\\\\' '/'`
    enable_plugin_dir=yes
    AC_MSG_NOTICE([Defaulting to --with-plugin-dir=$with_plugin_dir])
else # --with-plugin-dir=<dir|path>
    with_plugin_dir_setting="$with_plugin_dir"
    enable_plugin_dir=yes
fi

These parts of the CMake and Autotools build systems should be updated to use the value of H5_DEFAULT_PLUGINDIR and only if that is not set/available fall back to hardcode /usr/local/hdf5/lib/plugin.

Using the value of H5_DEFAULT_PLUGINDIR will make it work for most cases, instead of requiring HDF5_PLUGIN_PATH to be set because the default value is bad.

@DennisHeimbigner
Copy link
Collaborator

I see my confusion. When you said:

That would be my suggestion for a default value. That is where hdf5 will look in the absence of HDF5_PLUGIN_PATH.

I interpreted that to mean that it was another environment variable.
I now see that you you are saying that instead of hard-coding
e.g. /usr/local/hdf5/lib/plugin, we should get the value from the HDF5 build.

I note the following:

  1. It looks like this was added in the 1.10.x version of HDF5.
  2. that path (and the windows equivalent) are hard-wired into the hdf5 documentation.
  3. If netcdf-c is built without HDF5 but with NCZarr, then we do not have access to that HDF5 value, so we should do the same thing for the NCZarr code, but of course it would need to be coordinated with the HDF5 value. Alternatively, we could add an option for this (like HDF5 does). This does not seem an unreasonable idea since it only matters if the builder was already doing it for HDF5.

@edwardhartnett
Copy link
Contributor

We have actually added that default directory.

This issue can be closed.

@WardF WardF closed this as completed Aug 29, 2024
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

No branches or pull requests

5 participants