fix build error when Boost CONFIG_MACRO is the last of the CMAKE_ARGS #1220
Conversation
Build error happen when CONFIG_MACRO is in the last position of CMAKE_ARGS
|
||
foreach(i RANGE ${CMAKE_ARGC}) | ||
foreach(i RANGE ${ARGC_COUNT}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it's:
- argv[0] = cmake
- argv[1] = -P
- argv[2] = append-boost-config-macros.cmake
So the range should be foreach(i RANGE 3 ${ARGC_COUNT})
.
Please add it as a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it's:
- argv[0] = cmake
- argv[1] = -P
- argv[2] = append-boost-config-macros.cmake
I'd already noted that but I had doubts that cmake behavior will keep consistent between differente versions or invocation modes. I know that args will be filtered out because there are not prefixed with CONFIG_MACRO.
I checked with a simple cmake scripts and it seems that cmake is very permissive with arguments parsing. For example, non cmake command line arguments before of the -P
are ignored and passed to the scripts:
$ cat script.cmake
math(EXPR argc ${CMAKE_ARGC}-1)
foreach(i RANGE ${argc})
message("ARG${i}: '${CMAKE_ARGV${i}}'")
endforeach()
$ cmake pre1 pre2 -P script.cmake post1 post2
ARG0: 'cmake'
ARG1: 'pre1'
ARG2: 'pre2'
ARG3: '-P'
ARG4: 'cmake-scripts-args.cmake'
ARG5: 'post1'
ARG6: 'post2'
So the range should be foreach(i RANGE 3 ${ARGC_COUNT}).
Please add it as a comment.
Currently this is true when the scripts is called by url_sha_boost* but I prefer a more general comment like:
# args without prefix CONFIG_MACRO are filtered out
# (including 'cmake' '-P' 'append-boost-config-macros.cmake')
Are you ok with that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, non cmake command line arguments before of the -P are ignored
Actually it's quite a question how it should work because from documentation synopsis is:
cmake [(-D <var>=<value>)...] -P <cmake-script-file>
Is it designed to pass arguments without -D
?
I see we are using -C
by the way:
hunter/cmake/projects/OpenSSL/schemes/url_sha1_openssl.cmake.in
Lines 155 to 160 in b818355
"@CMAKE_COMMAND@" "-C@HUNTER_ARGS_FILE@" # for 'HUNTER_INSTALL_LICENSE_FILES' "-Dsrcdir=@HUNTER_PACKAGE_SOURCE_DIR@" "-Ddstdir=@HUNTER_PACKAGE_LICENSE_DIR@" -P "@HUNTER_SELF@/scripts/try-copy-license.cmake"
Also I've filed the bug once:: https://gitlab.kitware.com/cmake/cmake/issues/16431 😄
args without prefix CONFIG_MACRO are filtered out
I don't think we should rely on that. Such tricks lead to non-trivial bugs.
As a summary I think it's better to refactor this into cmake -D<var>=<value> -P <cmake-script-file>
and don't use CMAKE_ARGC
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use foreach(i RANGE 3 ${ARGC_COUNT})
here.
@@ -2,6 +2,6 @@ hunter_config( | |||
Boost | |||
VERSION ${HUNTER_Boost_VERSION} | |||
CMAKE_ARGS | |||
CONFIG_MACRO=BOOST_REGEX_MATCH_EXTRA;BOOST_MPL_CFG_NO_PREPROCESSED_HEADERS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep the original example, it's still valid. Please add unit test for the script instead.
I guess the closest example will be this test with CMAKE_ARGS
-> "etalon" comparing:
You can run it similar to regular example with PROJECT_DIR=tests/<name-of-the-test>
.
|
||
foreach(i RANGE ${CMAKE_ARGC}) | ||
foreach(i RANGE ${ARGC_COUNT}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use foreach(i RANGE 3 ${ARGC_COUNT})
here.
As suggested, I am trying to refactor the script to no use CMAKE_ARGS but I think it is material for another pull request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests looks good, please update PACKAGE_INTERNAL_DEPS_ID
:
Usually I totally agree with as little updates as possible but Boost update involves a lot of testing so if you want to change |
Please confirm that I should wait for updates. |
Use a -DBOOST_CONFIG_MACROS instead. Parsing of custom args is move to a new helper function hunter_parse_boost_config_macros()
@@ -0,0 +1,24 @@ | |||
cmake_minimum_required(VERSION 3.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed, we will inherit policies from parent.
endif() | ||
endforeach() | ||
set(${outvar} ${boost_config_macros_list} PARENT_SCOPE) | ||
endfunction(hunter_parse_boost_config_macros) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use endfunction()
.
"@HUNTER_GLOBAL_SCRIPT_DIR@/append-boost-config-macros.cmake" | ||
"@HUNTER_Boost_CMAKE_ARGS@" | ||
"@CMAKE_COMMAND@" | ||
"-DBOOST_CONFIG_MACROS=${boost_config_macros_list}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
examples/Boost-custom-args
is not working.
This line converted to -DBOOST_CONFIG_MACROS=BOOST_1
BOOST_2
BOOST_3=5
(3 arguments, 2 ignored) instead of -DBOOST_CONFIG_MACROS=BOOST_1;BOOST_2;BOOST_3=5
(1 argument). It's better to use configure_file
to create script.
…ros.cmake Rename ‘append-boost-config-macros.cmake’ to ‘append-boost-config-macros.cmake.in’
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good. Will run the tests soon, want to combine testing with another change.
There was an error in this change, fixed. Retesting: |
Thanks!
|
No description provided.