Add support for FindTPL<tplName>Dependencies.cmake and doc updates (#63, #299, #494) #495
Conversation
This is just a few places where I noticed that I had called set(<var>) expecting to set it to empty "" but that actually unsets the var and is equivalent to unset(<var>)! You initailize and empty var or list using set(<var> "").
…ub#63, TriBITSPub#494) And with this I can strengthen the tests later.
…b#63, TriBITSPub#299, TriBITSPub#494) Here is what I did in detail: * Created new module TribitsPackageDependencies.cmake (and unit test driver) to collect standard dependency defintion macros and functions into. * Created function tribits_external_package_define_dependencies() that sets <tplName>_LIB_ALL_DEPENDENCIES and tribits_external_package_setup_enabled_dependencies() that sets <tplName>_LIB_ENABLED_DEPENDENCIES based on what TPLs are actually enabled. (This should address TriBITSPub#494.) * Renamed function tribits_external_package_append_upstream_target_link_libraries_get_name_and_vis() to tribits_external_package_get_dep_name_and_vis() and moved from TribitsExternalPackageWriteConfigFile.cmake to TribitsPackageDependencies.cmake. * Improved a bit of the existing documentation.
…TriBITSPub#63, TriBITSPub#299, TriBITSPub#494) This commit primarily adds support for FindTPL<tplName>Dependencies.cmake files so you don't need to call tribits_external_package_define_dependencies() from the TPLsList.cmake file (which was a terrible hack). As part of this, I also updated the documentation related to external packages/TPLs. More specifically I did the following: * Added support for FindTPL<tplName>Dependencies.cmake files which including adding a new TPL var <tplName>_DEPENDENCIES_FILE. That included adding a new macro tribits_read_external_package_deps_files_add_to_graph() that gets run for each TPL during the generation of the project package dependencies (and added to teh call-graph for tribits_read_all_project_deps_files_create_deps_graph()). * Expanded documentation on TriBITS External Packages/TPLs: - Properly listed out FindTPL<tplName>.cmake and FindTPL<tplName>Dependencies.cmake in the section 'TriBITS External Package/TPL Core Files' - Added linked documentation entries for FindTPL<tplName>.cmake and FindTPL<tplName>Dependencies.cmake - Updated list of step to add a new TPL - Added a section 'Creating FindTPL<tplName>.cmake file' with mini-toc to the subsections to make easy to navigate - Added MUST_FIND_ALL_LIBS to example calls of tribits_tpl_find_include_dirs_and_libraries() * Refactored TribitsExampleProject2: - Added FindTPL<tplName>Depenencies.cmake files for Tpl2, Tpl3, and Tpl4. - Remove hacked calls to tribits_external_package_define_dependencies() in TPLsList.cmake * Other users and maintainers guides updates: - Sorted items in TribitsSystemMacroFunctionDocTemplate.rst - Changed from 'FindTPL*.cmake' to 'FindTPL<tplName>.cmake' in the documentation (which I think looks better). - Other small documentation fixups I noticed while looking at this stuff. * Changed over to foreach ( ... IN LISTS ... ) for a few loops.
…b#63, TriBITSPub#299, TriBITSPub#494) There was feedback from @KyleFromKitware that this was too long and too hard to see the prefix.
…Pub#63, TriBITSPub#299, TriBITSPub#494) I also reformated the entry noting that TPLs must declare dependencies.
|
@KyleFromKitware, can you please do a post-merge review of this PR? Please see specific requests in "Notes to reviewers" above. |
| foreach(TRIBITS_PACKAGE ${${PROJECT_NAME}_SE_PACKAGES}) | ||
| tribits_print_direct_package_dependencies_lists(${TRIBITS_PACKAGE}) | ||
| message("\nDumping direct dependencies for each package ...") | ||
| foreach(tribitsPkg IN LISTS ${PROJECT_NAME}_DEFINED_TPLS ${PROJECT_NAME}_SE_PACKAGES) |
There was a problem hiding this comment.
A few days ago you mentioned the idea of using double spaces between argument groups for readability. In that case, I think there should only be one space in IN LISTS.
There was a problem hiding this comment.
I find that I have trouble seeing the spaces between arguments in some CMake code because there are no comas or other delimiter like:
some_long_text_string and_does_this_keep_going
find it easier to see the gap when I use two spaces in these cases to give:
some_long_text_string and_does_this_keep_going
So I have gravitated to using two spaces to separate arguments that have no other delimiter. But I find it is easy to see the gap in cases like ...some_long_variable_name} some_other_long_argument or some_long_other_argument ${some_long_variable_name... or ...some_long_value" some_other_long_argument. The presence of } or ${ or " fixes the problem for me and I don't need the extra space to see the gap.
And, to be consistent, I have gravitated gravitated to using two spaces for all of these cases where there is no delimiter.
And the case for IN LISTS with foreach() is very confusing to me because I always mix this up with IN_LIST with if(). I keep wanting to write foreach( ... IN_LISTS ...) instead of foreach( ... IN LISTS ...) (which is a very annoying bug to track down).
| include(TribitsParseArgumentsHelpers) | ||
| include(MessageWrapper) | ||
|
|
||
| cmake_policy(SET CMP0011 NEW) # include() does policy push and pop |
There was a problem hiding this comment.
Unfortunately, once execution reaches this line, it's too late to set CMP0011 to NEW, since it was already OLD when the file was included and the policy scope not pushed.
That said, CMP0011 is very old, and anyone who has cmake_minimum_required(VERSION 2.6) or newer (which they absolutely should if they're using modern usage requirements as required by modern TriBITS), then they'll already have CMP0011 set to NEW anyway.
There was a problem hiding this comment.
I think the issue is that I am hitting this in the cmake -P script TribitsPackageDependencies_UnitTests.cmake because it does not include a call to cmake_mimimum_required(). I will try putting in cmake_mimimum_required(VERSION 3.17) in all of the TriBITS CMake -P scripts and take out all of these cmake_policy(SET CMP0011 NEW) calls. (My guess is that this is why I added several other cmake_policy() calls like this as well over the years.
There was a problem hiding this comment.
Addressed in #498
CMake -P scripts need to call cmake_minimum_required() to avoid CMake policy warnings like CMP0011 about file includes. This was flagged by @KyleFromKitware in TriBITSPub#495. I decided to also clean this up by calling: cmake_minimum_required(VERSION 3.17.0 FATAL_ERROR) in every CMake -P script in TriBITS. I also moved all of the policy setting to the file TribitsCMakePolicies.cmake and include that with: include(TribitsCMakePolicies NO_POLICY_SCOPE) Also, you have to include that or call cmake_minimum_version() before any other include(...).
…riBITSPub#299, TriBITSPub#495) I also removed the loop for PARSE_DEPENDENCIES as suggested by @KyleFromKitware in TriBITSPub#495 review.
| include(TribitsParseArgumentsHelpers) | ||
| include(MessageWrapper) | ||
|
|
||
| cmake_policy(SET CMP0011 NEW) # include() does policy push and pop |
There was a problem hiding this comment.
Addressed in #498
See the individual commits for details.
Notes for reviewers