ExternalLibraries/HDF5 does not set HDF5_INC_DIRS

Issue #1910 closed
Barry Wardell created an issue

When I set HDF5_DIR in my optionlist, the HDF5 external libraries thorn fails to set HDF5_INC_DIRS. This causes the CarpetIOHDF5 utils to fail to build as they cannot find hdf5.h. This should probably be fixed by setting HDF5_INC_DIRS in detect.sh.

This issue is related to #1170.

Keyword:

Comments (21)

  1. Erik Schnetter
    • removed comment

    This description is unclear.

    HDF5_INC_DIRS is a configuration variable to be set by the user, and is examined by the thorn to find out where an existing HDF5 library is installed. What do you mean by "setting" it in the thorn?

    Also, the build.sh script cannot set anything; only the detection script talks to Cactus.

  2. Barry Wardell reporter
    • removed comment

    The actual problem I'm encountering is that HDF5_INC_DIRS is used when compiling the CarpetIOHDF5 utilities, and without it the compilation fails due to being unable to find hdf5.h.

    Are you saying that HDF5_INC_DIRS should only be explicitly set by the user in the optionlist? It is set in detect.sh (sorry, I previously mis-referred to build.sh) if HDF5 is built, but not if it is found by HDF5_DIR having been set in the optionlist.

  3. Erik Schnetter
    • removed comment

    I think it should be set by detect.sh if the user didn't set it. And I see it is actually set there.

    Can you look into the autogenerated HDF5 configuration files in the "bindings" directories? Are the settings there correct? What does "bindings/Configurations/Thorns/CarpetIOHDF5" pull in?

  4. Barry Wardell reporter
    • removed comment

    There is the line HDF5_INC_DIRS="${HDF5_DIR}/include ${HDF5_DIR}/lib" in detect.sh, but that is only used when HDF5 is built, not when it is provided by the system.

    I don't think the problem is with CarpetIOHDF5 itself and have checked that it pulls in bindings/Configuration/Capabilities/make.HDF5.deps. The problem is that make.HDF5.defn has only HDF5_INC_DIRS = /usr/local/lib, i.e. it does not include /usr/local/include.

  5. Roland Haas
    • removed comment

    This is a bit tricky: the utilities (even though they live in thorns' src directories) are not considered part of any thorn so they don't get include paths set based on their "parent" thorn's REQUIRE lines. That is why they themselves have to pull in the required INC_DIRS and LIB_DIRS variables.

  6. Erik Schnetter
    • removed comment

    The settings in "detect.sh" are used unconditionally, independent of whether "build.sh" builds the library or not.

    Line 131 of "build.sh" reads:

        HDF5_INC_DIRS="${HDF5_DIR}/include ${HDF5_DIR}/lib"
    

    which is executed when the library is built, and sets "HDF5_INC_DIRS".

    I just see an error in line 118 there -- the HDF5 library version number is wrong. This could lead to this error. Will fix.

  7. Barry Wardell reporter
    • removed comment

    Assuming you mean line 131 of "detect.sh", this is the line I was referring to. This is inside a conditional

    if [ -n "$HDF5_BUILD" -o -z "${HDF5_DIR}" ]; then
    

    so it is not run in the case where HDF5_DIR is set.

  8. Erik Schnetter
    • removed comment

    Yes, the logic that sets HDF5_INC_DIRS if it is unset and if a system library is used is missing. The same logic is present in many (I hope all?) other external libraries. I don't know when it got lost; I assume that one of the hacks to improve the user experience broke things.

    Clearly, the configuration script has been edited in ways that have more do to with copy-and-paste than understanding shell scripting. For example, internal variables are set via the

    : ${var:=value}
    

    syntax that only makes sense if `var}} might be set before; the traditional distinction between upper case (user input) and lower case (internal) variables has not been kept; user input variables are modified, saved, and restored in the good old ways of BASIC where all variables are global, etc. The whole coding style reeks of "I don't understand what happens, so I'll voodoo some changes in front and in the back of some thing that I treat as black box. Hey -- it doesn't break the tests, so it must be correct".

    I've put a lot of effort into clean external libraries over the past years, but I'm giving up now. Apparently too many people think they can dabble, and break things left and right. My fault for not setting up a barrage of virtual machines against which each commit must be tested.

    These are the lines that are missing:

    # Set options
    if [ "${FFTW3_DIR}" != 'NO_BUILD' ]; then
        : ${FFTW3_INC_DIRS="${FFTW3_DIR}/include"}
        : ${FFTW3_LIB_DIRS="${FFTW3_DIR}/lib"}
    fi
    

    (taken from FFTW3; obviously the library name need to be changed.)

    It is left as exercise for the reader to find out where these lines should go.

    For the release, I suggest setting HDF5_INC_DIRS manually, together with HDF5_DIR, as well as HDF5_LIB_DIRS and HDF5_LIBS.

  9. Frank Löffler
    • removed comment

    Replying to [comment:10 eschnett]:

    These are the lines that are missing: `

    Set options

    if [ "${FFTW3_DIR}" != 'NO_BUILD' ]; then : ${FFTW3_INC_DIRS="${FFTW3_DIR}/include"} : ${FFTW3_LIB_DIRS="${FFTW3_DIR}/lib"} fi `

    I would also suggest to add (assuming testing shows that's fine):

    FFTW3_INC_DIRS="$(${CCTK_HOME}/lib/sbin/strip-incdirs.sh ${FFTW3_INC_DIRS})"
    FFTW3_LIB_DIRS="$(${CCTK_HOME}/lib/sbin/strip-libdirs.sh ${FFTW3_LIB_DIRS})"
    

    ... with the same obvious replacements.

    For the release, I suggest setting HDF5_INC_DIRS manually, together with HDF5_DIR, as well as HDF5_LIB_DIRS and HDF5_LIBS.

    So, you suggest not to add those lines for the release? Testing this automatically isn't trivial, as the testsuites wouldn't test for the utilities to be build, nor (I think) does Jenkins. As far as I read from the ticket, the actual configuration dos work, it is "only" the utilities that fail to build in that case.

    Let's add this problem to the release notes, fix it in trunk after the release, test it for some time there (about a month, with special emphasis on people really testing it), and back-porting it then.

  10. Barry Wardell reporter
    • removed comment

    Replying to [comment:10 eschnett]:

    These are the lines that are missing: `

    Set options

    if [ "${FFTW3_DIR}" != 'NO_BUILD' ]; then : ${FFTW3_INC_DIRS="${FFTW3_DIR}/include"} : ${FFTW3_LIB_DIRS="${FFTW3_DIR}/lib"} fi ` (taken from FFTW3; obviously the library name need to be changed.)

    It is left as exercise for the reader to find out where these lines should go.

    The attached patch implements this suggestion. I have tested that it works and fixes the original problem.

    Replying to [comment:11 knarf]:

    I would also suggest to add (assuming testing shows that's fine): FFTW3_INC_DIRS="$(${CCTK_HOME}/lib/sbin/strip-incdirs.sh ${FFTW3_INC_DIRS})" FFTW3_LIB_DIRS="$(${CCTK_HOME}/lib/sbin/strip-libdirs.sh ${FFTW3_LIB_DIRS})"

    ... with the same obvious replacements.

    Where should these go? I'm guessing right before the script finishes?

    For the release, I suggest setting HDF5_INC_DIRS manually, together with HDF5_DIR, as well as HDF5_LIB_DIRS and HDF5_LIBS.

    So, you suggest not to add those lines for the release?

    This affects all option lists that set HDF5_DIR but not HDF5_INC_DIRS or HDF5_LIB_DIRS. A quick check suggests that most simfactory optionlists fall into this category. Is it better to change all of these optionlists, or to fix the problem with detect.sh?

    Testing this automatically isn't trivial, as the testsuites wouldn't test for the utilities to be build, nor (I think) does Jenkins. As far as I read from the ticket, the actual configuration dos work, it is "only" the utilities that fail to build in that case.

    Yes, it is "only" the utilities that are affected, but this means that the last thing you see when running sim build is a failed compilation. This can be confusing and seems to me like something that should be fixed before the release.

  11. Ian Hinder
    • removed comment

    The test job in Jenkins uses "sim build", which builds the utilities. If this exits nonzero, the build is treated as failed, and the tests are not run. For the tests run on other machines, this will be up to the person running the test. We can ask people running tests to treat utility build failures in the same way as executable build failures.

  12. Barry Wardell reporter
    • removed comment

    Replying to [comment:13 hinder]:

    The test job in Jenkins uses "sim build", which builds the utilities. If this exits nonzero, the build is treated as failed, and the tests are not run. For the tests run on other machines, this will be up to the person running the test. We can ask people running tests to treat utility build failures in the same way as executable build failures.

    The Jenkins build probably doesn't see the issue, since in that case HDF5 is most likely installed in a system path and hdf5.h can be found without explicitly specifying the include directory. This is probably not the case for clusters, where HDF5 is probably installed in a location that isn't searched by default.

  13. Frank Löffler
    • removed comment

    Looking at this, HDF5_INC_DIRS should be set by bash_utils.sh (assuming you set HDF5_DIR, but none of the other variables in your option list).

    Can you please attach details about your setup, in particular: - How is your hdf5 library installed (which INC_DIRS, LIB_DIRS, LIBS would be correct)? - Your option list - The output of detect.sh, when called by Cactus as part of a build

  14. Barry Wardell reporter
    • removed comment

    The problem is not with finding HDF5 on my machine. That works fine by just setting HDF5_DIR as I do. The problem is that nothing correctly sets HDF5_INC_DIRS nor HDF5_LIB_DIRS and these are required when building the CarpetIOHDF5 utils.

    On my machine, hdf5 is installed in /usr/local. I have traced the evaluation through build.sh and bash_utils.sh. The library is found by [https://bitbucket.org/cactuscode/cactus/src/01d8937b1fdd8a31060aeb4e387bc97bf5eeefe2/lib/make/bash_utils.sh?at=master&fileviewer=file-view-default#bash_utils.sh-192 line 192 of bash_utils.sh] using find_standardlib. This correctly calls set_make_vars, but then [https://bitbucket.org/cactuscode/cactus/src/01d8937b1fdd8a31060aeb4e387bc97bf5eeefe2/lib/make/bash_utils.sh?at=master&fileviewer=file-view-default#bash_utils.sh-63 strip-incdirs.sh and strip-libdirs.sh] commands remove the /usr/local/include and /usr/local/lib that are required (note that these are not part of the standard paths in the OS X build system).

    In summary, I would suggest either committing the patch I posted, or else modifying the OS X option lists so that HDF5_INC_DIRS and HDF5_LIB_DIRS are set explicitly.

  15. Frank Löffler
    • removed comment

    Replying to [comment:16 barry.wardell]:

    On my machine, hdf5 is installed in /usr/local. I have traced the evaluation through build.sh and bash_utils.sh. The library is found by [https://bitbucket.org/cactuscode/cactus/src/01d8937b1fdd8a31060aeb4e387bc97bf5eeefe2/lib/make/bash_utils.sh?at=master&fileviewer=file-view-default#bash_utils.sh-192 line 192 of bash_utils.sh] using find_standardlib. This correctly calls set_make_vars, but then [https://bitbucket.org/cactuscode/cactus/src/01d8937b1fdd8a31060aeb4e387bc97bf5eeefe2/lib/make/bash_utils.sh?at=master&fileviewer=file-view-default#bash_utils.sh-63 strip-incdirs.sh and strip-libdirs.sh] commands remove the /usr/local/include and /usr/local/lib that are required (note that these are not part of the standard paths in the OS X build system).

    Ah, this then would be the problem. Cactus assumes paths like /usr/local/... to be correctly set up and usable without specific paths and thus removes those (has to). I cannot think of an easy and generic check whether this is the case. A check whether the environment is OSX would be far easier, but an actual OSX developer would need to implement and test that.

  16. Barry Wardell reporter
    • removed comment

    Replying to [comment:17 knarf]:

    Cactus assumes paths like /usr/local/... to be correctly set up and usable without specific paths and thus removes those (has to). I cannot think of an easy and generic check whether this is the case.

    In that case I suggest we go with the least-invasive solution to the original problem, which is to modify the OS X optionlist and specifically set _INC_DIRS and _LIB_DIRS. I will commit a patch implementing this today.

  17. Log in to comment