Use functions instead of macros where possible to avoid leaking variables #835
Conversation
|
Bump? |
|
Sorry for not responding on this before. I currently have to focus on ROS 2 related tasks. Hopefully I will have time to look at this in the next weeks. |
|
Thanks for the update :) |
| endif() | ||
| math(EXPR _index "${_index} + 1") | ||
| endwhile() | ||
| set(${VAR} "${result}" PARENT_SCOPE) | ||
| debug_message(10 "catkin_filter_libraries_for_build_configuration(${VAR} ${ARG_UNPARSED_ARGUMENTS} BUILD_TYPE ${ARG_BUILD_TYPE}) ${${VAR}}") |
There was a problem hiding this comment.
Does ${${VAR}} work in this context? Does it get the updated value set in the parent scope in the line above?
There was a problem hiding this comment.
Indeed it doesn't see the updated value, good catch. Seems like the child scope starts out with a copy of the parent scope at the start of the function, rather than dynamically inheriting any variables not present locally.
| macro(catkin_unpack_libraries_with_build_configuration VAR) | ||
| set(${VAR} "") | ||
| function(catkin_unpack_libraries_with_build_configuration VAR) | ||
| set(result "") | ||
| foreach(lib ${ARGN}) | ||
| string(REGEX REPLACE "^(debug|optimized|general)${CATKIN_BUILD_CONFIGURATION_KEYWORD_SEPARATOR}(.+)$" "\\1;\\2" lib "${lib}") | ||
| list(APPEND ${VAR} "${lib}") |
There was a problem hiding this comment.
This should append to result.
|
Thanks for the review, I've addressed those two issues in the last commit. Let me know if you want me to squash everything into 1 commit. |
| endif() | ||
| math(EXPR _index "${_index} + 1") | ||
| endwhile() | ||
| set(${VAR} "${result}" PARENT_SCOPE) | ||
| #debug_message(10 "catkin_pack_libraries_with_build_configuration(${VAR} ${_argn}) ${${VAR}}") |
There was a problem hiding this comment.
Just to be safe if someone enables this line in the future can you also update it to use ${result} instead of ${${VAR}}.
| endif() | ||
| endforeach() | ||
| set(${VAR} "${result}" PARENT_SCOPE) | ||
| #set(_argn ${ARGN}) | ||
| #debug_message(10 "catkin_replace_imported_library_targets(${VAR} ${_argn}) ${${VAR}}") |
There was a problem hiding this comment.
Just to be safe if someone enables this line in the future can you also update it to use ${result} instead of ${${VAR}}.
|
Thanks for iterating on this. I added two more comments inline. If you can squash the commits afterwards that would be great. Otherwise I can take care of it during the merge. |
…al variables. The leaked 'cmd' variable in particular triggers a bug in cmake's ExternalProject_Add, see https://gitlab.kitware.com/cmake/cmake/merge_requests/264 Also replace other macros with functions where possible.
|
Should all be tidied up now. |
|
Thanks for the update. It looks good to me. I ran a prerelease test locally with 100+ packages to convince myself that the chance for regressions is minimal. Lets hope the next release builds "agree" with that 😉 @ros/ros_team Please review. |
My primary reason for this PR is the first commit that makes
list_insert_in_workspace_ordera function, as it leaks a variable calledcmdwhich triggers a bug inExternalProject_Add(the value of thecmdvariable becomes the default BUILD and INSTALL command for the external project, soorder_paths.pyjust gets run a couple more times instead of actually building anything).I filed a PR for CMake to fix this (https://gitlab.kitware.com/cmake/cmake/merge_requests/264), but I think it would be good to also fix this on the catkin side regardless. In general it seems to me using functions instead of macros wherever possible is desirable to avoid polluting the caller's variable namespace, but I'm by no means a CMake expert.
All tests still seem to pass (and my projects still build, and
ExternalProject_Addworks as expected)