Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for exporting package info vars into <Package>Config.cmake files (#516) #520

Merged

Conversation

bartlettroscoe
Copy link
Member

@bartlettroscoe bartlettroscoe commented Aug 20, 2022

Addresses #516

This provides a mechanism for packages to provide information about how they were configured to downstream CMake projects.

Any cache vars set with tribits_add_option_and_define() are exported automatically but TriBITS packages can call tribits_pkg_export_cache_var(<cacheVarName>) to export any cache vars.

See updated documentation for details.

That code just clutters up the main macros() and makes them hard to reason
about and maintain.
…#516)

This started failing for some reason and I have no idea why. This refactored
command is better anyway.
This updates some existing tests for the export of selected package cache
vars, including vars from parent package in subpackage <Package>Config.cmake
files.
…iBITSPub#516)

This provides a mechanism for packages to provide information about how they
were configured to downstream CMake projects.

Any cache vars set with tribits_add_option_and_define() are exported
automatically but TriBITS packages can call
tribits_pkg_export_cache_var(<cacheVarName>) to export any cache vars.

See updated documentation for details.
@bartlettroscoe bartlettroscoe force-pushed the 516-export-pkg-info-vars branch from 45c038f to ea526c9 Compare August 23, 2022 01:19
@bartlettroscoe bartlettroscoe changed the title WIP: Export package info vars into <Package>Config.cmake files (#516) Export package info vars into <Package>Config.cmake files (#516) Aug 23, 2022
@bartlettroscoe bartlettroscoe changed the title Export package info vars into <Package>Config.cmake files (#516) Add support for exporting package info vars into <Package>Config.cmake files (#516) Aug 23, 2022
@bartlettroscoe
Copy link
Member Author

Hello @KyleFromKitware, can you do a quick review? If not, I can merge and we can set this up for a post-merge review when you have the time.

Added macro tribits_assert_cache_and_local_vars_same_value(<cacheVarName>)
(with strong unit tests) to ensure that if any exported cache vars also has
local vars of the same name that they also have the same value.

This was done in response to review of PR TriBITSPub#520 from @KyleFromKitware.
@bartlettroscoe
Copy link
Member Author

bartlettroscoe commented Aug 23, 2022

So asserting these exported cache vars has exposed one error in Trilinos CMakeLists.txt files that will need to be fixed in the Pliris package. Details are shown below

Testing against Trilinos and identification of defect with Pliris_ENABLE_DREAL: (click to expand)

NOTE: I tested this with the Trilinos clang-10.0.0 PR build for Trilinos 'develop' version:

$ cd Trilinos/

$ gitdist-status --dist-repos=.,TriBITS
-----------------------------------------------------------------------------------------------------
| ID | Repo Dir        | Branch                   | Tracking Branch                     | C | M | ? |
|----|-----------------|--------------------------|-------------------------------------|---|---|---|
|  0 | Trilinos (Base) | develop                  | github/develop                      |   |   |   |
|  1 | TriBITS         | 516-export-pkg-info-vars | rab-github/516-export-pkg-info-vars |   |   |   |
-----------------------------------------------------------------------------------------------------

$ gitdist-repo-versions --dist-repos=.,TriBITS
*** Base Git Repo: Trilinos
f0ba7236d6c0e36c3e7f7fbe0cd66e9ec26798a2 [Mon Aug 22 15:14:05 2022 -0600] <[email protected]>
Merge Pull Request #10914 from trilinos/Trilinos/stk-snapshot
*** Git Repo: TriBITS
903b9a1cd298d87b52550dd6ef420f75e9e3502e [Tue Aug 23 08:42:45 2022 -0600] <[email protected]>
Assert exported cache and local vars have same value (#516)

I ran:

$ ssh crf450

$ cd ~/Trilinos.base/BUILDS/PR/clang-10.0.0/

$ cat load-env.sh
export TRILINOS_DIR=${HOME}/Trilinos.base/Trilinos
source ${TRILINOS_SRC}/packages/framework/GenConfig/gen-config.sh \
--cmake-fragment GenConfigSettings.cmake \
rhel7_sems-clang-10.0.0-openmpi-1.10.1-serial_release-debug_shared_no-kokkos-arch_no-asan_no-complex_no-fpic_mpi_no-pt_no-rdc_no-uvm_deprecated-on_no-package-enables \
--force -y \
"$@" \
${TRILINOS_DIR}

$ cat do-configure
rm -rf CMakeCache.txt CMakeFiles
cmake \
-G Ninja \
-C GenConfigSettings.cmake \
-D Trilinos_TRIBITS_DIR:STRING=TriBITS/tribits \
-D Trilinos_ENABLE_ALL_FORWARD_DEP_PACKAGES=OFF \
-D Trilinos_ENABLE_TESTS=ON \
"$@" \
${TRILINOS_DIR}

$ . load-env.sh

$ time ./do-configure -DTrilinos_ENABLE_ALL_PACKAGES=ON &> configure.out

real    0m41.081s
user    0m35.373s
sys     0m11.629s

$ tail configure.out 

Finished configuring Trilinos!

Total time to configure Trilinos: 0m38.652s
-- If publishing results using Trilinos, please cite us: https://trilinos.github.io/cite.html
-- Configuring incomplete, errors occurred!
See also "/ascldap/users/rabartl/Trilinos.base/BUILDS/PR/clang-10.0.0/CMakeFiles/CMakeOutput.log".
See also "/ascldap/users/rabartl/Trilinos.base/BUILDS/PR/clang-10.0.0/CMakeFiles/CMakeError.log".

This showed the errors:

$ grep -A 2 ERROR: configure.out 
  ERROR: The cache variable Pliris_ENABLE_DREAL with the cache var value
  'OFF' is not the same value as the local variable Pliris_ENABLE_DREAL with
  value 'ON'!
--
  ERROR: The cache variable Pliris_ENABLE_DREAL with the cache var value
  'OFF' is not the same value as the local variable Pliris_ENABLE_DREAL with
  value 'ON'!

And reconfiguring with:

$ cmake . &> configure.2.out

showed the same errors.

A search of that package shows:

$ cd packages/pliris/

$ find . -name CMakeLists.txt -exec grep -nH _ENABLE_DREAL {} \;
./CMakeLists.txt:23:TRIBITS_ADD_OPTION_AND_DEFINE(${PACKAGE_NAME}_ENABLE_DREAL
./CMakeLists.txt:41:   NOT ${PACKAGE_NAME}_ENABLE_DREAL)
./CMakeLists.txt:43:  SET(${PACKAGE_NAME}_ENABLE_DREAL ON)
./test/CMakeLists.txt:1:IF(Pliris_ENABLE_DREAL)
./src/CMakeLists.txt:118:  IF(Pliris_ENABLE_DREAL)
./src/CMakeLists.txt:170:ELSEIF(Pliris_ENABLE_DREAL)

and you can see the error on line 43 :-(

Copy link
Collaborator

@rabartlett1972 rabartlett1972 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KyleFromKitware signed off on the fix in #520 (comment).

@bartlettroscoe bartlettroscoe merged commit c85490f into TriBITSPub:master Aug 23, 2022
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this pull request Aug 24, 2022
…g, refactor (TriBITSPub#516)

In PR TriBITSPub#520, I accidentally put a requirement that the macro var name had to be
set.  Turns out, there is special logic for it to not be set.  The
documentation nor any TriBITS test hinted to this.  But the file
muelu/CMakeLists.txt actually calls tribits_add_option_and_define() with an
empty macro var name.

To catch this use case, I added unit tests (which will fail if you undo this
patch).

In order for the unit tests to cover what is needed, I had to factor out the
function tribits_pkg_export_cache_var() into its own module
TribitsPkgExportCacheVars.cmake so it could be included in the unit test
driver.  Then, I decided it would be a good idea to factor out a function
tribits_pkg_append_set_commands_for_exported_vars() that is called in the
module TribitsWriteClientExportFiles.cmake.  Lastly, I moved the function
tribits_assert_cache_and_local_vars_same_value() into this module as well.
This encapsulates all of the logic to export of these package cache vars into
a single module (with just a few dependencies).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants