Fix <Project>Config.cmake for missing OPTIONAL_COMPONENTS (#299) #497
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
…TriBITSPub#299) The tests fail for this commit because it is showing a defect in TriBITS! This was reported by the Xyce team with testing against trilinos/Trilinos#10614. TriBITS was missing a test for find_package(<Project> ... OPTIONAL_COMPONENTS ...) for the <Project>Config.cmake file and the handling of these was broken. The code is successfully only including the found optional components in the var <PROJECT>_LIBRARIES but it was seting <PROJECT>_SELECTED_PACKAGES_LIST and <Project>::all_libs for all of the required and optional components, regardless if the optional components were found or not. What I did: * Upgraded the existing tests based on the DummyPackageClientCMakeLists.txt project to call find_package(<Package> REQUIRED COMPONENTS ... OPTIONAL_COMPONENTS ...) and accept the list of optional components. (This required updating several tests that were using this.) * Added a new failing test case TEST_7 that showed the incorrect handling of OPTIONAL_COMPONENTS in that it was not excluding optional components that were not found by not excluding them from the <Project>_SELECTED_PACKAGES_LIST variable or from the <Project>::all_selected_libs targets. NOTE: The new test case TEST_8 shows that you can pass in subpackages as components and it will handle that correctly as well. Other little stuff I did: * Updated some of the other vars in TribitsProjectConfigTemplate.cmake.in * Fixed some comment typos I happened to notice * Added some more comments about what things are being tested in various tests * Removed some redundancy in testing the <Project>Config.cmake and <Package>Config.cmake files * Added seperator comments between some long test code
This fixed the new failing test I just added last commit. It is just shocking how much test code I had to change to effectively test this 4-line change. But system-level tests of this type of feature are hard to write.
rabartlett1972
approved these changes
Jul 17, 2022
There was a problem hiding this comment.
I am going to allow this to merge without a real pre-merge review. This is so I can update the TriBITS snapshot for trilinos/Trilinos#10614 so Xyce can do updated testing.
16 tasks
|
Hello @KyleFromKitware, can you please do a post-merge review of this PR? |
|
LGTM |
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.
This adds a failing missing test and then fixes it for the handling of OPTIONAL_COMPONENTS with
<Project>Config.cmakefiles. The old implementation was setting<Project>_SELECTED_PACKAGES_LISTand<Project>::all_selected_libsto include all optional components, even the ones that were not found. (This was reported during Xyce testing of trilinos/Trilinos#10614.)Interestingly, the variable
<Project>_LIBRARIESwas getting set correctly. So the changes as part of #299 up to this point did not break backward compatibility with respect to OPTIONAL_COMPONENTS. It is just the new variable<Project>_SELECTED_PACKAGES_LISTand the new target<Project>::all_selected_libswere broken.See the commit message logs for more details.
Notes to Reviewers