Set new package deps graph vars combining external and internal packages (#63) #530

bartlettroscoe · 2022-10-14T17:40:29Z

Description

This PR sets up the new package dependency vars described in the section Variables defining the package dependencies graph (except the list names <Package>_FORWRD_[LIB|TEST]_DEP_PACKAGES was changed to <Package>_FORWRD_[LIB|TEST]_DEFINED_DEPENDENCIES for consistency with the new vars and to contrast with the old vars). This is another increment working to complete #63.

See the individual commits for more details.

Instructions for Reviewers

It may be easier to review this PR one commit at a time (note next item)
NOTE: There was a change made in the handling of <Project>_ASSERT_DEFINED_DEPENDENCIES and <Project>_ASSERT_MISSING_PACKAGES part way through the set of commits. (Therefore, be sure to compare with the final version of the file before putting any time into commenting an individual git commit patch.)

…63) These names are more consistent with the other var names.

I noticed this while working on #63.

…fo.cmake (#63) That is where it should be. This function was only being called in TribitsPrintDependencyInfo.cmake. It was never being called in TribitsAdjustPackageEnables.cmake.

* tribits_set_dep_packages(): Renamed local vars and removed debugging code * tribits_append_forward_dep_packages(): Renamed local vars * tribits_parse_subpackages_append_packages_add_options(): Renamed local vars and removed deubbing code * tribits_process_package_dependencies_lists(): Rename local vars * Added period after comment link

This also exposed a defect with the file tribits/examples/MockTrilinos/TPLsList.cmake in that it did not list the TPL 'Zlib' but Zlib is listed as a TPL dependency in the file MockTrilinos/packages/zoltan/cmake/Dependencies.cmake. The updated code asserted the missing definition of Zlib as a TPL and errored out. To leave this new behavior technically breaks backwards compatibility. To keep backwards compatibility, I introduced the variable: <Project>_ASSERT_DEFINED_DEPENDENCIES and leaving it off by default to maintain backwards compatibility. NOTE: If a real TriBITS project had a case like this where a listed TPL dependency was not listed in the TPLsList.cmake file, then it would error out when trying to access the find module thorugh ${depPkg}_FINDMOD so this is not so harmful but it would be good to get the error message sooner. NOTE: As part of this, a lot of tests had to be touched because the TriBITS processing of the dependencies files no longer results in listing Zlib as a dependency of Zoltan with MockTrilinos. But this seems proper and correct and the behavior otherwise does not change.

…efined TPL Zlib (#63)

…default and doc (#63) This is still a BOOL var.

#63)

I used this for <Project>_ASSERT_DEFINED_DEPENDENCIES and I will use it for a few others as well.

…_ASSERT_MISSING_PACKAGES (#63) This is breaking backward compatibility! I just decided that it was too complex supporting <Project>_ASSERT_DEFINED_DEPENDENCIES and deprecating <Project>_ASSERT_MISSING_PACKAGES. And this is the time to remove backward compatibility. If a user tries to set <Project>_ASSERT_MISSING_PACKAGES to any non-empty value, then it will error out with a FATAL_ERROR with a good error message telling them to set <Project>_ASSERT_DEFINED_DEPENDENCIES. Therefore, users will just need to change <Project>_ASSERT_MISSING_PACKAGES=ON to <Project>_ASSERT_DEFINED_DEPENDENCIES=FATAL_ERROR or <Project>_ASSERT_MISSING_PACKAGES=OFF to <Project>_ASSERT_DEFINED_DEPENDENCIES=OFF and they will get the same behavior (except undefined external packages/TPLs will now result in a fatal error by default). To make this work, I had to only conditionally list Zlib as a TPL of Zoltan in MockTrilinos.

* Don't mention a global list since that is not helpful (I don't think) (and take the list argument away from the function) * Mention setting <Project>_ASSERT_DEFINED_DEPENDENCIES=IGNORE

…_cache_var() (#63) A few other things done here: * Don't assert correct values of ${PROJECT_NAME}_ASSERT_CORRECT_TRIBITS_USAGE in tribits_report_invalid_tribits_usage() since that is now done when defining the cache var. (And this resulted in removing the unit test for tribits_report_invalid_tribits_usage() that set ${PROJECT_NAME}_ASSERT_CORRECT_TRIBITS_USAGE=INVALID_ARGUMENT since that error will no longer be caught in that function.) * Updated documentation a little.

…ges (#63) Set the following new deps vars: * <Package>_LIB_DEFINED_DEPENDENCIES * <Package>_TEST_DEFINED_DEPENDENCIES * <Package>_[LIB|TEST]_DEP_REQUIRED_<depPkg> * <Package>_FORWARD_LIB_DEFINED_DEPENDENCIES * <Package>_FORWARD_TEST_DEFINED_DEPENDENCIES Everything will be refactored next to use these and remove usage of the old deps vars that differentiate between external packages/TPLs and internal packages. As part of this: * Changed tribits_set_dep_packages() from a function to a macro so simplify varaible management. This also resulted in changing several global_set() and dual_scope_set() statements to set() statements. * Changed tribits_append_forward_dep_packages() from a function to a macro and changed its argument list to support the new forward deps vars. This also involved changing from dual_scope_set() to set(). * Updated tests for MockTrilinos and TribitsExampleProject do set <Project>_DUMP_FORWARD_PACKAGE_DEPENDENCIES=ON and check for the new backward and forward deps vars listed above.

…-data-structures-2 Resolved conflict in: * tribits/CHANGELOG.md by combining the two sets of items from different PRs.

bartlettroscoe · 2022-10-14T17:44:14Z

Hello @KyleFromKitware, can you please do a full review of this PR and select "Approve" if you approve (or suggest changes)? Otherwise, I will be working on the next PR based in this PR's branch so there is no huge rush to get this merged.

KyleFromKitware

Looks good for the most part, I suggested a few small changes.

test/core/DependencyUnitTests/CMakeLists.txt

tribits/core/package_arch/TribitsReadDepsFilesCreateDepsGraph.cmake

test/core/DependencyUnitTests/CMakeLists.txt

tribits/core/package_arch/TribitsReadDepsFilesCreateDepsGraph.cmake

tribits/core/utils/TribitsAddEnumCacheVar.cmake

@KyleFromKitware

…530) * Change `if (<var> ${<listName>})` to `if (<var> IN LISTS <listName>)` where possible (i.e. where `<listName>` is not a macro argument). (This was suggested by @KyleFromKitware in review of PR #530.) * Adjust whitespace to be `if (<var> IN LISTS <listName>)`. (I am getting to where I want two spaces around `IN LISTS` but one space between `IN` and `LISTS`. I am just trying to come up with formatting conventions that improve readability without causing too much right drift.) * Added whitespace to other foreach() loops.

@KyleFromKitware

This is to make consistent with other cache vars using :BOOL as well. (But this does not change any observable behavior.) Suggested by @KyleFromKitware in review of PR #530.

bartlettroscoe · 2022-10-15T15:41:59Z

@KyleFromKitware, can you please have a look at the remaining unresolved conversations above here and here and the approve the PR if you are satisfied?

bartlettroscoe · 2022-10-17T15:41:38Z

@KyleFromKitware, thanks for the detailed review and the feedback! I need two write up a CMake programming conventions guide to cover all of these issues and decide the best way to handle corner cases.

Updated TriBITS 'master' now catches undefined TPLs listed as dependencies of a TriBITS package. (Amazingly, classic TriBITS simply silently ignored these undefined TPLs. See tribits/CHANGELOG.md entry in TriBITSPub/TriBITS#530.) This works with older TriBITS and will be needed when TriBITS 'master' is snapshotted into Trilinos 'develop'.

…s#11152) Updated TriBITS 'master' now catches undefined TPLs listed as dependencies of a TriBITS package. (Amazingly, classic TriBITS simply silently ignored these undefined TPLs. See tribits/CHANGELOG.md entry in TriBITSPub/TriBITS#530.) This works with older TriBITS and will be needed when TriBITS 'master' is snapshotted into Trilinos 'develop'.

Updated TriBITS 'master' now catches undefined TPLs listed as dependencies of a TriBITS package. (Amazingly, classic TriBITS simply silently ignored these undefined TPLs. See tribits/CHANGELOG.md entry in TriBITSPub/TriBITS#530.) This works with older TriBITS and will be needed when TriBITS 'master' is snapshotted into Trilinos 'develop'.

bartlettroscoe added 16 commits October 4, 2022 10:12

Fix naming of <Package>_FORWARD_[LIB|TEST]_DEFINED_DEPENDENCIES vars (#…

2f70016

…63) These names are more consistent with the other var names.

Remove ignored "" args (#63)

f397906

Adjust regexs from files for '(' and ')' (#63)

d59b154

I noticed this while working on #63.

Move tribits_print_package_dependencies() to TribitsPrintDependencyIn…

3364281

…fo.cmake (#63) That is where it should be. This function was only being called in TribitsPrintDependencyInfo.cmake. It was never being called in TribitsAdjustPackageEnables.cmake.

WIP: tribits_set_dep_packages((): Add pkgsOrTpls argument (#63)

fb5e0a4

WIP: Factor macros and functions out of tribits_set_dep_packages() (#63)

6013890

WIP: Add test case for Trilinos_ASSERT_DEFINED_DEPENDENCIES=ON on und…

88cf092

…efined TPL Zlib (#63)

WIP: <Project>_ASSERT_DEFINED_DEPENDENCIES: Add proper cache var and …

d5cc5d2

…default and doc (#63) This is still a BOOL var.

WIP: Change <Project>_ASSERT_DEFINED_DEPENDENCIES from BOOL to an enum (

d049a42

#63)

WIP: Add function tribits_add_enum_cache_var() (#63)

65def4c

I used this for <Project>_ASSERT_DEFINED_DEPENDENCIES and I will use it for a few others as well.

WIP: Update error msg for tribits_abort_on_missing_package() (#63)

2f8980d

* Don't mention a global list since that is not helpful (I don't think) (and take the list argument away from the function) * Mention setting <Project>_ASSERT_DEFINED_DEPENDENCIES=IGNORE

bartlettroscoe added type: enhancement component: documentation component: core labels Oct 14, 2022

bartlettroscoe requested a review from KyleFromKitware October 14, 2022 17:40

bartlettroscoe assigned KyleFromKitware Oct 14, 2022

Merge remote-tracking branch 'github/master' into 63-combined-package…

b64983e

…-data-structures-2 Resolved conflict in: * tribits/CHANGELOG.md by combining the two sets of items from different PRs.

KyleFromKitware requested changes Oct 14, 2022

View reviewed changes

bartlettroscoe added 2 commits October 14, 2022 16:15

Add :BOOL to some cache var args (#63, #530)

515df6c

This is to make consistent with other cache vars using :BOOL as well. (But this does not change any observable behavior.) Suggested by @KyleFromKitware in review of PR #530.

bartlettroscoe enabled auto-merge October 14, 2022 22:49

KyleFromKitware approved these changes Oct 17, 2022

View reviewed changes

bartlettroscoe merged commit 63d7181 into master Oct 17, 2022

bartlettroscoe added type: refactor Changes are mostly just a refactoring and removed type: enhancement labels Oct 17, 2022

bartlettroscoe mentioned this pull request Oct 17, 2022

kokkos: new error during cmake 3.23.1 configuration related to HPX not being found trilinos/Trilinos#11152

Closed

bartlettroscoe mentioned this pull request Oct 17, 2022

Remove listing of undefined TPL deps (#11152) trilinos/Trilinos#11154

Merged

bartlettroscoe mentioned this pull request Dec 20, 2022

TriBITS Snapshot 2022-12-12 working toward TriBITSPub/TriBITS#63 trilinos/Trilinos#11380

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set new package deps graph vars combining external and internal packages (#63) #530

Set new package deps graph vars combining external and internal packages (#63) #530

bartlettroscoe commented Oct 14, 2022 •

edited

Loading

bartlettroscoe commented Oct 14, 2022

KyleFromKitware left a comment

bartlettroscoe commented Oct 15, 2022

bartlettroscoe commented Oct 17, 2022

Set new package deps graph vars combining external and internal packages (#63) #530

Set new package deps graph vars combining external and internal packages (#63) #530

Conversation

bartlettroscoe commented Oct 14, 2022 • edited Loading

Description

Instructions for Reviewers

bartlettroscoe commented Oct 14, 2022

KyleFromKitware left a comment

Choose a reason for hiding this comment

bartlettroscoe commented Oct 15, 2022

bartlettroscoe commented Oct 17, 2022

bartlettroscoe commented Oct 14, 2022 •

edited

Loading