tbeu / matio

MATLAB MAT File I/O Library
https://matio.sourceforge.io
BSD 2-Clause "Simplified" License
330 stars 97 forks source link

Consider target architecture specific install path #222

Closed tbeu closed 4 months ago

tbeu commented 7 months ago

I have the official HDF5 binaries for both Win32 and x64 installed on Windows default paths %PROGRAMFILES(X86)%/HDF_Group and %PROGRAMW6432%/HDF_Group, respectively. It happens, that with latest CMake 3.27.8 the target architecture is not considered for find_package, thus there only is a 50% chance to find the HDF5 package matching the target architecture Win32 or x64. This PR is my attempt (or workaround) to find the HDF5 package from the right program files folder after hours of trying to get it running using HINTS or PATHS of find_package.

@MaartenBent Let me know if you agree to set HDF5_ROOT as proposed. Thanks.

MaartenBent commented 7 months ago

I'm not sure if it is matio's job to provide search locations. Normally it's up to the user to make sure dependencies are available and findable. And in your proposal you are also overriding a user provided HDF5_ROOT.

When you run CMake from the command line, you can add -DCMAKE_PREFIX_PATH="<paths>" as argument and give the path(s) where CMake should look for packages. Then the matio code doesn't have to deal with it. In CMake GUI you can use a CMAKE_PREFIX_PATH environment variable with the path(s). Or have it in your PATH.

The correct path would in this case be <ProgramFiles>/HDF_Group/HDF5/1.14.3. Besides CMAKE_PREFIX_PATH you can also use HDF5_ROOT.

If you do want to continue with it, I propose something simpler. This should work, I use something similar:

if(MSVC)
    if(CMAKE_CL_64)
        list(APPEND CMAKE_PREFIX_PATH "$ENV{ProgramFiles}/HDF_Group/HDF5/1.14.3")
    else()
        list(APPEND CMAKE_PREFIX_PATH "$ENV{ProgramFiles\(x86\)}/HDF_Group/HDF5/1.14.3")
    endif()
endif()

Only the hardcoded version is not nice.

tbeu commented 7 months ago

Thanks for the reply. I use cmake-gui where it is rather annyoing that the wrong target architecture is found. I tried to avoid the hard-coded version number by using find GLOB (which worked in my case).

tbeu commented 4 months ago

@chris-se I also would appreciate your feedback on this PR. Thanks.

MaartenBent commented 4 months ago

I think HDF5 only provides installers for the msvc libraries? Then you might want to limit it to MSVC instead of WIN32. Though I also see clang and intel compiler installers, all for visual studio.

How does this work when there are multiple version installed, does glob promises any ordering or is the result ordered by name/date/random? Maybe you can use list(sort ...) so it tries the latest hdf5 version first.

If that doesn't work, or just as an alternative to globbing, you could also create a list of known subdirectories. But that requires updating if they ever release 1.8.31 or 3.21.0. Something like:

foreach(major RANGE 2 0 -1) 
    foreach(minor RANGE 20 0 -1)
        foreach(patch RANGE 30 0 -1)
            list(APPEND hdf_sub_dirs "HDF/${major}.${minor}.${patch}/share/cmake")
            list(APPEND hdf_sub_dirs "HDF/${major}.${minor}.${patch}/cmake")
        endforeach()
    endforeach()
endforeach()

You still override any HDF_ROOT that the user might set, by inserting the guesses at the beginning of HDF_ROOT. I think it is better to not set HDF5_ROOT, but use the PATHS option. It seems specifically designed for this: 9. Search paths specified by the PATHS option. These are typically hard-coded guesses.

find_package(HDF5 PATHS ${MATIO_TRY_HDF5_PATH})
tbeu commented 4 months ago

I tried again, but giving up after https://github.com/HDFGroup/hdf5/issues/3860.