From ceeea4e511d658b249615e5c98231f070aaedeb9 Mon Sep 17 00:00:00 2001 From: Craig Scott Date: Sun, 18 Aug 2024 22:38:02 +1000 Subject: cmake_parse_arguments: Set variable if empty string given after keyword If a single-value keyword is followed by an empty string, the command unsets the variable for that keyword instead of setting it to the empty string. This is inconsistent and unexpected. Add policy CMP0174 which ensures the variable for a single-value keyword is always set when any value is given, not just for a non-empty value. The new CMP0174 policy only affects the PARSE_ARGV form of cmake_parse_arguments. The older form silently drops all empty string arguments before processing the argument list. Fixes: #25972 --- Help/command/cmake_parse_arguments.rst | 6 +++ Help/manual/cmake-policies.7.rst | 1 + Help/policy/CMP0174.rst | 33 ++++++++++++++++ Source/cmParseArgumentsCommand.cxx | 32 ++++++++++++--- Source/cmPolicies.h | 6 ++- .../CornerCasesArgvN-stderr.txt | 11 ++++++ .../cmake_parse_arguments/CornerCasesArgvN.cmake | 45 ++++++++++++++++++++++ .../cmake_parse_arguments/test_utils.cmake | 4 +- 8 files changed, 131 insertions(+), 7 deletions(-) create mode 100644 Help/policy/CMP0174.rst create mode 100644 Tests/RunCMake/cmake_parse_arguments/CornerCasesArgvN-stderr.txt diff --git a/Help/command/cmake_parse_arguments.rst b/Help/command/cmake_parse_arguments.rst index 0bb1d91..70dbeeb 100644 --- a/Help/command/cmake_parse_arguments.rst +++ b/Help/command/cmake_parse_arguments.rst @@ -70,6 +70,12 @@ whether your macro was called with unrecognized parameters. received values. This can be checked to see if there were keywords without any values given. +.. versionchanged:: 3.31 + If a ```` is followed by an empty string as its value, + policy :policy:`CMP0174` controls whether a corresponding + ``_`` variable is defined or not. + + Consider the following example macro, ``my_install()``, which takes similar arguments to the real :command:`install` command: diff --git a/Help/manual/cmake-policies.7.rst b/Help/manual/cmake-policies.7.rst index 4130e14..e257c36 100644 --- a/Help/manual/cmake-policies.7.rst +++ b/Help/manual/cmake-policies.7.rst @@ -57,6 +57,7 @@ Policies Introduced by CMake 3.31 .. toctree:: :maxdepth: 1 + CMP0174: cmake_parse_arguments(PARSE_ARGV) defines a variable for an empty string after a single-value keyword. CMP0173: The CMakeFindFrameworks module is removed. CMP0172: The CPack module enables per-machine installation by default in the CPack WIX Generator. CMP0171: 'codegen' is a reserved target name. diff --git a/Help/policy/CMP0174.rst b/Help/policy/CMP0174.rst new file mode 100644 index 0000000..bde0a03 --- /dev/null +++ b/Help/policy/CMP0174.rst @@ -0,0 +1,33 @@ +CMP0174 +------- + +.. versionadded:: 3.31 + +:command:`cmake_parse_arguments(PARSE_ARGV)` defines a variable for an empty +string after a single-value keyword. + +One of the main reasons for using the ``PARSE_ARGV`` form of the +:command:`cmake_parse_arguments` command is to more robustly handle corner +cases related to empty values. The non-``PARSE_ARGV`` form doesn't preserve +empty arguments, but the ``PARSE_ARGV`` form does. For each single-value +keyword given, a variable should be defined if the keyword is followed by a +value. Prior to CMake 3.31, no variable would be defined if the value given +was an empty string. This meant the code could not detect the difference +between the keyword not being given, and it being given but with an empty +value, except by iterating over all the arguments and checking if the keyword +is present. + +For the ``OLD`` behavior of this policy, +:command:`cmake_parse_arguments(PARSE_ARGV)` does not define a variable for a +single-value keyword followed by an empty string. +For the ``NEW`` behavior, :command:`cmake_parse_arguments(PARSE_ARGV)` always +defines a variable for each keyword given in the arguments, even a single-value +keyword with an empty string as its value. With the ``NEW`` behavior, the +code can robustly check if a single-value keyword was given with any value +using just ``if(DEFINED _)``. + +.. |INTRODUCED_IN_CMAKE_VERSION| replace:: 3.31 +.. |WARNS_OR_DOES_NOT_WARN| replace:: warns +.. include:: STANDARD_ADVICE.txt + +.. include:: DEPRECATED.txt diff --git a/Source/cmParseArgumentsCommand.cxx b/Source/cmParseArgumentsCommand.cxx index 9b1e76b..f6e7937 100644 --- a/Source/cmParseArgumentsCommand.cxx +++ b/Source/cmParseArgumentsCommand.cxx @@ -6,6 +6,7 @@ #include #include +#include #include #include "cmArgumentParser.h" @@ -14,6 +15,7 @@ #include "cmList.h" #include "cmMakefile.h" #include "cmMessageType.h" +#include "cmPolicies.h" #include "cmRange.h" #include "cmStringAlgorithms.h" #include "cmSystemTools.h" @@ -41,7 +43,7 @@ std::string JoinList(std::vector const& arg, bool escape) } using options_map = std::map; -using single_map = std::map; +using single_map = std::map>; using multi_map = std::map>>; using options_set = std::set; @@ -82,12 +84,32 @@ static void PassParsedArguments( iter.second ? "TRUE" : "FALSE"); } + const cmPolicies::PolicyStatus cmp0174 = + makefile.GetPolicyStatus(cmPolicies::CMP0174); for (auto const& iter : singleValArgs) { - if (!iter.second.empty()) { - makefile.AddDefinition(cmStrCat(prefix, iter.first), iter.second); - } else { - makefile.RemoveDefinition(prefix + iter.first); + if (iter.second.has_value()) { + // So far, we only know that the keyword was given, not whether a value + // was provided after the keyword + if (keywordsMissingValues.find(iter.first) == + keywordsMissingValues.end()) { + // A (possibly empty) value was given + if (cmp0174 == cmPolicies::NEW || !iter.second->empty()) { + makefile.AddDefinition(cmStrCat(prefix, iter.first), *iter.second); + continue; + } + if (cmp0174 == cmPolicies::WARN) { + makefile.IssueMessage( + MessageType::AUTHOR_WARNING, + cmStrCat("An empty string was given as the value after the ", + iter.first, + " keyword. Policy CMP0174 is not set, so " + "cmake_parse_arguments() will unset the ", + prefix, iter.first, + " variable rather than setting it to an empty string.")); + } + } } + makefile.RemoveDefinition(prefix + iter.first); } for (auto const& iter : multiValArgs) { diff --git a/Source/cmPolicies.h b/Source/cmPolicies.h index 2c3763f..7b056ae 100644 --- a/Source/cmPolicies.h +++ b/Source/cmPolicies.h @@ -533,7 +533,11 @@ class cmMakefile; "the CPack WIX Generator.", \ 3, 31, 0, cmPolicies::WARN) \ SELECT(POLICY, CMP0173, "The CMakeFindFrameworks module is removed.", 3, \ - 31, 0, cmPolicies::WARN) + 31, 0, cmPolicies::WARN) \ + SELECT(POLICY, CMP0174, \ + "cmake_parse_arguments(PARSE_ARGV) defines a variable for an empty " \ + "string after a single-value keyword.", \ + 3, 31, 0, cmPolicies::WARN) #define CM_SELECT_ID(F, A1, A2, A3, A4, A5, A6) F(A1) #define CM_FOR_EACH_POLICY_ID(POLICY) \ diff --git a/Tests/RunCMake/cmake_parse_arguments/CornerCasesArgvN-stderr.txt b/Tests/RunCMake/cmake_parse_arguments/CornerCasesArgvN-stderr.txt new file mode 100644 index 0000000..1b87306 --- /dev/null +++ b/Tests/RunCMake/cmake_parse_arguments/CornerCasesArgvN-stderr.txt @@ -0,0 +1,11 @@ +Testing CMP0174 = NEW +Testing CMP0174 = OLD +Testing CMP0174 = WARN +CMake Warning \(dev\) at CornerCasesArgvN\.cmake:[0-9]+ \(cmake_parse_arguments\): + An empty string was given as the value after the P1 keyword\. Policy + CMP0174 is not set, so cmake_parse_arguments\(\) will unset the arg_P1 + variable rather than setting it to an empty string\. +Call Stack \(most recent call first\): + CornerCasesArgvN\.cmake:[0-9]+ \(test_cmp0174_warn\) + CMakeLists\.txt:[0-9]+ \(include\) +This warning is for project developers\. Use -Wno-dev to suppress it\. diff --git a/Tests/RunCMake/cmake_parse_arguments/CornerCasesArgvN.cmake b/Tests/RunCMake/cmake_parse_arguments/CornerCasesArgvN.cmake index 807ed03..04c0219 100644 --- a/Tests/RunCMake/cmake_parse_arguments/CornerCasesArgvN.cmake +++ b/Tests/RunCMake/cmake_parse_arguments/CornerCasesArgvN.cmake @@ -51,3 +51,48 @@ function(test4) TEST(upref_UNPARSED_ARGUMENTS foo "bar;none") endfunction() test4(MULTI foo bar\\ none) + +# Single-value keyword with empty string as value +message(NOTICE "Testing CMP0174 = NEW") +block(SCOPE_FOR POLICIES) + cmake_policy(SET CMP0174 NEW) + function(test_cmp0174_new_missing) + cmake_parse_arguments(PARSE_ARGV 0 arg "" "P1;P2" "") + TEST(arg_KEYWORDS_MISSING_VALUES "P2") + TEST(arg_P1 "") + TEST(arg_P2 "UNDEFINED") + endfunction() + test_cmp0174_new_missing(P1 "" P2) + + function(test_cmp0174_new_no_missing) + cmake_parse_arguments(PARSE_ARGV 0 arg "" "P1;P2" "") + TEST(arg_KEYWORDS_MISSING_VALUES "UNDEFINED") + TEST(arg_P1 "") + TEST(arg_P2 "UNDEFINED") + endfunction() + test_cmp0174_new_no_missing(P1 "") +endblock() + +message(NOTICE "Testing CMP0174 = OLD") +block(SCOPE_FOR POLICIES) + cmake_policy(SET CMP0174 OLD) + function(test_cmp0174_old) + cmake_parse_arguments(PARSE_ARGV 0 arg "" "P1;P2" "") + TEST(arg_KEYWORDS_MISSING_VALUES "P2") + TEST(arg_P1 "UNDEFINED") + TEST(arg_P2 "UNDEFINED") + endfunction() + test_cmp0174_old(P1 "" P2) +endblock() + +message(NOTICE "Testing CMP0174 = WARN") +block(SCOPE_FOR POLICIES) + cmake_policy(VERSION 3.30) # WARN + function(test_cmp0174_warn) + cmake_parse_arguments(PARSE_ARGV 0 arg "" "P1;P2" "") + TEST(arg_KEYWORDS_MISSING_VALUES "P2") + TEST(arg_P1 "UNDEFINED") + TEST(arg_P2 "UNDEFINED") + endfunction() + test_cmp0174_warn(P1 "" P2) +endblock() diff --git a/Tests/RunCMake/cmake_parse_arguments/test_utils.cmake b/Tests/RunCMake/cmake_parse_arguments/test_utils.cmake index 9ce99b8..23b804e 100644 --- a/Tests/RunCMake/cmake_parse_arguments/test_utils.cmake +++ b/Tests/RunCMake/cmake_parse_arguments/test_utils.cmake @@ -13,6 +13,8 @@ function(TEST variable) if(DEFINED ${variable}) message(FATAL_ERROR "'${variable}' shall be undefined but has value '${${variable}}'") endif() + elseif(NOT DEFINED ${variable}) + message(FATAL_ERROR "'${variable}' should be defined but is not") elseif("${expected}" STREQUAL "FALSE") if(NOT ${variable} STREQUAL "FALSE") message(FATAL_ERROR "'${variable}' shall be FALSE") @@ -23,7 +25,7 @@ function(TEST variable) endif() else() if(NOT ${variable} STREQUAL "${expected}") - message(FATAL_ERROR "'${variable}' shall be '${expected}'") + message(FATAL_ERROR "'${variable}' shall be '${expected}' but has value '${${variable}}'") endif() endif() endif() -- cgit v0.12