Tribits: fix variadic assert_defined()

[brian-kelley]

@trilinos/tribits

Motivation

assert_defined() allows you to pass any number of variable names to check, but in practice it only checks the first one (see #15114). We use it with multiple arguments in a lot of Trilinos packages. This PR changes it to use the special ARGV to capture all the arguments.

@bartlettroscoe This is probably not be the right place to make permanent TriBITS changes; should I backport to https://github.com/TriBITSPub/TriBITS ?

Testing

Tested locally. With this change,

#SET(mything ON)
SET(mything2 OFF)
assert_defined(mything mything2)

and

SET(mything ON)
#SET(mything2 OFF)
assert_defined(mything mything2)

each print that mything or mything2 are not defined, respectively. Before, the second case would silently pass.

Update: I didn't notice this TODO at the bottom of the file:

# ToDo: The VARS arg This really needs to be replaced with ${ARGV}.  I fear
# that only the first arg passed in is asserted.  However, to change this now
# is breaking backward compatibility.

Is the backward compatibility referring to Trilinos itself here? If it's intended to support only one argument, we should check that $ARGC == 0 to enforce that, and might as well get rid of this foreach.

[github-actions]

CDash for AT2 results [Currently only accessible from Sandia networks]

[jhux2]

Update: I didn't notice this TODO at the bottom of the file:

# ToDo: The VARS arg This really needs to be replaced with ${ARGV}.  I fear
# that only the first arg passed in is asserted.  However, to change this now
# is breaking backward compatibility.

Is the backward compatibility referring to Trilinos itself here? If it's intended to support only one argument, we should check that $ARGC == 0 to enforce that, and might as well get rid of this foreach.

Since developers have been using this macro all over the place under the assumption that it supports multiple arguments, I don't see how backwards compatibility (at least for Trilinos) is relevant.

[EDIT]

Meaning, I think @brian-kelley's fix is the correct one.

[bartlettroscoe]

Since developers have been using this macro all over the place under the assumption that it supports multiple arguments, I don't see how backwards compatibility (at least for Trilinos) is relevant.

See all of the failures this triggers:

• https://sems-cdash-son.sandia.gov/cdash/index.php?project=Trilinos&filtercount=1&showfilters=1&field1=buildname&compare1=63&value1=15132

That is what was meant by "backward compatibility".

That macro should have asserted to ensure that only one var name is passed in. (CMake does not do that automatically.)

But I guess this is easy to fix on the user's side. If you are getting errors after this change, you can always just remove the additional vars being passed in.

@bartlettroscoe This is probably not be the right place to make permanent TriBITS changes; should I backport to TriBITSPub/TriBITS ?

It should likely be done on the TriBITS side since we should add some unit tests to pin down the behavior. I would also like to add a backward compatibility mode (e.g. TRIBITS_ASSERT_DEFINED_IGNORE_MULTIPLE_ARGS=TRUE) for this that projects can turn on if they upgrade TriBITS and get hit by this, so they can do the upgrade and then fix their CMakeLists.txt files later.

Sound reasonable?

[jhux2]

Since developers have been using this macro all over the place under the assumption that it supports multiple arguments, I don't see how backwards compatibility (at least for Trilinos) is relevant.

See all of the failures this triggers:

* https://sems-cdash-son.sandia.gov/cdash/index.php?project=Trilinos&filtercount=1&showfilters=1&field1=buildname&compare1=63&value1=15132

That is what was meant by "backward compatibility".

Aren't those errors due to incorrect use of assert_defined in cmake/tribits/core/package_arch/TribitsPackageSetupCompilerFlags.cmake?

[EDIT]

But I see your point that this would require more extensive changes in Tribits itself.

[jhux2]

`assert_defined` usage in tribits

common_tpls/FindTPLADIOS2.cmake:61:   assert_defined(TPL_ADIOS2_INCLUDE_DIRS)
common_tpls/FindTPLNetcdf.cmake:112:     assert_defined(TPL_Netcdf_INCLUDE_DIRS)
core/utils/AssertDefined.cmake:11:# @FUNCTION: assert_defined()
core/utils/AssertDefined.cmake:18:#   assert_defined(<varName>)
core/utils/AssertDefined.cmake:34:#   assert_defined(SOME_VARBLE)
core/utils/AssertDefined.cmake:46:function(assert_defined VARS)
core/utils/AppendGlobalSet.cmake:27:  assert_defined(${VARNAME})
core/utils/PrintNonemptyVar.cmake:25:  assert_defined(VARIABLE_NAME)
core/utils/PrependGlobalSet.cmake:27:  assert_defined(${VARNAME})
core/utils/PrintVarWithSpaces.cmake:32:  assert_defined(VARIBLE_NAME)
core/utils/RemoveGlobalDuplicates.cmake:32:  assert_defined(${VARNAME})
core/test_support/TribitsAddTestHelpers.cmake:307:  assert_defined(${PROJECT_NAME}_TEST_CATEGORIES)
core/package_arch/TribitsCreateClientTemplateHeaders.cmake:91:  assert_defined(HAVE_${PARENT_PACKAGE_NAME_UC}_EXPLICIT_INSTANTIATION)
core/package_arch/TribitsTplFindIncludeDirsAndLibraries.cmake:288:  assert_defined(TPL_ENABLE_${TPL_NAME})
core/package_arch/TribitsInternalPackageWriteConfigFile.cmake:523:        assert_defined(${depPkg}_CONFIG)
core/package_arch/TribitsReadDepsFilesCreateDepsGraph.cmake:343:  tribits_assert_defined_package_var(LIB_REQUIRED_DEP_PACKAGES ${PACKAGE_NAME})
core/package_arch/TribitsReadDepsFilesCreateDepsGraph.cmake:344:  tribits_assert_defined_package_var(LIB_OPTIONAL_DEP_PACKAGES ${PACKAGE_NAME})
core/package_arch/TribitsReadDepsFilesCreateDepsGraph.cmake:345:  tribits_assert_defined_package_var(TEST_REQUIRED_DEP_PACKAGES ${PACKAGE_NAME})
core/package_arch/TribitsReadDepsFilesCreateDepsGraph.cmake:346:  tribits_assert_defined_package_var(TEST_OPTIONAL_DEP_PACKAGES ${PACKAGE_NAME})
core/package_arch/TribitsReadDepsFilesCreateDepsGraph.cmake:348:  tribits_assert_defined_package_var(LIB_REQUIRED_DEP_TPLS ${PACKAGE_NAME})
core/package_arch/TribitsReadDepsFilesCreateDepsGraph.cmake:349:  tribits_assert_defined_package_var(LIB_OPTIONAL_DEP_TPLS ${PACKAGE_NAME})
core/package_arch/TribitsReadDepsFilesCreateDepsGraph.cmake:350:  tribits_assert_defined_package_var(TEST_REQUIRED_DEP_TPLS ${PACKAGE_NAME})
core/package_arch/TribitsReadDepsFilesCreateDepsGraph.cmake:351:  tribits_assert_defined_package_var(TEST_OPTIONAL_DEP_TPLS ${PACKAGE_NAME})
core/package_arch/TribitsReadDepsFilesCreateDepsGraph.cmake:957:function(tribits_assert_defined_package_var  PACKAGE_VAR  PACKAGE_NAME)
core/package_arch/TribitsProcessTplsLists.cmake:84:  assert_defined(REPOSITORY_NAME)
core/package_arch/TribitsProcessTplsLists.cmake:236:      assert_defined(${REPOSITORY_NAME}_TPLS_FILE)
core/package_arch/TribitsProcessExtraRepositoriesList.cmake:129:  assert_defined(PROJECT_NAME)
core/package_arch/TribitsProcessExtraRepositoriesList.cmake:250:  assert_defined(${PROJECT_NAME}_EXTRAREPOS_DIR_VCTYPE_REPOURL_PACKSTAT_CATEGORY)
core/package_arch/TribitsProcessExtraRepositoriesList.cmake:581:      #assert_defined(${PROJECT_NAME}_ENABLE_KNOWN_EXTERNAL_REPOS_TYPE)
core/package_arch/TribitsProcessExtraRepositoriesList.cmake:605:      assert_defined(PROJECT_SOURCE_DIR)
core/package_arch/TribitsGlobalMacros.cmake:838:  assert_defined(${PROJECT_NAME}_EXTRA_EXTERNAL_REPOS_FILE_NAME)
core/package_arch/TribitsGlobalMacros.cmake:1037:      assert_defined(CMAKE_INSTALL_PREFIX)
core/package_arch/TribitsGlobalMacros.cmake:1038:      assert_defined(${PROJECT_NAME}_INSTALL_LIB_DIR)
core/package_arch/TribitsGlobalMacros.cmake:1138:  assert_defined(${PROJECT_NAME}_PRE_REPOSITORIES)
core/package_arch/TribitsGlobalMacros.cmake:1139:  assert_defined(${PROJECT_NAME}_NATIVE_REPOSITORIES)
core/package_arch/TribitsGlobalMacros.cmake:1140:  assert_defined(${PROJECT_NAME}_EXTRA_REPOSITORIES)
core/package_arch/TribitsGlobalMacros.cmake:1430:  assert_defined(${PROJECT_NAME}_ENABLE_C)
core/package_arch/TribitsGlobalMacros.cmake:1440:  assert_defined(${PROJECT_NAME}_ENABLE_CXX)
core/package_arch/TribitsGlobalMacros.cmake:1450:  assert_defined(${PROJECT_NAME}_ENABLE_Fortran)
core/package_arch/TribitsSetupBasicCompileLinkFlags.cmake:135:  assert_defined(${PROJECT_NAME}_ENABLE_C CMAKE_C_COMPILER_ID)
core/package_arch/TribitsSetupBasicCompileLinkFlags.cmake:148:  assert_defined(${PROJECT_NAME}_ENABLE_CXX CMAKE_CXX_COMPILER_ID)
core/package_arch/TribitsSetupBasicCompileLinkFlags.cmake:161:  assert_defined(${PROJECT_NAME}_ENABLE_Fortran)
core/package_arch/TribitsSetupBasicCompileLinkFlags.cmake:174:  assert_defined(${PROJECT_NAME}_ENABLE_COVERAGE_TESTING COVERAGE_OPTIONS)
core/package_arch/TribitsSetupStrongCompileWarnings.cmake:49:  assert_defined(${PROJECT_NAME}_ENABLE_C CMAKE_C_COMPILER_ID)
core/package_arch/TribitsSetupStrongCompileWarnings.cmake:61:  assert_defined(${PROJECT_NAME}_ENABLE_CXX CMAKE_CXX_COMPILER_ID)
core/package_arch/TribitsSetupStrongCompileWarnings.cmake:73:  assert_defined(${PROJECT_NAME}_ENABLE_Fortran)
core/package_arch/TribitsAddExecutable.cmake:371:  assert_defined(${PROJECT_NAME}_LINK_SEARCH_START_STATIC)
core/package_arch/TribitsPackageSetupCompilerFlags.cmake:56:  assert_defined(${PROJECT_NAME}_ENABLE_C CMAKE_C_COMPILER_ID)
core/package_arch/TribitsPackageSetupCompilerFlags.cmake:62:  assert_defined(${PROJECT_NAME}_ENABLE_CXX CMAKE_CXX_COMPILER_ID)
core/package_arch/TribitsPackageSetupCompilerFlags.cmake:68:  assert_defined(${PROJECT_NAME}_ENABLE_Fortran)
core/package_arch/TribitsPackageSetupCompilerFlags.cmake:122:  assert_defined(PARSE_CLEANED)
core/package_arch/TribitsPackageSetupCompilerFlags.cmake:124:  assert_defined(${PROJECT_NAME}_ENABLE_C ${PROJECT_NAME}_ENABLE_C_DEBUG_COMPILE_FLAGS)
core/package_arch/TribitsPackageSetupCompilerFlags.cmake:129:  assert_defined(${PROJECT_NAME}_ENABLE_CXX ${PROJECT_NAME}_ENABLE_CXX_DEBUG_COMPILE_FLAGS)
core/package_arch/TribitsPackageMacros.cmake:423:    assert_defined(${packageEnableVarName})
core/package_arch/TribitsPackageMacros.cmake:424:    assert_defined(${havePackageUpstreamPackageMacroVarName})
core/package_arch/TribitsProcessPackagesAndDirsLists.cmake:108:  assert_defined(REPOSITORY_NAME)
core/package_arch/TribitsProcessPackagesAndDirsLists.cmake:276:  assert_defined(${PACKAGE_NAME_IN}_PARENT_REPOSITORY)
core/package_arch/TribitsProcessPackagesAndDirsLists.cmake:388:  assert_defined(${REPOSITORY_NAME}_PACKAGES_AND_DIRS_AND_CLASSIFICATIONS)
core/package_arch/TribitsAdjustPackageEnables.cmake:727:  assert_defined(${PROJECT_NAME}_ENABLE_${packageName})
ctest_driver/TribitsCTestDriverCore.cmake:1820:  assert_defined(${PROJECT_NAME}_ENABLE_DEVELOPMENT_MODE)
ctest_driver/TribitsCTestDriverCoreHelpers.cmake:424:    assert_defined(${PROJECT_NAME}_ENABLE_${TRIBITS_PACKAGE})
doc/guides/UtilsMacroFunctionDocTemplate.rst:16:@FUNCTION: assert_defined() +
doc/guides/TribitsCMakeLanguageOverviewAndGotchas.rst:173:`assert_defined()`_ as::
doc/guides/TribitsCMakeLanguageOverviewAndGotchas.rst:175:   assert_defined(SOME_VARBLE)
examples/TribitsExampleProject/cmake/CallbackDefineRepositoryPackaging.cmake:3:  assert_defined(${REPOSITORY_NAME}_SOURCE_DIR)
examples/TribitsExampleProject/cmake/CallbackSetupExtraOptions.cmake:3:  assert_defined(${PROJECT_NAME}_ENABLE_INSTALL_CMAKE_CONFIG_FILES)
examples/TribitsExampleProject/cmake/CallbackSetupExtraOptions.cmake:25:  assert_defined(${PROJECT_NAME}_ENABLE_Fortran)
examples/TribitsExampleProject/packages/with_subpackages/c/tests/CMakeLists.txt:37:assert_defined(WithSubpackagesB_ENABLE_MixedLang)
examples/TribitsExampleProject/packages/with_subpackages/b/tests/CMakeLists.txt:46:assert_defined(${PACKAGE_NAME}_ENABLE_MixedLang)
examples/TribitsExampleProject/packages/with_subpackages/b/tests/testlib/CMakeLists.txt:10:assert_defined(${PACKAGE_NAME}_ENABLE_MixedLang)

[cgcgcg]

@bartlettroscoe I am not sure I understand the backward compatibility angle. What behavior do users of the macro expect? It seems that at the moment the first variable is checked and all others are ignored. Is there a use case where that is actually desired behavior?

[brian-kelley]

I can only imagine the intent with lines like

assert_defined(${PROJECT_NAME}_ENABLE_C CMAKE_C_COMPILER_ID)

was at some point to check multiple variables, which would mean that undefined vars here possibly indicate a real issue. If we were to go enforce getting only 1 argument, and went through and rewrote these calls to check 1 variable at a time, there would still be the same errors, no?

[bartlettroscoe]

If we were to go enforce getting only 1 argument, and went through and rewrote these calls to check 1 variable at a time, there would still be the same errors, no?

@brian-kelley, yes, any change will break backward compatibility. My preference would be to change to assert a list of vars (as this PR does) by default but to allow projects an easy upgrade by setting TRIBITS_ASSERT_DEFINED_IGNORE_MULTIPLE_ARGS=TRUE and put that in the error message. That option would then be deprecated to be removed at some future release.

[bartlettroscoe]

@bartlettroscoe I am not sure I understand the backward compatibility angle. What behavior do users of the macro expect? It seems that at the moment the first variable is checked and all others are ignored. Is there a use case where that is actually desired behavior?

I can't honestly remember. If I had to guess, at some point I forgot that it only asserts the first arg and tried to use more than one arg, like the case @brian-kelley mentions above. I think I wrote the first version of this macro back in 2008 when I was just learning CMake and did not understand the quarks of the language.

There is a good bit to do:

• EPIC: Refactoring TriBITS Core and TriBITS-based CMake projects to conform to modern CMake TriBITSPub/TriBITS#411

I best that modern AI coding agents and frontier models with the right Agent Skills could automate a lot of this work.

[bartlettroscoe]

I am perfectly happy to make this change in the TriBITS repo (with tests) and snapshot to Trilinos develop and fix the errors in Trilinos.