Extend TriBITS TPL system to better accommodate Find<TPLNAME>.cmake modules

[bartlettroscoe]

The current approach for utilizing Find.cmake modules (such as are shipped with CMake) is to call FIND_PACKAGE(<TPLNAME>), then set the variables TPL_<TPLNAME>_INCLUDE_DIRS and TPL_<TPLNAME>_LIBRARIES, then call TRIBITS_TPL_FIND_INCLUDE_DIRS_AND_LIBRARIES(). The problem with this approach is that it bypasses the variables <TPLNAME>_INCLUDE_DIRS and <TPL>_LIBRARY_NAMES.

To demonstrate the problem, in pull-request #59, FIND_PACKAGE(HDF5) only finds the hdf5 library and not the other libraries, for example, needed by CASL VERA which are specified with:

SET(HDF5_DIR "${TPL_INSTALL_DIR}/hdf5-1.8.10")
SET(HDF5_LIB_DIR "${HDF5_DIR}/lib")
SET(HDF5_LIBRARY_NAMES "hdf5_hl;hdf5_cpp;hdf5_fortran;hdf5;z" CACHE STRING "")
SET(HDF5_INCLUDE_DIRS
  "${HDF5_DIR}/include;${TPL_COMMON_DIR}/zlib-1.2.7/include"
  CACHE FILEPATH "" )
SET(HDF5_LIBRARY_DIRS
  "${HDF5_LIB_DIR};${TPL_INSTALL_DIR}/zlib-1.2.7/lib"
  CACHE FILEPATH "" )

Using FIND_PACKAGE(HDF5) bypasses the variables HDF5_LIBRARY_NAMES and HDF5_LIBRARY_DIRS which are used to point to the desired libs, which include the extra libraries.

To demonstrate this, I checked out the branch findpackage-hdf5 for TriBITS for VERA and configured with:

$ env HDF5_ROOT=/projects/vera/gcc-4.8.3/tpls/opt/hdf5-1.8.10 \
  ./do-configure \
  -DVERA_EXTRA_REPOSITORIES=Trilinos,TeuchosWrappersExt,VERAInExt \
  -DVERA_ENABLE_VERAIn=ON 

This showed in the configure output:

Processing enabled TPL: HDF5 (enabled explicitly, disable with -DTPL_ENABLE_HDF5=OFF)
-- Found HDF5: debug;/projects/vera/gcc-4.8.3/tpls/opt/hdf5-1.8.10/lib/libhdf5.so;debug;/usr/lib64/libz.so;debug;/usr/lib64/librt.so;debug;/usr/lib64/libm.so;optimized;/projects/vera/gcc-4.8.3/tpls/opt/hdf5-1.8.10/lib/libhdf5.so;optimized;/usr/lib64/libz.so;optimized;/usr/lib64/librt.so;optimized;/usr/lib64/libm.so

See, the problem is that the other required libraries hdf5_hl, hdf5_cpp and hdf5_fortran are not listed.

When I tried to build this software, I got a bunch of link failures (as you would expect).

Now yes, you could manually set TPL_<TPLNAME>_INCLUDE_DIRS and TPL_<TPLNAME>_LIBRARIES but that is very verbose and it would break a lot of configure scripts (most importantly, CASL VERA).

What I would like to propose in this issue ticket is that we upgrade TRIBITS_TPL_FIND_INCLUDE_DIRS_AND_LIBRARIES() to allow it to take in defaults for the libraries and include directories (outputs from FIND_PACKAGE(<TPLNAME>)) but if the user sets the cache variables <TPLNAME>_INCLUDE_DIRS, <TPL>_LIBRARY_NAMES, or <TPL>_LIBRARY_DIRS, then the standard existing search for libraries is used. So, you would write FindTPLHDF5.cmake like:

IF (
  HDF5_INCLUDE_DIRS STREQUAL ""
  AND
  HDF5_LIBRARY_NAMES STREQUAL ""
  AND
  HDF5_LIBRARY_DIRS STREQUAL ""
  AND
  TPL_HDF5_INCLUDE_DIRS STREQUAL ""
  AND
  TPL_HDF5_LIBRARIES STREQUAL ""
  )
  FIND_PACKAGE(HDF5)
  IF(HDF5_FOUND)
    SET(TPL_HDF5_INCLUDE_DIRS ${HDF5_INCLUDE_DIRS} CACHE PATH
      "HDF5 include dirs")
    SET(TPL_HDF5_LIBRARIES ${HDF5_LIBRARIES} CACHE FILEPATH
      "HDF5 libraries")
    SET(TPL_HDF5_LIBRARY_DIRS ${HDF5_LIBRARY_DIRS} CACHE PATH
      "HDF5 library dirs")
ENDIF()

...

TRIBITS_TPL_FIND_INCLUDE_DIRS_AND_LIBRARIES( HDF5
  REQUIRED_HEADERS ${REQUIRED_HEADERS}
  REQUIRED_LIBS_NAMES ${REQUIRED_LIBS_NAMES}
  )

What this does is it maintains the current behavior of the find module when any of the exist variables are set by the user but if none are set, then FIND_PACKAGE(HDF5) will be used instead. To support this mode and allow tweaking, the function TRIBITS_TPL_ALLOW_PACKAGE_PREFIND() which will be used like:

TRIBITS_TPL_ALLOW_PACKAGE_PREFIND(HDF5  HDF5_ALLOW_PREFIND)
IF (HDF5_ALLOW_PREFIND)
  FIND_PACKAGE(HDF5)
  ...
ENDIF()

The function TRIBITS_TPL_ALLOW_PACKAGE_PREFIND() should take into consideration:

• If TPL_<TPLNAME>_INCLUDE_DIRS or TPL_<TPLNAME>_LIBRARIES is set, then return FALSE.
• If <TPLNAME>_INCLUDE_DIRS, <TPL>_LIBRARY_NAMES, or <TPL>_LIBRARY_DIRS is set and <TPLNAME>_FORCE_PACKAGE_PREFIND=FALSE, then return FALSE.
• If <TPLNAME>_INCLUDE_DIRS, <TPL>_LIBRARY_NAMES, or <TPL>_LIBRARY_DIRS is set but <TPLNAME>_FORCE_PACKAGE_PREFIND=TRUE, then return TRUE.

Idea behind <TPLNAME>_FORCE_PACKAGE_PREFIND is to allow the user, or the FindTPL<TPNAME>.cmake module itself, to always call FIND_PACKAGE(<TPLNAME> ...) unless TPL_<TPLNAME>_INCLUDE_DIRS or TPL_<TPLNAME>_LIBRARIES are set. The reason this is important is that the cache variable names <TPLNAME>_INCLUDE_DIRS and <TPLNAME>_LIBRARY_DIRS collide with the names used by many of the standard Find<TPLNAME>.cmake modules, such as is the case with HDF5 (see above).

Tasks:

1. Implement and unit test TRIBITS_TPL_ALLOW_PACKAGE_PREFIND()
2. Refactor FindTPLHDF5.cmake on the branch findpackage-hdf5 to use TRIBITS_TPL_ALLOW_PACKAGE_PREFIND() as shown above (and get it to find hdf5_fortran when Fortran is enabled to keep backward compatibility).
3. Test the updated FindTPLHDF5.cmake file on Trilinos and VERA to ensure correct behavior.
4. Updated documentation in the TriBITS Developers Guide and related TriBITS functions.
5. Merge updated findpackage-hdf5 into TriBITS github/master branch, snapshot to Trilinos, etc.

[bartlettroscoe]

This as now been pushed to the master branch for the TriBITS repos on github and casl-dev and I snapshotted this to Trilinos in the commit:

commit 842e7fb9126a1ddd1c26e28ccdd3c93c4747be7b
Author: Roscoe A. Bartlett <[email protected]>
Date:   Wed May 6 17:27:35 2015 -0400

    Automatic snapshot commit from tribits at d708fbe

    Origin repo remote tracking branch: 'origin/master'
    Origin repo remote repo URL: 'origin = [email protected]:TriBITSPub/TriBITS'

    At commit:

    f1df791 Refactor to better use FIND_PACKAGE(HDF5) (TriBITS #59 and #67)
    Author: Roscoe A. Bartlett <[email protected]>
    Date: Wed May 6 09:04:31 2015 -0400

    Build/Test Cases Summary
    Enabled Packages:
    Disabled Packages: STK,MueLu,Zoltan2,Tpetra
    Enabled all Packages
    0) MPI_DEBUG => passed: passed=1374,notpassed=0 (37.62 min)
    1) SERIAL_RELEASE => passed: passed=1376,notpassed=0 (27.15 min)

Documentation on how to use FIND_PACKAGE() in a TriBITS TPL is shown online here. I am not super happy with that documentation but at least it is a start. I am sure we will find more issues as we extend other FindTPL<TPL_NAME>.cmake modules to call FIND_PACKAGE() while not breaking existing configure scripts.

[bartlettroscoe]

I am closing this story. Further improvements will be done offline.

[nschloe]

I tested this and it works, but I have to say that I dislike the magic that is behind
https://github.com/TriBITSPub/TriBITS/blob/master/tribits/common_tpls/FindTPLHDF5.cmake#L104. FindTPLHDF5.cmake would be much clearer if there was an ELSE() clause around everything that's not IF (HDF5_ALLOW_PREFIND), but I'm sure that there's is some reason for not doing this, cf. "[...] and set some other standard varaibles" (sic).

Anyhow, can we apply the same logic to LAPACK, BLAS, BOOST as well?

[bartlettroscoe]

Nico,

It is important to always call TRIBITS_TPL_FIND_INCLUDE_DIRS_AND_LIBRARIES() at the end. This is an important hook to make sure things are being done correctly. For example, this is where we will put an assert that these libraries and include dirs actually exit (see this todo).

As for BLAS, LAPACK, and BOOST, yes, of course we can use the same approach. Feel free to follow the example in FindTPLHDF5.cmake and update these as well. As with HDF5, we need to make sure that existing overrides still hold. All we are doing is changing the default search. That might technically break backward compatibility as well but if the Find<TPLNAME>.cmake can't do a better job at finding BLAS, LAPACK, and Boost on a machine that a static list of library names, then they are not much good.

[nschloe]

I don't really understand much of what comes after the ENDIF() in the HDF5 finding code. I would have expected a structure like

IF (HDF5_ALLOW_PREFIND)
  # the new way of finding
ELSE()
  # the old way of finding
ENDIF()

You say that that TRIBITS_TPL_FIND_INCLUDE_DIRS_AND_LIBRARIES() has to be called no matter what. Alright, so let's put that at the end. On top of that, though, a bunch of other things are happening, e.g.,

SET(REQUIRED_HEADERS hdf5.h)
SET(REQUIRED_LIBS_NAMES hdf5)

With a structure like

IF (HDF5_ALLOW_PREFIND)
  # the new way of finding
ENDIF()
# the old way of finding

it looks like # the new way of finding is overridden. This obviously doesn't happen as I can see from trying it out, but I don't know how. I suppose behind the scenes you are testing if some variables exist and do something only in case they don't?

I will move around the BLAS and LAPACK files just like that, but really I don't know what I'm doing.

[bartlettroscoe]

it looks like # the new way of finding is overridden. This obviously doesn't
happen as I can see from trying it out, but I don't know how. I suppose >
behind the scenes you are testing if some variables exist and do something
only in case they don't?

Yes, it is all based on variables and the behavior is unit tested to make sure it is correct. We just need to document this process a little better but what is described now here should be sufficient to help people use this.

I know this is not ideal but this setup is the best I could do given the constraints (i.e. want a better default find but need to maintain the existing overrides to maintain backward compatibility and need to continue to support better uniform control for overrides) and the fact that CMake is not the most flexible language in existence.

I will move around the BLAS and LAPACK files just like that, but really I
don't know what I'm doing.

I will take a look at the pull-request in a bit here.

[nschloe]

Could everything from ENDIF() up to TRIBITS_TPL_FIND_INCLUDE_DIRS_AND_LIBRARIES() go into an ELSE()-clause? If yes, I'd vote for that.

[bartlettroscoe]

It could be but it would make the code more complex IMHO, or at least more verbose and longer since you have to define the vars somehow if you call TRIBITS_TPL_FIND_INCLUDE_DIRS_AND_LIBRARIES() only once.

[nschloe]

The reason why I'd prefer

IF (HDF5_ALLOW_PREFIND)
  # the CMake way of finding
ELSE()
  # the TriBits way of finding
ENDIF()
TRIBITS_TPL_FIND_INCLUDE_DIRS_AND_LIBRARIES(...)

over

IF (HDF5_ALLOW_PREFIND)
  # the CMake way of finding
ENDIF()
# the TriBits way of finding
TRIBITS_TPL_FIND_INCLUDE_DIRS_AND_LIBRARIES(...)

is that, as a developer, the former gives you the guarantee that you don't need to fiddle with # the TriBits way of finding when dealing with # the CMake way of finding.

(Also, I think I didn't understand what you mean by

if you call TRIBITS_TPL_FIND_INCLUDE_DIRS_AND_LIBRARIES() only once.

-- It's called once in any case, right?)

[bartlettroscoe]

@nschloe, take a look at updated FindTPLHDF5.cmake file in #70 and see if that is more clear to you. I have a todo to improve the documentation for uniform handing of TriBITS TPLs but there are some higher priority items to address, namely #63 (BTW, let me now if you have any comments on that. I don't see any comments in the Google Doc yet.)