Design and implement strategy for deprecating TriBITS code

[bartlettroscoe]

Parent Epic: #411

Description

As noted in trilinos/Trilinos#9894 (comment), we need to consider how to handle deprecated functions and macros in TriBITS. I don't think it is desirable to just use raw message(DEPRECATION <msg>) since that might generate thousands of lines of output if the deprecated function/macro is used in a loop. But it would be nice to have a system where the user can decide to show deprecated code or not. Also, it would be good to allow the separation of deprecation warnings for TriBITS from deprecations for other CMake modules and packages and even the user's own project functions.

A setup similar to what is described in Regulated Backward Compatibility and Deprecated Code would be good but apply it to TriBITS CMake code itself with respect to projects that use TriBITS.

What would likely be good would be to define and use a function like:

  tribits_deprecated(<msg>)

which would be called in any deprecated TriBITS function or macro. By default, this would call:

  message(DEPRECATION <msg>)

but when TRIBITS_HIDE_DEPRECATED_TRIBITS_WARNING_MESSAGES=TRUE, then no deprecated warnings would be printed at all.

[e10harvey]

I think having a way to enable/disable deprecation warning or error messages is sufficient. I would suggest adding one function for this purpose:

tribits_deprecated(msg):
  if CMAKE_ERROR_DEPRECATED
    message(FATAL_ERROR, msg)
  else if CMAKE_WARN_DEPRECATED
    message(WARN, msg)
  else
    return

[bartlettroscoe]

@e10harvey, here is the idea demonstrated with the CMake script deprecated_msg.cmake:

function(tribits_deprecated msg)
  if (NOT TRIBITS_HIDE_DEPRECATED_TRIBITS_WARNING_MESSAGES)
    message(DEPRECATION "${msg}")
  endif()
endfunction()

# Example deprecated TriBITS function

if (NOT TRIBITS_HIDE_TRIBITS_DEPRECATED_CODE)

function(tribits_deprecated_function)
  tribits_deprecated("TriBITS function '${CMAKE_CURRENT_FUNCTION}()' is deprecated!")
endfunction()

endif() # TRIBITS_HIDE_TRIBITS_DEPRECATED_CODE

# User code

function(project_function)

  tribits_deprecated_function()

  message(DEPRECATION "Project function '${CMAKE_CURRENT_FUNCTION}()' is deprecated!")

endfunction()

project_function()

Here is the output and error return codes you get for different use cases (using CMake 3.17.1):

Default (show deprecated warnings but do not trigger fail)

$ cmake -P deprecated_msg.cmake 
CMake Deprecation Warning at deprecated_msg.cmake:3 (message):
  TriBITS function 'tribits_deprecated_function()' is deprecated!
Call Stack (most recent call first):
  deprecated_msg.cmake:12 (tribits_deprecated)
  deprecated_msg.cmake:21 (tribits_deprecated_function)
  deprecated_msg.cmake:27 (project_function)


CMake Deprecation Warning at deprecated_msg.cmake:23 (message):
  Project function 'project_function()' is deprecated!
Call Stack (most recent call first):
  deprecated_msg.cmake:27 (project_function)



$ echo $?
0

Hide all deprecated warnings in all CMake code (in TriBITS and the project's own CMake code):

$ cmake -DCMAKE_WARN_DEPRECATED=FALSE -P deprecated_msg.cmake

$ echo $?
0

Only hide deprecated code in TriBITS but show deprecated code in project:

$ cmake -DTRIBITS_HIDE_DEPRECATED_TRIBITS_WARNING_MESSAGES=TRUE -P deprecated_msg.cmake
CMake Deprecation Warning at deprecated_msg.cmake:23 (message):
  Project function 'project_function()' is deprecated!
Call Stack (most recent call first):
  deprecated_msg.cmake:27 (project_function)

$ echo $?
0

Elevate deprecated warnings to errors (in TriBITS and project code)

$ cmake -DCMAKE_ERROR_DEPRECATED=TRUE -P deprecated_msg.cmake
CMake Deprecation Error at deprecated_msg.cmake:3 (message):
  TriBITS function 'tribits_deprecated_function()' is deprecated!
Call Stack (most recent call first):
  deprecated_msg.cmake:12 (tribits_deprecated)
  deprecated_msg.cmake:21 (tribits_deprecated_function)
  deprecated_msg.cmake:27 (project_function)

$ echo $?
1

Only show and elevate deprecated warnings to errors in project code:

 cmake -DTRIBITS_HIDE_DEPRECATED_TRIBITS_WARNING_MESSAGES=TRUE -DCMAKE_ERROR_DEPRECATED=TRUE -P deprecated_msg.cmake
CMake Deprecation Error at deprecated_msg.cmake:23 (message):
  Project function 'project_function()' is deprecated!
Call Stack (most recent call first):
  deprecated_msg.cmake:27 (project_function)

$ echo $?
1

Validate that project is not using any deprecated TriBITS function for sure:

$ cmake -DTRIBITS_HIDE_TRIBITS_DEPRECATED_CODE=TRUE -P deprecated_msg.cmake
CMake Error at deprecated_msg.cmake:21 (tribits_deprecated_function):
  Unknown CMake command "tribits_deprecated_function".
Call Stack (most recent call first):
  deprecated_msg.cmake:27 (project_function)

That latter has the advantage over just printing a deprecated error message in that it validates that the deprecated code is actually gone.

Note that another purpose of:

if (NOT TRIBITS_HIDE_TRIBITS_DEPRECATED_CODE)

# Deprecated code ...

endif() # TRIBITS_HIDE_TRIBITS_DEPRECATED_CODE

is that it allows you to write a tool like unifdef but for CMake code.

[bartlettroscoe]

@KyleFromKitware, would you be willing to look into implementing this for TriBITS? I know we are very underspent on the contract for help with TriBITS and I could use help with stuff like this (beyond just reviews).

[KyleFromKitware]

Perhaps it would be best to have a single variable called TRIBITS_HANDLE_TRIBITS_DEPRECATED_CODE=<IGNORE|WARN|ERROR>. If it's IGNORE then all deprecated code works without warning. If it's WARN (default) then all deprecated code works with a warning. If it's ERROR then all deprecated code exits with FATAL_ERROR as soon as it's called. For example:

function(tribits_deprecated function_name)
  if("${TRIBITS_HANDLE_TRIBITS_DEPRECATED_CODE}" STREQUAL "" OR "${TRIBITS_HANDLE_TRIBITS_DEPRECATED_CODE}" STREQUAL "WARN")
    message(DEPRECATION "${function_name} is deprecated")
  elseif("${TRIBITS_HANDLE_TRIBITS_DEPRECATED_CODE}" STREQUAL "ERROR")
    message(FATAL_ERROR "${function_name} is deprecated")
  elseif(NOT "${TRIBITS_HANDLE_TRIBITS_DEPRECATED_CODE}" STREQUAL "IGNORE")
    message(FATAL_ERROR "Invalid value for TRIBITS_HANDLE_TRIBITS_DEPRECATED_CODE: \"${TRIBITS_HANDLE_TRIBITS_DEPRECATED_CODE}\"")
  endif()
endfunction()

# ...

function(tribits_some_deprecated_function)
  tribits_deprecated(tribits_some_deprecated_function)

  # Do some deprecated stuff...
endfunction()

[bartlettroscoe]

@KyleFromKitware, I have found that it is really useful to support both SEND_ERROR and FATAL_ERROR. When developing on a CMake project where everything is clear, it is good to set checking to FATAL_ERROR so that new errors are easier to see. But when you first turn checks on for an existing CMake project with lots of potential violations, you would typically like to use SEND_ERROR so that you can see all of the violations in a single CMake configure, but developers can't just ignore them. (I have found that many Trilinos developers and users just ignore CMake warnings.)

[bartlettroscoe]

@KyleFromKitware, also, when deprecating code, it is often useful to provide an informative error message that tells the developer/user how to fix their code or at least where to look for that documentation (e.g. in the CHANGELOG.md file).

[bartlettroscoe]

@KyleFromKitware, with the basic support for the new tribits_deprecated[_command]() functions implemented in PR #538, would you be willing to apply this some of the existing TriBITS code that is deprecated? Here are some places where I think these new functions could be used:

tribits/core/package_arch/TribitsAddExecutable.cmake:378:      " is deprecated.  Instead, pass them through 'TARGET_DEFINES'.  The 'DEFINES'"
tribits/core/package_arch/TribitsAddExecutable.cmake:661:        " is deprecated!  Instead, please pass through TESTONLYLIBS instead!"
tribits/core/package_arch/TribitsAddExecutable.cmake:662:        "  DEPLIBS is deprecated!")
tribits/core/package_arch/TribitsAddExecutable.cmake:666:      " is deprecated!  The library '${depLib}' appears to be a"
tribits/core/package_arch/TribitsAddExecutable.cmake:677:        " DEPLIBS is deprecated!  Instead, pass through IMPORTEDLIBS!"
tribits/core/package_arch/TribitsAddExecutable.cmake:678:        "  DEPLIBS is deprecated!"

tribits/core/package_arch/TribitsAddLibrary.cmake:633:# ToDo: Turn the below deprecated WARNING messages to FATAL_ERROR once we
tribits/core/package_arch/TribitsAddLibrary.cmake:675:        "  Such usage is deprecated (and this warning will soon become an error)!"
tribits/core/package_arch/TribitsAddLibrary.cmake:681:      # ToDo: Turn the above to FATAL_ERROR after dropping deprecated code
tribits/core/package_arch/TribitsAddLibrary.cmake:685:        " cmake project!  Such usage is deprecated (and"
tribits/core/package_arch/TribitsAddLibrary.cmake:695:        " a lib defined in the current cmake project!  Such usage is deprecated (and"
tribits/core/package_arch/TribitsAddLibrary.cmake:718:# ToDo: Turn the below deprecated WARNING messages to FATAL_ERROR once we
tribits/core/package_arch/TribitsAddLibrary.cmake:730:        "  Such usage is deprecated (and this warning will soon become an error)!"
tribits/core/package_arch/TribitsAddLibrary.cmake:732:      # ToDo: Turn the above to FATAL_ERROR after dropping deprecated code

tribits/core/package_arch/TribitsIncludeDirectories.cmake:89:# Deprecated.  Use tribits_include_directories() instead!
tribits/core/package_arch/TribitsIncludeDirectories.cmake:105:#    message(WARNING "Warning: the override include_directories() is deprecated,"

tribits/core/package_arch/TribitsTplDeclareLibraries.cmake:44:    "WARNING: tribits_tpl_declare_libraries() is deprecated, instead use"

tribits/core/package_arch/TribitsAddExecutableAndTest.cmake:212:  tribits_fwd_parse_arg(CALL_ARGS DEPLIBS)  # Deprecated

tribits/core/package_arch/TribitsGeneralMacros.cmake:341:    "WARNING: tribits_set_ss_for_dev_mode() is deprecated,"

tribits/core/package_arch/TribitsTestCategories.cmake:100:      message_wrapper(WARNING "Warning: The test category 'WEEKLY' is deprecated"

tribits/core/package_arch/TribitsSetUpEnabledOnlyDependencies.cmake:70:      " is deprecated!  Please instead set"

tribits/core/package_arch/TribitsListHelpers.cmake:123:  message(WARNING "package_disable_on_platforms() is deprecated!"
tribits/core/package_arch/TribitsListHelpers.cmake:139:      message("-- " "WARNING: ${THING_TYPE} ${THING_NAME} TESTGROUP 'PS' is deprecated."
tribits/core/package_arch/TribitsListHelpers.cmake:145:      message("-- " "WARNING: ${THING_TYPE} ${THING_NAME} TESTGROUP 'SS' is deprecated."
tribits/core/package_arch/TribitsListHelpers.cmake:151:      message("-- " "WARNING: ${THING_TYPE} ${THING_NAME} TESTGROUP 'TS' is deprecated."

tribits/core/utils/ParseVariableArguments.cmake:45:macro(parse_arguments_deprecated_warning)
tribits/core/utils/ParseVariableArguments.cmake:46:  message(DEPRECATION "parse_arguments() is deprecated and should not be used."
tribits/core/utils/ParseVariableArguments.cmake:50:parse_arguments_deprecated_warning()
tribits/core/utils/ParseVariableArguments.cmake:205:  parse_arguments_deprecated_warning()

tribits/core/utils/AppendStringVar.cmake:58:# **DEPRECATED**: Instead, use::

[bartlettroscoe]

With the merge of #538 and #539, I think this is mostly complete. I will move this into review and will wait to close this until after I have been able to update the TriBITS version in Trilinos and see what happens.

[bartlettroscoe]

This issue is needs some more work:

• Update the deprecation message to mention the cache var TRIBITS_HANDLE_TRIBITS_DEPRECATED_CODE and how to set it to remove deprecation warnings (or turn them into errors).
• Add the non-cache var TRIBITS_HANDLE_TRIBITS_DEPRECATED_CODE_DEFAULT so that a TriBITS project can provide its own default for TRIBITS_HANDLE_TRIBITS_DEPRECATED_CODE.
• Add documentation for the cache var TRIBITS_HANDLE_TRIBITS_DEPRECATED_CODE in the TriBITS build reference and developers guides.
• Add a TriBITS script to replace a token with another token (which can be used for refactorings like changing include_directories to tribits_include_directories)
• Add a CHANGELOG.md entry for deprecation warnings and show to deal with them.

[bartlettroscoe]

The fact that this needs some work was highlighted I went to update TriBITS in Trilinos and it produced over 1,300 deprecation warnings as described in trilinos/Trilinos#11380 (comment).

[bartlettroscoe]

This has been implemented for a long time. No sense keeping it open anymore.