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.

Sign up for GitHub

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

Parse CLI parameters and YAML files #508

Merged
merged 3 commits into from
Sep 25, 2019
Merged

Parse CLI parameters and YAML files #508

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8e56c51
Enable incremental parameter yaml file parsing.
hidmic Sep 20, 2019
74b03b8
Parse CLI parameter files and overrides in rcl.
hidmic Sep 20, 2019
c1f4c3c
Address peer review comments.
hidmic Sep 24, 2019
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
9 changes: 6 additions & 3 deletions rcl/include/rcl/arguments.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,8 +283,11 @@ rcl_arguments_get_param_files(
rcl_allocator_t allocator,
char *** parameter_files);

/// Return all parameter overrides specified on the command line.
/// Return all parameter overrides parsed from the command line.
/**
* Parameter overrides are parsed directly from command line arguments and
* parameter files provided in the command line.
*
* <hr>
* Attribute | Adherence
* ------------------ | -------------
Expand All @@ -294,8 +297,8 @@ rcl_arguments_get_param_files(
* Lock-Free | Yes
*
* \param[in] arguments An arguments structure that has been parsed.
* \param[out] parameter_overrides Zero or more parameter overrides.
* This structure must be finalized by the caller.
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is still a valid sentence to leave in the documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good catch! Re-added it in c1f4c3c

* \param[out] parameter_overrides Parameter overrides as parsed from command line arguments.
* The output is NULL if no parameter overrides were parsed.
* \return `RCL_RET_OK` if everything goes correctly, or
* \return `RCL_RET_INVALID_ARGUMENT` if any function arguments are invalid, or
* \return `RCL_RET_BAD_ALLOC` if allocating memory failed, or
Expand Down
1 change: 1 addition & 0 deletions rcl/package.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
<test_depend>ament_cmake_pytest</test_depend>
<test_depend>ament_lint_auto</test_depend>
<test_depend>ament_lint_common</test_depend>
<test_depend>rcpputils</test_depend>
<test_depend>rmw</test_depend>
<test_depend>rmw_implementation_cmake</test_depend>
<test_depend>launch</test_depend>
Expand Down
39 changes: 32 additions & 7 deletions rcl/src/rcl/arguments.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,13 +124,17 @@ rcl_arguments_get_param_overrides(
RCL_CHECK_ARGUMENT_FOR_NULL(arguments, RCL_RET_INVALID_ARGUMENT);
RCL_CHECK_ARGUMENT_FOR_NULL(arguments->impl, RCL_RET_INVALID_ARGUMENT);
RCL_CHECK_ARGUMENT_FOR_NULL(parameter_overrides, RCL_RET_INVALID_ARGUMENT);

if (NULL != *parameter_overrides) {
RCL_SET_ERROR_MSG("Output parameter override pointer is not null. May leak memory.");
return RCL_RET_INVALID_ARGUMENT;
}
*parameter_overrides = rcl_yaml_node_struct_copy(arguments->impl->parameter_overrides);
if (NULL == *parameter_overrides) {
return RCL_RET_BAD_ALLOC;
*parameter_overrides = NULL;
if (NULL != arguments->impl->parameter_overrides) {
*parameter_overrides = rcl_yaml_node_struct_copy(arguments->impl->parameter_overrides);
if (NULL == *parameter_overrides) {
return RCL_RET_BAD_ALLOC;
}
}
return RCL_RET_OK;
}
Expand Down Expand Up @@ -218,6 +222,7 @@ rcl_ret_t
_rcl_parse_param_file(
const char * arg,
rcl_allocator_t allocator,
rcl_params_t * params,
char ** param_file);

/// Parse an argument that may or may not be a parameter file rule.
Expand All @@ -237,6 +242,7 @@ rcl_ret_t
_rcl_parse_param_file_rule(
const char * arg,
rcl_allocator_t allocator,
rcl_params_t * params,
char ** param_file);

#define RCL_ENABLE_FLAG_PREFIX "--enable-"
Expand Down Expand Up @@ -443,11 +449,11 @@ rcl_parse_arguments(
// Attempt to parse argument as parameter file rule
if (strcmp(RCL_PARAM_FILE_FLAG, argv[i]) == 0) {
if (i + 1 < argc) {
// Attempt to parse next argument as remap rule
// Attempt to parse next argument as parameter file rule
args_impl->parameter_files[args_impl->num_param_files_args] = NULL;
if (
RCL_RET_OK == _rcl_parse_param_file(
argv[i + 1], allocator,
argv[i + 1], allocator, args_impl->parameter_overrides,
&args_impl->parameter_files[args_impl->num_param_files_args]))
{
++(args_impl->num_param_files_args);
Expand Down Expand Up @@ -606,7 +612,8 @@ rcl_parse_arguments(
args_impl->parameter_files[args_impl->num_param_files_args] = NULL;
if (
RCL_RET_OK == _rcl_parse_param_file_rule(
argv[i], allocator, &(args_impl->parameter_files[args_impl->num_param_files_args])))
argv[i], allocator, args_impl->parameter_overrides,
&args_impl->parameter_files[args_impl->num_param_files_args]))
{
++(args_impl->num_param_files_args);
RCUTILS_LOG_WARN_NAMED(ROS_PACKAGE_NAME,
Expand Down Expand Up @@ -743,6 +750,12 @@ rcl_parse_arguments(
}
}

// Drop parameter overrides if none was found.
ivanpauno marked this conversation as resolved.
Show resolved Hide resolved
if (0U == args_impl->parameter_overrides->num_nodes) {
rcl_yaml_node_struct_fini(args_impl->parameter_overrides);
args_impl->parameter_overrides = NULL;
}

// Shrink unparsed_ros_args
if (0 == args_impl->num_unparsed_ros_args) {
// No unparsed ros args
Expand Down Expand Up @@ -1811,26 +1824,34 @@ rcl_ret_t
_rcl_parse_param_file(
const char * arg,
rcl_allocator_t allocator,
rcl_params_t * params,
char ** param_file)
{
RCL_CHECK_ARGUMENT_FOR_NULL(arg, RCL_RET_INVALID_ARGUMENT);
RCL_CHECK_ARGUMENT_FOR_NULL(params, RCL_RET_INVALID_ARGUMENT);
RCL_CHECK_ARGUMENT_FOR_NULL(param_file, RCL_RET_INVALID_ARGUMENT);

*param_file = rcutils_strdup(arg, allocator);
if (NULL == *param_file) {
RCL_SET_ERROR_MSG("Failed to allocate memory for parameters file path");
return RCL_RET_BAD_ALLOC;
}
if (!rcl_parse_yaml_file(*param_file, params)) {
// Error message already set.
return RCL_RET_ERROR;
}
return RCL_RET_OK;
}

rcl_ret_t
_rcl_parse_param_file_rule(
const char * arg,
rcl_allocator_t allocator,
rcl_params_t * params,
char ** param_file)
{
RCL_CHECK_ARGUMENT_FOR_NULL(arg, RCL_RET_INVALID_ARGUMENT);
RCL_CHECK_ARGUMENT_FOR_NULL(params, RCL_RET_INVALID_ARGUMENT);
RCL_CHECK_ARGUMENT_FOR_NULL(param_file, RCL_RET_INVALID_ARGUMENT);

const size_t param_prefix_len = strlen(RCL_PARAM_FILE_ARG_RULE);
if (strncmp(RCL_PARAM_FILE_ARG_RULE, arg, param_prefix_len) == 0) {
Expand All @@ -1841,6 +1862,10 @@ _rcl_parse_param_file_rule(
return RCL_RET_BAD_ALLOC;
}
snprintf(*param_file, outlen + 1, "%s", arg + param_prefix_len);
if (!rcl_parse_yaml_file(*param_file, params)) {
// Error message already set.
return RCL_RET_ERROR;
}
return RCL_RET_OK;
}
RCL_SET_ERROR_MSG("Argument does not start with '" RCL_PARAM_FILE_ARG_RULE "'");
Expand Down
1 change: 1 addition & 0 deletions rcl/src/rcl/arguments_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ typedef struct rcl_arguments_impl_t

/// Parameter override rules parsed from arguments.
rcl_params_t * parameter_overrides;

ivanpauno marked this conversation as resolved.
Show resolved Hide resolved
/// Array of yaml parameter file paths
char ** parameter_files;
/// Length of parameter_files.
Expand Down
3 changes: 2 additions & 1 deletion rcl/test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ find_package(launch_testing_ament_cmake REQUIRED)

find_package(test_msgs REQUIRED)

find_package(rcpputils REQUIRED)
find_package(rmw_implementation_cmake REQUIRED)

find_package(osrf_testing_tools_cpp REQUIRED)
Expand Down Expand Up @@ -148,7 +149,7 @@ function(test_target_function)
ENV ${rmw_implementation_env_var}
APPEND_LIBRARY_DIRS ${extra_lib_dirs}
LIBRARIES ${PROJECT_NAME} osrf_testing_tools_cpp::memory_tools
AMENT_DEPENDENCIES "osrf_testing_tools_cpp"
AMENT_DEPENDENCIES "osrf_testing_tools_cpp" "rcpputils"
)

rcl_add_custom_gtest(test_remap${target_suffix}
Expand Down
12 changes: 9 additions & 3 deletions rcl/test/cmake/rcl_add_custom_gtest.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,16 @@ set(rcl_add_custom_gtest_INCLUDED TRUE)
# :type LIBRARIES: list of strings
# :param AMENT_DEPENDENCIES: list of depends to pass ament_target_dependencies
# :type AMENT_DEPENDENCIES: list of strings
#
# :param WORKING_DIRECTORY: the working directory for invoking the
# executable in, default defined by ``ament_add_test()``
# :type WORKING_DIRECTORY: string

# @public
#
macro(rcl_add_custom_gtest target)
cmake_parse_arguments(_ARG
"SKIP_TEST;TRACE"
"TIMEOUT"
"TIMEOUT;WORKING_DIRECTORY"
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't look like this new parameter is being used anywhere. Do we really need this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A leftover from a previous attempt. I rolled back all the changes to this file in c1f4c3c

"SRCS;ENV;APPEND_ENV;APPEND_LIBRARY_DIRS;INCLUDE_DIRS;LIBRARIES;AMENT_DEPENDENCIES"
${ARGN})
if(_ARG_UNPARSED_ARGUMENTS)
Expand All @@ -72,10 +75,13 @@ macro(rcl_add_custom_gtest target)
if(_ARG_TIMEOUT)
set(_ARG_TIMEOUT "TIMEOUT" ${_ARG_TIMEOUT})
endif()
if(_ARG_WORKING_DIRECTORY)
set(_ARG_WORKING_DIRECTORY "WORKING_DIRECTORY" ${_ARG_WORKING_DIRECTORY})
endif()

# Pass args along to ament_add_gtest().
ament_add_gtest(${target} ${_ARG_SRCS} ${_ARG_ENV} ${_ARG_APPEND_ENV} ${_ARG_APPEND_LIBRARY_DIRS}
${_ARG_SKIP_TEST} ${_ARG_TIMEOUT})
${_ARG_SKIP_TEST} ${_ARG_TIMEOUT} ${_ARG_WORKING_DIRECTORY})
# Check if the target was actually created.
if(TARGET ${target})
if(_ARG_TRACE)
Expand Down
Loading