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

EPIC: Features to remove to simplify TriBITS #569

Open
bartlettroscoe opened this issue Mar 28, 2023 · 3 comments
Open

EPIC: Features to remove to simplify TriBITS #569

bartlettroscoe opened this issue Mar 28, 2023 · 3 comments
Labels
component: core type: refactor Changes are mostly just a refactoring

Comments

@bartlettroscoe
Copy link
Member

bartlettroscoe commented Mar 28, 2023

Description

There a number of features supported by TriBITS that if removed would (greatly) simplify TriBITS. Some of these major features that could be removed without seriously degrading the main features of TriBITS are:

  1. Removing support for subpackages (a lot of complexity here) [Minor break in backward comparability]
  2. Remove support for auto-linking to libraries in upstream packages and require manual linking of libraries (and not just the <UpstreamPkg>::all_libs targets but intermediate targets in dependent upstream packages) ... Currently this will yield much longer link lines with duplicate libraries, see below
  3. Switch to passing in include directories in for each target (the be added with target_include_directories()) and stop using the global include_directories() command.
  4. Remove all handling of compiler options (also mentioned in EPIC: Refactoring TriBITS Core and TriBITS-based CMake projects to conform to modern CMake #411) [Minor break in backward comparability]
  5. Remove support for generating consistent explicit template instantiation (ETI) sets based on package dependencies (which sweeps backwards from downstream to upstream packages)
  6. Remove support for legacy TriBITS TPLs that take in lists of include directories, header file names, library directories, and library names and does a find (or allows the user to pass in the exact include directories and library files in order). [Major break in backward comparability]

NOTES:

Even with loosing all of the functionality listed above (and doing all of the refactorings in #411), what remains is TriBITS is very significant and not duplicated by anything in raw CMake or in any of the provided CMake modules. (Not even VTK Modules can do what TriBITS does with the flexible handling of internal and external packages.)

The improvements in dependency handling and streamlining builds provided by subpackages lost by removing support for subpackages in #1 is mitigated by moving to manual linking of intermediate targets in #2 (and also, manually linking makes the CMakeLists.txt files look more like raw CMake). As a consequence, for example, you would have to configure all of the Teuchos package but downstream packages could choose to just link against Teuchos::teuchoscore instead of Teuchos::all_libs. This means you are configuring more software that you might need but you are still only building the upstream libraries needed to build downstream targets. (But doing the install of a package would require building everything in that package, including all of its required upstream dependencies and needing to find all required upstream TPLs.) To mitigate the all-or-nothing build of existing packages currently broken into subpackages like Teuchos and SEACAS, package developers would be tempted to add their own conditional enable/disable logic which would create a lot of complexity in downstream packages to compensate for this. (This would shift complexity from the framework/tribits to the package developers which may not be a good idea and would likely make less-than-everything builds much more fragile to maintain.) This will break backward compatibility for existing Trilinos user configure scripts but will be easy and safe to fix in their scripts.

Switching to passing in include directories in #3 would allow ripping some code in code in TriBITS that gets the include directories directory property (set by the include_directories()) command and yields CMakeLists.txt files that look more like raw CMake. This would not impact Trilinos users at all.

Removing support for subpackages in #1 and some compiler option handling in #4, however, would break the ability to target specific compiler options to specific subpackages (see #442, #453 and trilinos/Trilinos#10111, for example).

The code that implements #5 is pretty complex but it greatly simplifies creating consistent explicit template instantiation sets across dependent packages. Loosing this would likely make the Trilinos CMakeLists.txt files more complex and make Trilinos harder to configure for Trilinos customers.

For removing support for legacy TriBITS TPLs in #6, that requires that all external packages/TPLs be found with a simple find_package(<ExternalPkg>) command and it requires those to find 100% modern CMake packages (i.e. provides all modern CMake targets and calls find_dependency() on all of their upstream dependencies). But there is still a problem where the name of the imported target may change or may not even exist depending on the version of the external package installed or how it is found (see the expended discussion below). However, of all of the changes listed above, this change will most strongly impact Trilinos users and break all of their existing configure scripts for Trilinos. All of the other changes would have minor impact on Trilinos users.

@bartlettroscoe bartlettroscoe added component: core type: refactor Changes are mostly just a refactoring labels Mar 28, 2023
@bartlettroscoe bartlettroscoe changed the title EPIC: Features to remove to greatly simplify TriBITS EPIC: Features to remove to (greatly) simplify TriBITS Mar 28, 2023
@bartlettroscoe
Copy link
Member Author

bartlettroscoe commented Apr 14, 2023

NOTE: One might consider removing support for external packages/TPLs with TriBITS and force packages to call find_package(<UpstreamPkg>). That would remove a good bit of complexity from TriBITS and would make it easier to pull out package subdirectories and build them as their own CMake project with little-to-know framework support. But the problem with that is that it would break the pattern of allowing the flexibility to decide, at runtime, if a package should be treated as internal or external.

@bartlettroscoe
Copy link
Member Author

But if one could not completely remove support for external packages/TPLs in TriBITS as it would destroy the ability to treat internal and external packages in a flexible manner (as discussed above), TriBITS could be greatly simplified by removing support for the TriBITS Legacy TPL System (i.e. that accepted lists of include directories, library directories, and library names and build modern CMake targets and packages out of these) and instead just rely on find_package(<ExternalPkg>) calls. This would require moving to explicit linking of all targets since there is no standard target name for "all the library targets" (e.g. #2 above and bellow bullet). In this case, every external package/TPL pulled in with find_package(<ExternalPkg>) would need to be provided by either a 100% modern CMake compliant find module Find<ExternalPkg>.cmake or package config file <ExternalPkg>Config.cmake (and the names of the IMPORTED targets would have to be stable and consistent no matter if a find module or a package config file was used, see below).

With those changes and assumptions, all that the TriBITS system would need to do is to call find_package(<ExternalPkg>) and there would be no need for the FindTPL<ExternalPkg>.cmake files. That would remove 95% of the external packages/TPL software in TriBITS and Trilinos and other TriBITS projects (because you eliminate all of the FindTPL<ExternalPkg>.cmake files).

However, this beautify utopia of CMake packages is not currently practical right now because of issues like:

  1. The current ecosystem of CMake package with find modules Find<ExternalPkg>.cmake and package config files <ExternalPkg>Config.cmake are not 100% modern CMake compliant. (That is, they don't provide modern imported CMake targets and they don't correct call find_dependency() on their upstream dependencies.) That should be the goal in the future but it is not possible right now (e.g. see the NetCDF and HDF5 example in Update MPI, HDF5, and Netcdf TriBITS find modules (#533, #534) #535 (comment)).

  2. The names of the IMPORTED targets is not always the same when calling find_package(<ExternalPkg>) between find modules and package config files. For example, the FindHDF5.cmake module shipped with CMake 3.26 provides the HDF5::HDF5 interface imported target for all provided HDF5 libraries but the HDF5Config.cmake file installed by HDF5 1.11 does not seem to provide that target and instead provides a variable HDF5_EXPORT_LIBRARIES (see here) where HDF5_EXPORT_LIBRARIES is not supported by the FindHDF5.cmake module in CMake 3.26. (Therefore, it is impossible to write simple downstream code in CMakeLists.txt files that calls target_link_libraries(<libName> PRIVATE HDF5::HDF5).

Because of constraints and problems like above, we have to have TriBITS TPL find wrapper modules like FindTPLHDF5.cmake. These wrapper FindTPL<ExternalPkg>.cmake modules provide a way to take external pakages/TPLs and provide a 100% uniform modern CMake target <ExternalPkg>::all_libs can be linked against as easy as target_link_libraries(<libName> PRIVATE HDF5::all_libs) that that works no matter how HDF5 is found externally.

@bartlettroscoe bartlettroscoe changed the title EPIC: Features to remove to (greatly) simplify TriBITS EPIC: Features to remove to simplify TriBITS Jun 23, 2023
@bartlettroscoe
Copy link
Member Author

@sebrowne, @rppawlo, @jwillenbring, I just learned that if TriBITS and Trilinos switches to manual library links, that will likely result in much longer link links due to duplicate libraries. That will slow down the links and will likely crash the link on some systems that have very long link lines (or force you to switch to the usage of resource files). That appears to be a CMake "feature". For more details, see:

So with modern CMake (as of the current release CMake 3.30), you need a system like TriBITS to manually do your linking for you in order to avoid long link lines with lots of duplicate libraries. Amazing. (NOTE: And that will yield much longer link lines than some overlinking that may be currently occurring with TriBITS auto-linking all libs from dependent upstream packages.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: core type: refactor Changes are mostly just a refactoring
Projects
Status: ToDo
Development

No branches or pull requests

1 participant