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

Preserving forwarding of empty string arguments #461

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .cmake-format
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,5 @@ include: ["cmake/.cmake-format-additional_commands-cpm"]
format:
tab_size: 2
line_width: 100
line_ending: auto
dangle_parens: true
60 changes: 50 additions & 10 deletions cmake/CPM.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -505,17 +505,50 @@ function(cpm_override_fetchcontent contentName)
set_property(GLOBAL PROPERTY ${propertyName} TRUE)
endfunction()

macro(cpm_cmake_eval)
set(__ARGN "${ARGN}")
if(COMMAND cmake_language)
cmake_language(EVAL CODE "${__ARGN}")
else()
file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/eval.cmake "${__ARGN}")
include(${CMAKE_CURRENT_BINARY_DIR}/eval.cmake)
endif()
endmacro()

# Download and add a package from source
function(CPMAddPackage)
macro(CPMAddPackage)
set(__ARGN "${ARGN}")
list(LENGTH __ARGN __ARGN_Length)
if(__ARGN_Length EQUAL 1)
cpm_add_package_single_arg(${ARGN})
else()
# Forward preserving empty string arguments
# (https://gitlab.kitware.com/cmake/cmake/-/merge_requests/4729)
set(__ARGN_Quoted)
foreach(__ARG IN LISTS __ARGN)
string(APPEND __ARGN_Quoted " [==[${__ARG}]==]")
endforeach()
cpm_cmake_eval("cpm_add_package_multi_arg( ${__ARGN_Quoted} )")
endif()
endmacro()
Comment on lines +519 to +533
Copy link
Contributor

Choose a reason for hiding this comment

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

There is, however, a specific case to be aware of which can result in unexpected behavior. Because macros treat their arguments as string substitutions rather than as variables, if they use ARGN in a place where a variable name is expected, the variable it will refer to will be in the scope from which the macro is called, not the ARGN from the macro’s own arguments.

From chapter 9.2 professional-cmake


macro(cpm_add_package_single_arg arg)
cpm_set_policies()
cpm_parse_add_package_single_arg("${arg}" __ARGN_multi)

list(LENGTH ARGN argnLength)
if(argnLength EQUAL 1)
cpm_parse_add_package_single_arg("${ARGN}" ARGN)
# The shorthand syntax implies EXCLUDE_FROM_ALL
# cmake-format: off
list(APPEND __ARGN_multi
EXCLUDE_FROM_ALL YES
SYSTEM YES
)
# cmake-format: on

# The shorthand syntax implies EXCLUDE_FROM_ALL and SYSTEM
set(ARGN "${ARGN};EXCLUDE_FROM_ALL;YES;SYSTEM;YES;")
CraigHutchinson marked this conversation as resolved.
Show resolved Hide resolved
endif()
cpm_add_package_multi_arg(${__ARGN_multi}) # Forward function arguments to CPMAddPackage()
endmacro()

function(cpm_add_package_multi_arg)
cpm_set_policies()

set(oneValueArgs
NAME
Expand All @@ -538,7 +571,7 @@ function(CPMAddPackage)

set(multiValueArgs URL OPTIONS DOWNLOAD_COMMAND)

cmake_parse_arguments(CPM_ARGS "" "${oneValueArgs}" "${multiValueArgs}" "${ARGN}")
cmake_parse_arguments(PARSE_ARGV 0 CPM_ARGS "" "${oneValueArgs}" "${multiValueArgs}")

# Set default values for arguments

Expand Down Expand Up @@ -916,7 +949,13 @@ function(cpm_declare_fetch PACKAGE VERSION INFO)
return()
endif()

FetchContent_Declare(${PACKAGE} ${ARGN})
# Forward preserving empty string arguments
# (https://gitlab.kitware.com/cmake/cmake/-/merge_requests/4729)
set(__argsQuoted)
foreach(__item IN LISTS ARGN)
string(APPEND __argsQuoted " [==[${__item}]==]")
endforeach()
cpm_cmake_eval("FetchContent_Declare(${PACKAGE} ${__argsQuoted} )")
endfunction()

# returns properties for a package previously defined by cpm_declare_fetch
Expand Down Expand Up @@ -1102,8 +1141,9 @@ function(cpm_prettify_package_arguments OUT_VAR IS_IN_COMMENT)
SYSTEM
GIT_SHALLOW
)

set(multiValueArgs URL OPTIONS DOWNLOAD_COMMAND)
cmake_parse_arguments(CPM_ARGS "" "${oneValueArgs}" "${multiValueArgs}" ${ARGN})
cmake_parse_arguments(PARSE_ARGV 2 CPM_ARGS "" "${oneValueArgs}" "${multiValueArgs}")

foreach(oneArgName ${oneValueArgs})
if(DEFINED CPM_ARGS_${oneArgName})
Expand Down
2 changes: 1 addition & 1 deletion test/integration/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ To run all tests from the repo root execute:
$ ruby test/integration/runner.rb
```

The runner will run all tests and generate a report of the exeuction.
The runner will run all tests and generate a report of the execution.

The current working directory doesn't matter. If you are in `<repo-root>/test/integration`, you can run simply `$ ruby runner.rb`.

Expand Down
58 changes: 58 additions & 0 deletions test/unit/forward_arguments.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
cmake_minimum_required(VERSION 3.14 FATAL_ERROR)

include(${CPM_PATH}/CPM.cmake)
include(${CPM_PATH}/testing.cmake)

# Intercept underlying `FetchContent_Declare`
function(FetchContent_Declare)
set_property(GLOBAL PROPERTY last_FetchContent_Declare_ARGN "${ARGN}")
endfunction()
cpm_declare_fetch(PACKAGE VERSION INFO EMPTY "" ANOTHER)

# TEST:`cpm_declare_fetch` shall forward empty arguments
get_property(last_FetchContent_Declare_ARGN GLOBAL PROPERTY last_FetchContent_Declare_ARGN)
assert_equal("${last_FetchContent_Declare_ARGN}" "PACKAGE;EMPTY;;ANOTHER")

# TEST:`CPMDeclarePackage` shall store all including empty
CPMDeclarePackage(FOO EMPTY "" ANOTHER)
assert_equal("${CPM_DECLARATION_FOO}" "EMPTY;;ANOTHER")

# Stub the actual fetch
set(fibonacci_POPULATED YES)
set(fibonacci_SOURCE_DIR ".")
set(fibonacci_BINARY_DIR ".")
macro(FetchContent_GetProperties)

endmacro()

# TEST:`CPMAddPackage` shall call `FetchContent_declare` with unmodified arguments including any
# Empty-string arguments
CPMAddPackage(
NAME fibonacci
GIT_REPOSITORY https://github.com/cpm-cmake/testpack-fibonacci.git
VERSION 1.2.3 EMPTY_OPTION "" COMMAND_WITH_EMPTY_ARG foo "" bar
)
get_property(last_FetchContent_Declare_ARGN GLOBAL PROPERTY last_FetchContent_Declare_ARGN)
assert_equal(
"${last_FetchContent_Declare_ARGN}"
"fibonacci;EMPTY_OPTION;;COMMAND_WITH_EMPTY_ARG;foo;;bar;GIT_REPOSITORY;https://github.com/cpm-cmake/testpack-fibonacci.git;GIT_TAG;v1.2.3"
)

# Intercept underlying `cpm_add_package_multi_arg`
function(cpm_add_package_multi_arg)
set_property(GLOBAL PROPERTY last_cpm_add_package_multi_arg_ARGN "${ARGN}")
endfunction()

# TEST: CPM Module file shall store all arguments including empty strings
include(${CPM_MODULE_PATH}/Findfibonacci.cmake)
get_property(
last_cpm_add_package_multi_arg_ARGN GLOBAL PROPERTY last_cpm_add_package_multi_arg_ARGN
)
assert_equal(
"${last_cpm_add_package_multi_arg_ARGN}"
"NAME;fibonacci;GIT_REPOSITORY;https://github.com/cpm-cmake/testpack-fibonacci.git;VERSION;1.2.3;EMPTY_OPTION;;COMMAND_WITH_EMPTY_ARG;foo;;bar"
)

# remove generated files
file(REMOVE_RECURSE ${CPM_MODULE_PATH})
file(REMOVE ${CPM_PACKAGE_LOCK_FILE})