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

Selective application of launch-prefix from CLI #257

Closed
camm73 opened this issue Jul 19, 2021 · 2 comments · Fixed by #261
Closed

Selective application of launch-prefix from CLI #257

camm73 opened this issue Jul 19, 2021 · 2 comments · Fixed by #261
Assignees
Labels
enhancement New feature or request

Comments

@camm73
Copy link
Contributor

camm73 commented Jul 19, 2021

Feature request

Feature description

#254 adds the --launch-prefix flag to provide a means of prepending the same prefix (e.g. xterm -e valgrind) to each executable command in the launch file. For large launch files this launches many instances of the desired prefix program even if the user may only require it for certain executables. Currently selectively applying prefixes can be achieved by editing the launch file and directly specifying the applied prefix for a given executable; however, this is a slow process if the prefix is frequently changed or the developer only wishes to use a prefix one time.

To provide a means of filtering which executables the --launch-prefix value is applied to, I propose that we add a --launch-prefix-filter flag (or something to this effect) which allows the user to provide a list of executable names for which the prefix should be applied. When used alone --launch-prefix would apply the prefix to all executables, and when used in conjunction with the new --launch-prefix-filter flag, the prefix would only be applied to matching executables.

The goal of these two flags is to provide a faster means of debugging multiple, specific executables without requiring the launch file to be edited.

Implementation considerations

For the launch_ros repo:

  • Add --launch-prefix-filter argument which accepts at least 1 executable name
  • Pass list of executable names as a / separated list via service.context.launch_configurations (this is because Unix & Windows filenames cannot contain /)

For the launch repo (Assuming #254 provides the prefix via service.context.launch_configurations):

  • Modify ExecuteProcess.__expand_substitutions() such that when a launch prefix filter is available in the launch configuration, the prefix is only prepended to the command if its executable name matches an executable name that was provided via the launch configuration

The name property within ExecuteProcess is purposely not used so that the behavior is the same between Node and ExecuteProcess. Node sets the ExecuteProcess name to the executable name by default, but when the user creates their own ExecuteProcess in a launch file, they are able to provide their own name, overriding it from being set to the executable name. This could cause an issue where user-named ExecuteProcess elements wouldn't be properly selected by the filter, so it's executable name should be extracted directly from the cmd property instead.

@hidmic
Copy link
Contributor

hidmic commented Aug 6, 2021

Pass list of executable names as a / separated list via service.context.launch_configurations (this is because Unix & Windows filenames cannot contain /)

I like the idea but this bit is unusual. How about using regex instead?

@camm73
Copy link
Contributor Author

camm73 commented Aug 9, 2021

I like the idea but this bit is unusual. How about using regex instead?

Yeah I agree—that seems like a cleaner implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants