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

Address some issues for initial implementation for treating internal packages as external (#63) #562

Open
1 of 10 tasks
bartlettroscoe opened this issue Feb 21, 2023 · 4 comments

Comments

@bartlettroscoe
Copy link
Member

bartlettroscoe commented Feb 21, 2023

Parent Issue:

Description

While the basic implementation for treating internally defined packages as external package in PR #560 is is complete (part of #63), it could use some more checking and smooth out some use cases. The following should be done following the merge of #560 when we have some time:

  • Address TriBITS-compliant external packages missing TriBITS-compliant upstream dependencies:

    • Add check that <tplName>::all_libs are defined for all enabled TPLs at the end of the processing of TPLs (and add a test case that violates this) ... DID NOT IMPLEMENT

    • or

    • Consider making it so that if a TriBITS-compliant external packages do not provide the <UpstreamPkg>::all_libs target for all of its upstream external packages/TPLs then the external package/TPL must be found again by TriBITS in the 2nd loop (and put in checks that external packages/TPLs upstream from these also don't have the <UpstreamPkg>::all_libs target defined either) ... Implemented in PR Support non-fully TriBITS-compliant external packages (#63) #576

  • Address needing to find again subpackage or the parent package already found by TriBITS-compliant external packages being pulled in by either:

    • Setting <ParentPackage>_ROOT in subpackage <Package>Config.cmake files and then looping over TriBITS-compliant external packages (and those upstream from them) in reverse order (i.e. so that find_package(KokkosKernels) will call find_dependency(Kokkos<Subpkg>) which will result in setting Kokkos_ROOT which will allow find_package(Kokkos) to find KokkosConfig.cmake). ... Looping in reverse order was implemented in PR #576 but not setting <ParentPackage>_ROOT.

    • or

    • Looping over subpackages for TriBITS-compliant external packages and not trivially enabled parent TriBITS-compliant external packages (i.e. call tribits_adjust_internal_external_packages() before tribits_do_final_parent_packages_enables_for_subpackage_enables() and set var <Package>_IS_TRIVIALLY_ENABLED_PARENT_PACKAGE=TRUE for parent packageS that are only enabled at the end because one or more of their subpackages were enabled) (don't call find_package() on trivially enabled parent TriBITS-compliant external packages). ... This will NOT be implemented!

  • Add check that each TriBITS-compliant external package has the same upstream enables and disables as determined in the downstream TriBITS project (and add a test case that violates this). (This can only be done for fully TriBITS-compliant external packages.)

  • Add tests for setting <Project>_ENABLE_<tplName>=ON and <Package>_ENABLE_<tplName>=ON and make sure that the TPL is enabled correctly and processed in each case since <Project>_ENABLE_<tplName>=ON now triggers the enable of a TriBITS external package/TPL (i.e. defined with a FindTPL<tplName>.cmake file).

  • Add support and tests for FindTPL<tplName>Dependencies.cmake files for TriBITS-compliant external packages listed in the TPLsList.cmake file with the special value TRIBITS_PKG:<path-to-dir-or-file>.

  • Add selected unit tests for tribits_extpkgwit_XXX() functions in TribitsExternalPackageWriteConfigFile_UnitTests.cmake.

  • Update existing TribitsExampleProject_External and TribitsExampleProject2_External tests to directly include() every TriBITS-generated <tplName>Config.cmake file in a different dummy CMake project to ensure they can stand alone.

  • Extend package-by-package mode with tribits_ctest_driver() to build and install packages one at a time and configure the next package (consider copying the TriBITS project source dir and deleting all of the files and dirs under <packageDir>/ except the Dependencies.cmake file after the package is installed).

  • Set up test for TribitsExampleProject using package-by-package mode with intermediate install of each package in order.

  • Set up test for TribitsExampleProject2 using package-by-package mode with intermediate install of each package in order.

@bartlettroscoe
Copy link
Member Author

CC: @ibaned, @crtrott, @jwillenbring, @rppawlo

@dalg24 and @masterleinad, following up from our meeting on Wednesday, I think there is a way to address special TPLs upstream from Kokkos like CUDA where you don't want to have to provide upstream <UpstreamPkg>::all_libs targets like CUDA::all_libs targets or TriBITS-compliant external package config files like CUDAConfig.cmake. As mentioned above, the idea is:

  • Address TriBITS-compliant external packages missing TriBITS-compliant upstream dependencies:

    • ...

    • Consider making it so that if a TriBITS-compliant external packages do not provide the <UpstreamPkg>::all_libs target for all of its upstream external packages/TPLs then the external package/TPL must be found again by TriBITS in the 2nd loop (and put in checks that external packages/TPLs upstream from these also don't have the <UpstreamPkg>::all_libs target defined either) ...

The idea is that when TriBITS calls find_package(Kokkos) when processing Kokkos as a TriBITS-compliant external package (which sets Kokkos_ENABLE_CUDA=ON) and KokkosConfig.cmake does not provide CUDA::all_libs target, then TriBITS will go ahead and process the TPL (e.g. CUDA) as usual TriBITS TPL that loads the file:

which is just two lines:

find_package(CUDAToolkit REQUIRED)  # Will abort if not found!
tribits_extpkg_create_imported_all_libs_target_and_config_file( CUDA
  INNER_FIND_PACKAGE_NAME  CUDAToolkit
  IMPORTED_TARGETS_FOR_ALL_LIBS  CUDA::cufft  CUDA::cublas  CUDA::cudart )

So if KokkosConfig.cmake called find_package(CUDAToolkit), then find_package(CUDAToolkit REQUIRED) would hopefully process the same FindCUDAToolkit.cmake find module, producing the same targets (actually skipping those because they are already defined) and the command tribits_extpkg_create_imported_all_libs_target_and_config_file() would create the CUDA::all_libs target and generate the CUDAConfig.cmake file as needed by downstream TriBITS packages that depend on CUDA.

I think that solves the problem you expressed (with the disadvantage that a different TPL could be found by Trilinos that what is found by Kokkos).

The constraint is that if a TriBITS-compliant external package does provide the <UpstreamPkg>::all_libs target from one of its upstream TPLs (known by the downstream TriBITS project), then it must provide them for all of that TPLs downstream TPLs as well. For example, if KokkosKernelsConfig.cmake provided CUBLAS::all_libs but did not provide CUSPARSE::all_libs, then that would be an error (because CUSPARSE --> CUBLAS --> CUDA as shown by FindTPLCUBLASDependencies.cmake and FindTPLCUBLASDependencies.cmake).

Does that make sense?

So we would be back in the situation that it may be possible for the configuration of Trilinos to find different TPLs that what Kokkos found when it was configured. That is not great but those are the breaks for packages that are not fully TriBITS compliant. (Fully TriBITS-compliant external packages that provide <UpstreamPkg>::all_libs targets and <UpstreamPkg>Config.cmake files for all of their TriBITS-known upstream TPLs would result in never having the same TPL searched for twice.)

This makes TriBITS more complex but it is what it is.

bartlettroscoe added a commit that referenced this issue Apr 4, 2023
I noticed these while reading over documentation while adding a comment
to #562.
@bartlettroscoe
Copy link
Member Author

CC: @ibaned, @crtrott, @jwillenbring, @rppawlo

@dalg24 and @masterleinad,

The other option is to treat Kokkos as an non-TriBITS-compliant external package with imported targets as described in:

This would allow the creation of a FindTPLKokkos.cmake file to map from whatever KokkosConfig.cmake is providing to what is needed by downstream TriBITS Trilinos packages. And we would need to write a wrapper CMakeLists.txt file under Trilinos/packages/kokkos that did similar mappings. So if you turn off all of the Kokkos tests, that could make it so that the native Kokkos raw CMake build system would not have to be TriBITS compliant at all (other than using modern CMake targets for everything).

Support for this type package that that is listed both as a TriBITS external package/TPL (in the TPLsList.cmake file) and as a TriBITS internal package (in the PackagesLists.cmake file) is not there yet but it may not be that hard to add such support. That would give the maximum flexibility for how existing CMake-based software can be pulled into a TriBITS project (either as external or internal packages or both with the decision made at runtime).

@jwillenbring
Copy link
Contributor

@bartlettroscoe For the first option, in the case where everything was being built within Spack, wouldn't it be the case that the Spack dependencies could be used to help make sure we are using consistent versions of TPLs? This obviously does not fully solve the problem, but it would mitigate the impact of the possibility of finding different versions of TPLs if that was not possible for the case where we are building with Spack.

@bartlettroscoe
Copy link
Member Author

CC: @ibaned, @crtrott, @jwillenbring, @rppawlo, @dalg24, @masterleinad

@bartlettroscoe For the first option, in the case where everything was being built within Spack, wouldn't it be the case that the Spack dependencies could be used to help make sure we are using consistent versions of TPLs?

@jwillenbring, for dependencies upstream from Kokkos that are found with find_package(<UpstreamDep> ...), then an overlord like Spack should make sure that the same dependency if found by find_package(<UpstreamDep> ...) whether called by Kokkos or any downstream CMake project. But this can really only be guaranteed as long as only one version/configuration of <UpstreamDep> is is findable. If more than one version/configuration of <UpstreamDep> is findable, then this is not guaranteed, even with the setting of the <UpstreamDep>_ROOT environment or cache variable because of case sensitivity as described in:

And note the fundamental problem with finding different external dependencies with multiple calls to find_package() in even the same CMake project described in:

This problem has no answer with raw CMake other than to tell users to make sure that does not happen (and modify the calls to find_package() in some cases).

This new TriBITS implementation and the way it handles external dependencies should fully resolve that problem by making sure that an external dependency is ever only found once, no matter how many individual CMake projects are stringed together and may have the same dependency with no need to add anything to the environment in downstream CMake projects. And by providing these more robust <Package>Config.cmake files, systems like Spack benefit from that as well and be more robust going forward. (There is a win-win relationship here between TriBITS and Spack.)

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

No branches or pull requests

2 participants