Add, use, and document new function tribits_external_package_create_imported_all_libs_target_and_config_file() (#299)

[bartlettroscoe]

This adds the new module TribitsExternalPackageFindTplHelpers.cmake with the new function tribits_external_package_create_imported_all_libs_target_and_config_file(). This makes it very smooth and easy to use find_package(<externalPkg>) that creates proper IMPORTED targets.

Using the new function will fix the problem with FindTPLGTest.cmake reported in sandialabs/seacas#317 (comment).

Things done here:

• Added new module TribitsExternalPackageFindTplHelpers.cmake and function tribits_external_package_create_imported_all_libs_target_and_config_file() and used it to upgrade the FindTPL<tplName>.cmake modules for TribitsExampleProject2 (and updated tests to make sure they work).

• Updated documentation and howto for adding a new TriBITS TPL and write the FindTPL<tplName>.cmake file. (Hopefully this new documentation will be easier to find the code example that a particular FindTPL<tplName>.cmake file should follow.)

Notes to post-merge reviewers

• Please review the individual commits one-at-a-time and read the commit log messages for details on what was added or changed.
• The updated documentation to review is mostly at:
  • https://tribitspub.github.io/TriBITS/users_guide/index.html#tribits-external-package-tpl
  • https://tribitspub.github.io/TriBITS/users_guide/index.html#how-to-add-a-new-tribits-external-package-tpl
  • https://tribitspub.github.io/TriBITS/users_guide/index.html#tribits-external-package-create-imported-all-libs-target-and-config-file
  • https://tribitspub.github.io/TriBITS/users_guide/index.html#tribits-tpl-find-include-dirs-and-libraries
  • https://tribitspub.github.io/TriBITS/users_guide/index.html#tribits-external-package-write-config-file-str

[bartlettroscoe]

I am going to go ahead and merge so we can get the updated documentation built and deployed. Then I will request a post-merge review.

[rabartlett1972]

Going to merge so we can see the updated documentation and so we can update TriBITS in sandialabs/seacas#317, the merge sandialabs/seacas#322 and verify that the problem is resolved.

I will then request a post-merge review.

[bartlettroscoe]

Hello @KyleFromKitware, can you please do a post-merge review of this PR? Please see the notes above in the section "Notes to post-merge reviewers".

Hello @gsjaardema, can you please look over the updated documentation related to writing and upgrading FindTPL<tplName>.cmake files in:

• https://tribitspub.github.io/TriBITS/users_guide/index.html#how-to-add-a-new-tribits-external-package-tpl

and especially in the section:

• https://tribitspub.github.io/TriBITS/users_guide/index.html#creating-findtpl-cmake-using-find-package-with-imported-targets

?

That should solve the problem with seacas/cmake/TPLs/FindTPLGTest.cmake being experienced in sandialabs/seacas#317 and is used in PR sandialabs/seacas#322.

[bartlettroscoe]

FYI @keitat

[KyleFromKitware]

Should be @FUNCTION, not @FUCNTION

[KyleFromKitware]

One of the difficulties I often have with reviewing TriBITS is the very long names of functions. Because there's no Prefix_CamelCase or prefix__snake_case, there's not a clear distinction between the prefix and the function name. I know public-facing functions can't be changed due to backwards compatibility, but it would be nice if internal functions used some clearer naming, like ExternalProject_Add which CMake has.

[bartlettroscoe]

CC: @keitat

@KyleFromKitware

Yea, I see the problem. I will need to think about how to resolve this. How about going with the concept of collections of functions and using a prefix with CamelCase like TribitisExtPkg_add_find_upstream_dependencies_str() instead of tribits_external_package_add_find_upstream_dependencies_str()? If you go with CamelCase, you could do TribitisExtPkg_addFindUpstreamDependenciesStr(). An issue with CMake is that command calls are case insensitive so people could call that as tribitisextpkg_addfindupstreamdependenciesstr() which is completely unreadable. I guess all things considered, I would rather type and read TribitisExtPkg_addFindUpstreamDependenciesStr() than tribits_external_package_add_find_upstream_dependencies_str(). And we can use case-insensitive greps to find all occurrences of a function.

If you look at most of the public-facing TriBITS functions worth documenting at TriBITS Macros and Functions, you will see that some of them are grouped as:

• tribits_package_xxx()
• tribits_tpl_xxx()

so those could become:

• TribitsPkg_xxx()
• TribitsExtPkg_xxx()

(because we are changing the name from "TPL" to "External Package" as part of #63).

There is a desire to break TriBITS up into smaller collections of reusable software components (i.e. #368) and we could give each of these "components" a single prefix, such as TribitsTesting_ for all of the testing-related functions and macros.

What we need is a CMake coding standards guide for larger software collections written in CMake that helps to address problems like this and other issues since CMake lacks core language features that other languages have (like module scoping).

As for backward compatibility, this is now the time to break that and we could support function renaming with deprecated wrappers (i.e. #429) or with renaming scripts that clients can run on their code when they upgrade. (The former is more work but is a better path.)

But I would like to hold off on a major renaming of TriBITS functions until we get the epic #367 completed while trying to avoid breaking backward compatibility as much as possible. But after that epic is complete, we need to set our sights on the larger refactorings to further componentize TriBITS Core (i.e. #368) and modernize it (i.e. #411).

[bartlettroscoe]

@KyleFromKitware, also note that the function:

• tribits_external_package_append_upstream_target_link_libraries_get_name_and_vis()

is being renamed to:

• tribits_external_package_get_dep_name_and_vis()

as part of #494 shown here.

[KyleFromKitware]

If you go with CamelCase, you could do TribitisExtPkg_addFindUpstreamDependenciesStr(). An issue with CMake is that command calls are case insensitive so people could call that as tribitisextpkg_addfindupstreamdependenciesstr() which is completely unreadable.

Yes, and the answer to this problem is "don't do that".

FWIW, I would use UpperCamelCase_UpperCamelCase rather than UpperCamelCase_lowerCamelCase. That's the convention used by ExternalProject and FetchContent.

[bartlettroscoe]

As a short-term compromise, how about I change the long prefix tribits_external_package_ to the shorter prefix tribits_extpkg_? As these are new functions that have not been technically synced to Trilinos yet, it is fine to change the name now.

[KyleFromKitware]

Sounds fine to me - it's ultimately up to you on how to name it.

[bartlettroscoe]

FWIW, I would use UpperCamelCase_UpperCamelCase rather than UpperCamelCase_lowerCamelCase. That's the convention used by ExternalProject and FetchContent.

I am just so used to coding conventions where proper nouns for package names, namespaces, and modules use UpperCamelCase but verbs for function and macro names use lowerCamelCase(). In C++, the function would be named TribitisExtPkg::addFindUpstreamDependenciesStr(). So a direct mapping to CMake would be to replace : with _ with TribitisExtPkg_addFindUpstreamDependenciesStr(). I guess to make this more clear/formal we could use a double underscore __ to replace : so it would be TribitisExtPkg__addFindUpstreamDependenciesStr(). It is all what you are used to I guess.

[KyleFromKitware]

we could support function renaming with deprecated wrappers (i.e. #429)

If you're using a new enough CMake, here's a convenient way to add deprecated aliases for functions:

function(add_deprecated_alias alias name)
  cmake_language(EVAL CODE "
    macro(${alias})
      message(DEPRECATION \"${alias} is deprecated - use ${name} instead\")
      ${name}(\${ARGN})
    endmacro()")
endfunction()

Note that this doesn't get you perfect argument forwarding, which unfortunately is impossible with the current state of CMake.