Repalce usage of legacy vars <Package>_[LIB|TEST]_[REQUIRED|OPTIONAL]_DEP_[PACKAGES|TPLS] (#63) #546
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Noticed this while working on #63
This is the right thing to do but it also helps in testing dependencies printouts for ReducedMockTrilinos.
…s order, fix printing deps (#63) I did several things here: * Renamed the macro tribits_setup_direct_package_dependencies_lists_and_lib_required_enable_vars() to tribits_setup_enabled_dependencies_lists_and_enable_vars() * Removed the resetting of <Package>_[LIB|TEST]_DEFINED_DEPENDENCIES after the adjustment of the enables/disables. (This resulted in <Package>_[LIB|TEST]_DEFINED_DEPENDENCIES also listing out TPLs as well.) * Refactored tribits_print_nonempty_package_deps_list() to accept 'definedOrEnabled' argument. * Changed the order of TPLs and Packagse in <Package>_[LIB|TEST]_DEFINED_DEPENDENCIES so that PACKAGES are listed first, then TPLS second. This just looks more logical. (This required updating some of the tests that are sensitive to this ordering.) * Changed the printing of <Package>_[LIB|TEST]_ENABLED_DEPENDENCIES to use tribits_print_nonempty_package_deps_list(). * Changed printing of "<Package>: No dependendies!" to "<Package>: No defined dependendies!" (to differentiate from "<Package>: No enabled dependendies!"
…ILD_STATUS (#63) Turns out that this var was already being set for external and internal packages. I just added some more printouts and another test for this. (I have no memory of having set this.)
#63) There is no usage for this varaible. Removing it just removes clutter and future possible problems.
#63) This refactors just the macro tribits_sweep_forward_apply_disables() and the macro it calls to use the new deps lists. But a lot of infrastructure had to be set up to do this and some defects had to be fixed in the new data-structures. This commit includes: * Created new macro tribits_append_legacy_forward_dep_packages() from the existing macro tribits_append_forward_dep_packages() that only updates the legacy lists and the updated macro tribits_append_forward_dep_packages() was refactored to only update the new lists (and just reads `<Package>_[LIB|TEST]_DEFINED_DEPENDENCIES`). * Populated `<Package>_FORWARD_LIB_DEFINED_DEPENDENCIES` for external packages/TPLs * Reordered `<Package>_[LIB|TEST]_DEFINED_DEPENDENCIES` to add defined internal packages first and then defined external packages * Changed so that hard disables of upstream TPLs disable downstream TPLs in addition to disabling downstream internal packages. (Forward disables TPL_ENABLE_<tplName>=OFF instead of <Project>_ENABLE_<tplName>=OFF. Added tribits/CHANGLOG.md for this change in behavior that might break backward compatibility for some rare cases. Removed enable/disable behavior "TPL disable triggers auto-disables of downstream dependencies" and updated "Package disable triggers auto-disables of downstream dependencies" to handle both external and internal packages.) * Now, messages printed for the disables of downstream packages for disabled required upstream TPLs say "package" instead of "TPL" in the messages "... dependence on disabled package <tplName>". (Hopefully that will not break any backwards compatibility for any users who might be grepping for this but it did impact tests that had to be updated.) * Added new module TribitsGetPackageEnableStatus.cmake with new function tribits_get_package_enable_status() that will make refactoring the rest of the code easier and compact it. * Updated local var names in some macros and functions * Added and updated tests for all of these changes Hopefully the rest of the refactorings will go more smoothly now.
…to ${PROJECT_NAME}_[REVERSE_]NOTDISABLED_INTERNAL_PACKAGES (#63)
I also moved some code around to be more logical
* Rename tribits_set_package_enable_based_on_global_enable() to
tribits_set_package_enable_based_on_project_enable()
* Remove more debug-only code based on ${PROJECT_NAME}_VERBOSE_CONFIGURE (it
is just clutter really)
* Simplify some if() logic
* Rename some local vars
* Remove unused vars
* Break up long lines to be within about 90 chars
This will help to tighen up some code as part of #63.
…deps vars (#63) * Rename local vars * Simplifiy/improve if() logic There was no code using the old deps vars.
…e new deps vars (#63) Also: * Refactored tribits_get_[enabled|disabled|nonenabled|nondisabled]_list() fucntions to operate on both internal and external packages correctly * Add test for catching a missed test case * Add new lists ${PROJECT_NAME}_[REVERSE_]ENABLED_INTERNAL_PACKAGES for one usage * Renamed local vars * Cleaned up some if() logic SQUASH AGAINST: WIP: Refactor tribits_sweep_backward_enable_upstream_packages() to use new deps vars (#63)
…#63) These new names make more sense and they will sort together in the documenation.
This just makes sense to alert the user of this override. Before, the override was silent.
…ocess TPLs (#63) * Added calls to tribits_get_package_enable_status() to make work for internal and external packages. * The message printed when enabling a TPL with the combined logic is different so the tests had to change. * Fixed logic in tribits_set_up_enabled_only_dependencies() for package's with index '0' that was handled incorrectly. * Had to set ${PROJECT_NAME}_DEFINED_PACKAGES in unit test driver because this is now used in tribits_adjust_package_enables(). * Change name of TPL_<tplName>_ENABLING_PKG to <tplName>_ENABLING_PKG and set for all packages, not just for packages triggering the enable of required TPLs. (Was needed for info on why a TPL got enabled so a user understands.) * Removed some commented-out debug code. WARNING: This does **not** handle the implicit enable of upstream TPLs correctly. With this current implementation, if a package depends on LAPACK, for example, TriBITS will automatically try to enable BLAS. That is not really the behavior that we want (yet) and breaks backward compatibility. I will fix that in a later commit. (It just so happens that the entire TriBITS test suite passes so that means we are missing a test for this use case.)
Now that the same code is being used for internal and external packages, we are not even calling this TPL-specific code anymore.
… new deps vars (#63) As part of this: * Removed the macro tribits_private_add_optional_tpl_enable() and just call tribits_private_add_optional_package_enable() for TPL dependencies. (This changes the doc string to say "package" instead of "TPL" but in the new TriBITS a TPL is just an external package so that is okay.) * Changed name of tribits_set_up_optional_package_enables_and_cache_vars() to tribits_setup_optional_package_enables_and_cache_vars() (since "setup" is a legitimate noun). * Renamed some local vars
…_enables() to use new deps vars (#63) * Changed name from tribits_enable_parent_package_for_subpackage_enable() to tribits_do_final_parent_packages_enables_for_subpackage_enables() which is more accurate. * Change the name from tribits_postprocess_package_with_subpackages_enables() to tribits_set_parent_package_subpackage_enable_for_enabled_subpackages() which is more accurate. * Changed name from tribits_postprocess_package_with_subpackages_test_example_enables() to tribits_set_parent_package_test_example_enable_for_enabled_subpackages() which is more accurate. * Changed names of some local vars * Improved some comments
…stPackageEnables.cmake (#63) This macro is every only called from inside of TribitsAdjustPackageEnables.cmake so it should be in there.
* Removed some vars that were never being used. (We can add then back later if they are ever needed.) * Renamed some local vars
…b_required_enable_vars() to use new deps vars (#63) This really compated the code in the macro tribits_setup_enabled_dependencies_lists_and_enable_vars(). Also: * Updated comments * Adjust some if() statment formatting
…s() to use new deps vars (#63) I also did: * Updated some documentation * Renamed some local vars
…new deps vars (#63) This is not really updated correctly because it include exteranl dependencies as well but this does not break any TriBITS packages and I am not sure who is using component-based installs with TriBITS projects. This can be fixed correctly once I determine that it is needed and we can develop a test for this.
It seems there is no more need of this function as it is not used anywhere in TriBITS. And this is a trivial function which is really just a single loop.
Otherwise, I just updated a few comments. But the code is unchanged.
* Make fit in < 90 char columns (mostly) * Rename local vars * Make dependency on ctest_driver/dump-cdash-deps-xml-file.py script optional
* Factored out function tribits_get_legacy_package_deps_sublist() * Factored out function tribits_append_dep_to_xml_string()
* Removed code
* Removed tests
* Removed documentation
NOTE: These separate list vars are maintained in several places to maintain
backward compatibility but none of the key internal logic uses these.
NOTE: To find this I ran a recursive grep using the regex:
(LIB|TEST|[$][{][a-zA-Z_]+[}])_(OPTIONAL|REQUIRED|[$][{][a-zA-Z_]+[}])_DEP_(PACKAGES|TPLS|[$][{][a-zA-Z_]+[}])
There was no test in TriBITS that was testing that upstream TPLs don't get enabled by default (yet). Amazingly, the test IndirectTplDependency_NoImplicitEnableBLAS passes! I had expected it to fail giving the implementation I used in the refactoring. In a later refactoring, I think we should make it so that TPLs upstream from an enabled TPL should automatically be enabled (perhaps as determined by a configure-time option).
Just cleaning up some for files I touched.
|
Hello @KyleFromKitware, this is a big PR (see above suggestions for how to do the review commit-by-commit). It would be great if you could review it before you go on vacation next week. But if you can't, I will just merge it and set it up for a post-merge review when you get back in Jan 2023. |
rabartlett1972
approved these changes
Dec 12, 2022
|
Hello @KyleFromKitware, can you please do a post-merge review of this PR once you get back and have some time? |
4 tasks
|
@KyleFromKitware, is this all of the feedback you have on this PR? |
|
This is it for the moment, but I'll have more time on Wednesday and Thursday if you'd like me to take another look after our biweekly meeting tomorrow. |
@KyleFromKitware, sure, this is a lot of code touched and refactored so it should take a couple of hours to go over all of it carefully. For now I will just post a PR that addresses the comments above and then I can post another one after that to address any other comments. |
bartlettroscoe
added a commit
to bartlettroscoe/TriBITS
that referenced
this pull request
Dec 12, 2022
This was feedback from @KyleFromKitware. * Fixed spelling of tribits_create_reverse_list() * Use lower-case set() * Put "${...}" around var name var in if ( ... "${${nameOfVar}}" STREQUAL "") to avoid confusion Run 'git log -p -1 --word-diff-regex=.' to best see what changed (and motivation for the changes should be obvious).
9 tasks
bartlettroscoe
added a commit
to bartlettroscoe/TriBITS
that referenced
this pull request
Feb 21, 2023
, TriBITSPub#546) This documents a break in backwards compatibility that occurred with the merge of PR TriBITSPub#546.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The primary purpose of this PR is remove the setting and usage of the legacy vars
<Package>_[LIB|TEST]_[REQUIRED|OPTIONAL]_DEP_[PACKAGES|TPLS]that treat external TPLs and internal Packages as separate entities and instead use the new vars:<Package>_[LIB|TEST]_[DEFINED|ENABLED]_DEPENDENCIES<Package>_[LIB|TEST]_DEP_REQUIRED_<depPkg><Package>_PACKAGE_BUILD_STATUSIt also replaces some of the loops over just internal packages and TPLs with the entire list of packages (and then switching on
<Package>_PACKAGE_BUILD_STATUS).This takes us one step closer to completing #63 and being able to allow packages to be defined internally or externally in a fluid way.
There are still some more refactoring needed to complete the initial use case driving #63 (Use Case 3: Configure/build pointing to a subset of already installed TriBITS packages in same repo).
Instructions for Reviewers
This PR involving large blocks of code around in the same file and between different files. But these simple movements of code (with no changes in that code) are done in their own commits. Therefore, the best way to review this PR is to review it commit-by-commit.
There were also some other small refactorings and updates that were done and these are isolated into their own commits.