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

Complete initial implementation for treating internal packages as external found with find_package() (#63) #560

Merged
merged 49 commits into from
Mar 29, 2023

Conversation

bartlettroscoe
Copy link
Member

@bartlettroscoe bartlettroscoe commented Jan 24, 2023

Description

This PR completes the basic implementation for treating internal packages as pre-installed external packages. This completes:

in #63.

Most of the changes are related to dependency-related logic for figuring out what packages needs to be treated as external based on what packages are flagged as external packages with TPL_ENABLE_<Package>=ON. (There are some tricky use cases related to subpackages that had to be addressed.)

This includes some examples to show that it works.

Instructions for reviewers

Tasks

  • Stop the install of TribitsExProjConfig.cmake in the upstream package installs
  • Provide more examples on pre-installing packages like MixedLang
  • Get the WrapExternal package to work (likely by reading the lib name from the target using generator expressions)
  • Figure out how should we treat TPLs that are enabled in the pre-installed packages in the downstream TriBITS project? (We should not be finding them again but should just use them as found by the upstream TriBITS-compatible package installed.)
  • Update all documentation
  • Test against Kokkos, KokkosKernels, Trilinos incremental build/install driving case (see Tweaks to Trilinos to get working with pre-installed Kokkos and KokkosKernels (with TriBITS updates) (#11545) trilinos/Trilinos#11546).

@bartlettroscoe bartlettroscoe force-pushed the 63-combined-package-data-structures-6 branch from 7ad12ad to 0d8cc97 Compare January 24, 2023 20:18
@bartlettroscoe bartlettroscoe changed the title WIP: Complete initial implementation for treating internal packages as external found with find_package() WIP: Complete initial implementation for treating internal packages as external found with find_package() (#63) Jan 24, 2023
@bartlettroscoe bartlettroscoe force-pushed the 63-combined-package-data-structures-6 branch 4 times, most recently from 7e85ccc to d8e800c Compare January 25, 2023 17:29
@bartlettroscoe
Copy link
Member Author

CC: @jwillenbring, @KyleFromKitware

Following up on the task above:

  • Figure out how should we treat TPLs that are enabled in the pre-installed packages in the downstream TriBITS project? (We should not be finding them again but should just use them as found by the upstream TriBITS-compatible package installed.)

This is more complex than it would first seem. For example, with TribitsExampleProject, when we enable SimpleTpl and configure, build and install SimpleCxx, we should not be trying to find HeaderOnlyTpl and SimpleTpl again when we configure the rest of TribitsExampleProject when we pull in SimpleCxx as a pre-installed external package/TPL, we should not be trying to find HeaderOnlyTpl or SimpleTpl again (i.e. like we currently are as shown here in TEST_7 Configure rest of TribitsExampleProject against pre-installed SimpleCxx). Likewise, when we build and install Kokkos and then build the rest of Trilinos against Kokkos, we should not be trying to find the TPLs that Kokkos depends on either. The TriBITS TPLs being used by Kokkos are Pthread, CUDA, HWLOC, CUSPARSE and DLib as shown by:

$ cd Trilinos/

$ find packages/kokkos -name Dependencies.cmake -exec echo \; -exec echo {} \; -exec echo \; -exec grep _TPLS {} \; 

packages/kokkos/algorithms/cmake/Dependencies.cmake

  LIB_OPTIONAL_TPLS Pthread CUDA HWLOC
  TEST_OPTIONAL_TPLS CUSPARSE

packages/kokkos/cmake/Dependencies.cmake


packages/kokkos/containers/cmake/Dependencies.cmake

  LIB_OPTIONAL_TPLS Pthread CUDA HWLOC
  TEST_OPTIONAL_TPLS CUSPARSE

packages/kokkos/core/cmake/Dependencies.cmake

  LIB_OPTIONAL_TPLS Pthread CUDA HWLOC DLlib
  TEST_OPTIONAL_TPLS CUSPARSE

packages/kokkos/simd/cmake/Dependencies.cmake

  LIB_OPTIONAL_TPLS Pthread CUDA HWLOC
  TEST_OPTIONAL_TPLS CUSPARSE

We should not be trying finding these TPL again when we pull in pre-installed Kokkos when building downstream packages but should instead use what was found in the the upstream Kokkos package configure.

Likewise, what about a case with Trilinos where you might be installing Tpetra and all of its upstream internal package (e.g. Epetra, Teuchos, KokkosKernels, Kokkos) and external packages (e.g. CUDA, CUBLAS, CUSOLVER, BLAS, LAPACK, Pthread, etc.). So in that case, none of the these upstream external packages/TPLs should be directly processed but should instead just process the external package/TPL Tpetra in a downstsream TriBITS CMake project build.

But then what about situations where there are external packages/TPLs with dependencies like TriBITS TPLs CUBLAS and CUSPARSE depend on the TriBITS TPL CUDA? For example, Kokkos depends on the TPLs CUDA and CUSPARSE but not CUBLAS. When we build and install Kokkkos with CUDA and CUSPARSE and then try to to configure Tpetra (which needs CUBLAS) in a downstream CMake project against that external Kokkos. So when the FindTPLCUBLAS.cmake file is processed in the downstream CMake project that builds Tpetra, the CUDA::all_libs target must have already been defined. But that target will not be defined until find_package(Kokkos) is called later in the external package/TPL loop.

So that means that we can't loop over the external packages/TPLs in one loop. Instead, we need to do two loops:

  1. a first loop is over the external packages/TPLs that involve full TriBITS compliant <Package>Config.cmake files
  2. a second loop for the remaining standard TriBITS TPLs, like CUBLAS

So in the first loop, we only want to call find_package() for the most downstream TriBITS-complient external packages/tpls (i.e. that have no downstream TriBITS-complient external packages/tpls). Calling find_package() on just those terminal TriBITS-complient external packages/tpls will result in find_dependency() being called on all of the upstream TriBITS external packages. So by the time we are done calling find_package(), we should have all of the <Package>::all_libs targets defined in that subgraph of external packages/TPLs. So, in our Trilinos/Kokkos example above, the targets CUDA::all_libs and CUSPARSE::all_libs will be defined when find_package(Kokkos) is called.

Then on the second loop, any upstream packages processed in the first loop will will already have their <Package>::all_libs targets defined so we can generate their <Package>Config.cmake files dependencies on these upstream packages, we can call target_link_libraries() successfully. As for putting in links to upstream TPLs, we will need to use the set(<UpstreamTpl>_DIR ...) then find_dependency(<UpstreamTpl>) approach since we can't assume where the<UpstreamTpl>Config.cmake file actually is contained. This is where things start getting tricky.

The test for this is being added in the next commit.
This should be a little more efficient and it uses less text to call it
…NAL (#63)

This sets an INTERNAL package to EXTERNAL if TPL_ENABLE_<Package>=ON or if a
downstream dependent package is EXTERNAL.

NOTE: This is missing the changes needed to affect the processing of internal
vs. external packages but this should complete the dependency-based logic on
what packages need to be treated as EXTERNAL based on the setting of
TPL_ENABLE_<Package>=ON.

This also changes the logic so that the backward sweep to enable upstream
external packages only does so explicitly if the downstream dependent package
has <Package>_SOURCE_DIR!="" so it if it was initially defined as an internal
package then it will not change the enable/disable behavior.  This is
important because we don't (yet) want an external TPL enable to trigger the
enable of an upstream TPL.  We only want that logic to apply to initially
internal packages.  (The dependency logic can be pretty confusing if we are
not careful.)

I also removed the documentation that setting:

  -D <PackageTreatedAsExternal>_ROOT=<path>

will make an initially internal package be treated as an external package/TPL.
I think that is too subtle.  The user can still set to change where
find_package(<PackageTreatedAsExternal>) will find the
<PackageTreatedAsExternal>Config.cmake file, but that will be the only impact
of setting <PackageTreatedAsExternal>_ROOT.  (We may change our minds on this
latter and the logic for making that decision is pretty encapsulated.)

I had to make some tweaks to get this logic to work correctly:

* tribits_get_package_enable_status(): Changed logic to first just look for
  non-empty vars ${PROJECT_NAME}_ENABLE_${packageName} and
  TPL_ENABLE_${packageName} to determine if the package should be treated as
  internal or external.  If both of those are null, then use
  ${packageName}_PACKAGE_BUILD_STATUS.  This is needed for the logic where
  ${packageName}_PACKAGE_BUILD_STATUS is not changed from INTERNAL to EXTERNAL
  until later in the process.
This avoids passing two bools side-by-side in these functions.  This will make
it safter to add another enum argument for INTERNAL, EXTERNAL or "" (so any).
Noticed this while looking over this code.
…_status_from_var() (#63)

This will be used to refactor the functions in
TribitsPrintEnabledPackagesLists.cmake to display the set of internal and
external packages according to <Package>_PACKAGE_BUILD_STATUS instead of fixed
lists formed when the package are first defined.

This included adding a new filtering function
tribits_get_sublist_internal_external()
…geSublists.cmake (#63)

This is a better name for this module now that includes functions for more
that just filtering based on enable/disable status.
…63)

Now printing of the final sets of internal and external packages is based on
<Package>_PACKAGE_BUILD_STATUS and so initially internal packages that are
treated as external packages are not listed under:

  "Final set of [enabled|non-enabled] [toplevel] packages:"

but instead are listed under:

  "Final set of [enabled|non-enabled] external packages/TPLs:"

Hopefully this will make it more clear to the user how these packages are
being treated.

The new tests were updated to adjust to this different output.  (NOTE: We
could remove the direct printing and checking of
<Package>_PACKAGE_BUILD_STATUS but I will leave that in for now.)

This also helped to remove some code by doing:

* The functions tribits_print_internal_[toplevel_]package_list_enable_status()
  were renamed to tribits_print_[toplevel_]package_list_enable_status() and
  the argument internalOrExternal was added.

* The function tribits_print_tpl_list_enable_status() was deleted.
…ckages/TPL (#63)

This should hopefully make things less confusing when dealing with internal
packages that have subpackages that are treated as external packages.

At some point, I will need to add some documentation that explains these
tricky aspects of new external package support.
This also required moving the function tribits_assert_include_empty_param().

This will make it easier to implement more logic.
@@ -449,10 +445,15 @@ external package is provided by the variable::

${PACKAGE_NAME}_PACKAGE_BUILD_STATUS=[INTERNAL|EXTERNAL]

(NOT: The value of ``${PACKAGE_NAME}_PACKAGE_BUILD_STATUS`` is only changed
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE:

@bartlettroscoe
Copy link
Member Author

@KyleFromKitware and @jwillenbring, see the updated section Instructions for reviewers for the links to the rendered documentation added and updated related to this PR.

…-data-structures-6 (#63)

I resolved the conflicts in the file

* tribits/CHANGELOG.md

by moving the minimum CMake version change to the top.

I also did some minor editing which you can see by running some diffs.
@bartlettroscoe
Copy link
Member Author

@KyleFromKitware, as we just discussed, please finish and post your review of this PR or post whatever review you have complete (and note what commit you got to) before Monday, March 27, 2023. If you can't complete your review before then, we will address whatever review comments you have, merge the PR, and set up the PR for a post-merge review of the final commits.

Turns out this code was not even being used anymore after the previous
refactorings.  This removes a very long comment as well that is just not
needed.
rabartlett1972
rabartlett1972 previously approved these changes Mar 29, 2023
Copy link
Collaborator

@rabartlett1972 rabartlett1972 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With all of the review comments in #560 (review) addressed (or deferred to new Issues), this PR is ready to merge.

@bartlettroscoe bartlettroscoe dismissed KyleFromKitware’s stale review March 29, 2023 16:54

Comments were addressed or deferred to new Issues

bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this pull request Mar 29, 2023
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this pull request Mar 29, 2023
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this pull request Mar 29, 2023
Turns out this code was not even being used anymore after the previous
refactorings.  This removes a very long comment as well that is just not
needed.
@bartlettroscoe bartlettroscoe force-pushed the 63-combined-package-data-structures-6 branch 2 times, most recently from 02da993 to ee67e05 Compare March 29, 2023 17:00
Copy link
Collaborator

@rabartlett1972 rabartlett1972 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reapproving after merge and then forced push.

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

Successfully merging this pull request may close these issues.

3 participants