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

TPLs: compatibility with CMake-distributed FindXXX.cmake and XXXConfig.cmake files #340

Closed
sethrj opened this issue Nov 14, 2020 · 17 comments

Comments

@sethrj
Copy link
Contributor

sethrj commented Nov 14, 2020

One issue I've come across multiple times is that the tribits TPL-finding scripts lag (in features, cross-platform compatibility, etc.) behind the official ones. Another is the inconsistencies introduced between TriBITS and CMake variables for hints and locations for libraries, e.g. manually having to specify TPL_HDF5_... vs using HDF5_ROOT or CMAKE_PREFIX_PATH to look up the install location.

Related (but maybe should be split): CMake-distributed modules that call add_library, such as UseSWIG, aren't compatible with TriBITS without a lot of hacking. TriBITS has the potential to simplify 90% of cmake code, but it should be designed so that the remaining 10% won't be substantially harder to integrate correctly.

@bartlettroscoe
Copy link
Member

@sethrj, any TriBITS FindTPLXxX.cmake module can use find_package(XXX) internally as described at:

and in fact some of the standard TriBITS FindTPLXXX.cmake modules do just that. See, for example:

But note that to get those to work, we had to provide modified versions of the standard FindXXX.cmake modules you will find under:

Past attempts to use the standard FindXXX.cmake modules have not been very successful. (For example, see the attempt to use find_package(BLAS) and find_package(LAPACK) in #69 and #89.)

Related (but maybe should be split): CMake-distributed modules that call add_library, such as UseSWIG, aren't compatible with TriBITS without a lot of hacking.

Why? Why can't FindTPLSWIG.cmake just be modified to call find_package(SWIG) and then set TPL_SWIG_LIBRARIES to the targets added with add_library() for now? The refactored version of TriBITS will be to have FindTPLXXX.cmake modules create the targets specified here.

@sethrj
Copy link
Contributor Author

sethrj commented Nov 15, 2020

The first issue I recall having was with Geant4 a couple of years ago. Geant4 distributes a cmake config file (which these days are modern cmake targets Geant4::XXX), and I had to do some nasty work to extract the library names and include paths for compatibility with the tribits TPL header/libraries as your documentation states.

HDF5, BLAS, and LAPACK are the problem children indeed: the CMake-distributed ones undergo incremental iteration for the variety of install configurations and platforms, whereas the TriBITS ones don't. This exemplifies one of my key issues with TriBITS: standard ways don't work, and TriBITS ways aren't compatible/up-to-date.

Finally, SWIG isn't a library to be linked to. It's a generator used for creating libraries. The UseSWIG module to which I was referring provides a swig_add_library which under the hood calls CMake add_library. There's no way to force it to call tribits_add_library instead. This is one of the main reasons I moved ForTrilinos away from TriBITS: hacking SWIG to work with TriBITS (or vice versa) is not sustainable.

@sethrj
Copy link
Contributor Author

sethrj commented Nov 15, 2020

Incidentally, it looks like since 2012 SCALE has had a "FindTPLHDF5" of its own that presumably overrides the tribits-supplied one -- in my earlier comment I mistakenly conflated that with the tribits-provided one. This duplication could be the source of the problems I've had with FindHDF5... 😒

At the top of the file are the comments:

# We override the default tribits HDF5 for a few reasons:
# 1. The default TriBITS HDF5 doesn't link against the high-level
#    HDF5 library (hdf5_hl)
# 2. We add logic to enable HDF5 fortran automatically for VERA.
# 3. We add the ability to determine whether HDF5 was built in parallel

@bartlettroscoe
Copy link
Member

Finally, SWIG isn't a library to be linked to. It's a generator used for creating libraries. The UseSWIG module to which I was referring provides a swig_add_library which under the hood calls CMake add_library. There's no way to force it to call tribits_add_library instead. This is one of the main reasons I moved ForTrilinos away from TriBITS: hacking SWIG to work with TriBITS (or vice versa) is not sustainable.

Right, but that that has nothing to do with how TriBITS handles TPLs. This is just an example of how it would be good for TriBITS to better support non-native TriBITS code generating libraries, etc. But you can do that even now within TriBITS as is demonstrated with:

and as we did with MOOSE/Bison in CASL VERA.

@bartlettroscoe
Copy link
Member

Incidentally, it looks like since 2012 SCALE has had a "FindTPLHDF5" of its own that presumably overrides the tribits-supplied one

@sethrj, correct. And TriBITS supports that as described at:

which says:

A TPL defined in a upstream repo can listed again in a downstream repo, which allows redefining the find module that is used to specify the TPL. This allows downstream repos to add additional requirements for a given TPL (i.e. add more libraries, headers, etc.). However, the downstream repo's find module file must find the TPL components that are fully compatible with the upstream's find module.

So you have full control of how the TPLs get defined for your project. (If there is some extra bit of control desired, let's define that.)

Trying to auto-find an upstsream TPL and all of the components you need on every platform is extremely difficult to do. That is why, IMHO, the most robust way to point to an upstream TPL is to just provide a list of link libraries and include directories. The part that TriBITS is missing is dependencies between TPLs so that a downstream TPL does not have to worry about getting the parts it needs from an upstream TPL. (But that is being fixed in #63.)

@bartlettroscoe bartlettroscoe changed the title TPLs: compatibility with CMake-distributed FindXXX.cmake TPLs: compatibility with CMake-distributed FindXXX.cmake and XXXConfig.cmake files Nov 4, 2021
@bartlettroscoe
Copy link
Member

bartlettroscoe commented Jul 14, 2022

The first issue I recall having was with Geant4 a couple of years ago. Geant4 distributes a cmake config file (which these days are modern cmake targets Geant4::XXX), and I had to do some nasty work to extract the library names and include paths for compatibility with the tribits TPL header/libraries as your documentation states.

@sethrj, this is now completely fixed as of PR #493. See the updated documentation for this at Creating FindTPL*.cmake using find_package() with IMPORTED targets.

This will be synced to Trilinos 'develop' in the new couple of days as part of PR trilinos/Trilinos#10614.

@bartlettroscoe
Copy link
Member

@sethrj, note that all of the concreate FindTPL<tplName>.cmake modules that call find_package(<externalPkg>) will then need to be upgraded as well (such as I did here). This can be done in a way that maintains backward compatibility for configure scripts that are currently passing in lists of libraries and include dirs as described in the lower part of the section Creating FindTPL*.cmake using find_package() with IMPORTED targets with the usage of tribits_tpl_allow_pre_find_package().

@bartlettroscoe
Copy link
Member

From the TriBITS perspective, I believe this is complete as of the merge of PR #493. I am putting this issue "In Review" until this gets merged into Trilinos proper. Then it will be up to the projects using TriBITS to upgrade TriBITS and refactor their FindTPL<tplName>.cmake modules as per:

@sethrj
Copy link
Contributor Author

sethrj commented Jul 15, 2022

Thanks @bartlettroscoe . We're at the final stage of rewriting the SCALE build system to use native modern CMake (with a very few thin helper functions) so the TriBITS dependencies for us now are a moot point. Cheers!

@bartlettroscoe
Copy link
Member

@sethrj, is SCALE going to be producing individual <Package>Config.cmake files or just one big massive SCALEConfig.cmake file?

@sethrj
Copy link
Contributor Author

sethrj commented Jul 15, 2022

We've got just a single SCALEConfig.cmake file. Our code is still too tightly coupled for separate package-level granularity to be meaningful: over time if we can develop stable, limited-surface-area APIs then we might be able to break our packages into separate, versioned, cleanly coupled code bases. Until then, we're just going to require that all the components that can be built will be built. (We allow disabling of some dependencies such as Qt and CUDA.)

@bartlettroscoe
Copy link
Member

So is MPACT and other downstream CMake packages going to just include the one SCALEConfig.cmake file with find_package(SCALE ...). Is SCALEConfig.cmake going to support COMPONENTS? Just curious.

@sethrj
Copy link
Contributor Author

sethrj commented Jul 15, 2022

Yep! That is the idea. I think we could use COMPONENTS to indicate the configurable options (e.g. support for CUDA, Qt, etc.) or if we add configurable package components, but our immediate goal is a very few number of possible configurations with 100% reliability for being able to configure/compile/link/test/run.

@bartlettroscoe
Copy link
Member

Yep! That is the idea. I think we could use COMPONENTS to indicate the configurable options (e.g. support for CUDA, Qt, etc.) or if we add configurable package components, but our immediate goal is a very few number of possible configurations with 100% reliability for being able to configure/compile/link/test/run.

But does that not require that SCALE have some internal structure so that you can provide only the libraries and include dirs needed to provide those COMPONENTS? So initially, there are no COMPONENTS? If someone want to use something from SCALE they will have to build all of it and put all of the SCALE libraries on the link line? Is SCALE even broken up into libraries (i.e. with no circular dependences)?

@sethrj
Copy link
Contributor Author

sethrj commented Jul 15, 2022

COMPONENTS isn't to my knowledge used to eliminate capability, just to require it. So hypothetically if you have a CUDA-enabled SCALE and don't list the CUDA component, linking against SCALE can still require linking against CUDA.

We now have about 25 "packages" (each of which has one library, down from 200+ subpackages) based on high-level code structure and "software products", among which there are no circular dependencies. We're also taking advantage of CMake's public/private dependencies, so generally linking against SCALE components only requires a couple of target_link_library arguments.

@bartlettroscoe
Copy link
Member

bartlettroscoe commented Jul 15, 2022

COMPONENTS isn't to my knowledge used to eliminate capability, just to require it.

Actually, it is completely up to the package to determine the meaning of COMPONENTS and what it means when COMPONENTS (or OPTIONAL_COMPONENTS) is missing as stated in:

Handling of COMPONENTS and OPTIONAL_COMPONENTS is defined by the package.

For Trilinos, when you leave off COMPONENTS and OPTIONAL_COMPONENTS from find_pacakge(Trilinos ...) it returns all of the Trilinos packages that were enabled, built, and installed.

We now have about 25 "packages" (each of which has one library, down from 200+ subpackages) based on high-level code structure and "software products", among which there are no circular dependencies. We're also taking advantage of CMake's public/private dependencies, so generally linking against SCALE components only requires a couple of target_link_library arguments.

How do you manage the dependencies between these 25 internal "packages"? How do you manage the dependency graph of these packages if you only enable a subset of them? Or, are all 25 of these "packages" always enabled and built unconditionally when you build SCALE? So to use one little piece of SCALE, you have to build all of SCALE? (This is the Qt problem we had where we had to build all of Qt just to use a little piece of it.)

@bartlettroscoe
Copy link
Member

Closing since PR trilinos/Trilinos#10614 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Status: Done
Development

No branches or pull requests

2 participants