Open wjwwood opened 1 year ago
Looks like this breaks some tests, so I need to see why this might not be a safe change to make.
I don't think this is right place/approach for this fix, and I think these issues are related:
But in the case of some_exec flag1:=flag1Value
the argument parser of the application would need to handle the splitting. I don't actually think we want launch splitting flag1
and flag1Value
apart.
The pull request only support whitespace delimiters because that's what shell's support.
Either way a test for the := would be nice.
What would the test look like? Just something with :=
in it?
But in the case of
some_exec flag1:=flag1Value
the argument parser of the application would need to handle the splitting. I don't actually think we want launch splittingflag1
andflag1Value
apart.The pull request only support whitespace delimiters because that's what shell's support.
Either way a test for the := would be nice.
What would the test look like? Just something with
:=
in it?
Ah right got it nevermind, shell doesn't need to worry about it. Looks good to me !
I tested this with shlex.split() and it seems like it splits it up instead of pre-expanding the command.
My understanding is that any of our substitutions should be processed before the changes in this pr get to it, or if you want to use bash-like things like $(cmd args ...)
then you need to use shell=True
and then this change has no effect.
And yes, there are cases where the user will just need to quote things to avoid them being split.
Also, I'll mention that the argument parsing that happens in xml/yaml is still very fragile, and I would love to improve the robustness of that, for example something like the xml <executable cmd="python3 -c "print('$(var some_launch_config)')"" ... />
will break the parser because it tries to shlex.split()
the string python3 -c "print('
and then crashes because of unmatched quotes. But this is a pre-existing issue that this pr neither made better or worse. However, I didn't have time to fully track down a solution to that. It's a tricky problem for sure.
Similarly, we might want to set comments=True on shlex.
I actually considered that, but I thought that might interfere with arguments that contain #
, but then they could just be quoted and/or escaped, so maybe it would be good to do that, but on the other hand, why include comments in the command like that? I mean I guess it might be from user input or something, but there really shouldn't be comments in the cmd and if there are, then for them to be process, you should use shell=True
. I don't think it's the responsibility/scope of the split_arguments
option/idea to handle everything a shell might do.
So I'm kind of :-1: on turning on comments=True
for shlex.split()
, but I think I could be convinced otherwise.
Looks like the shlex.split()
may be breaking something on Windows with the \
path separators.
I tested this with shlex.split() and it seems like it splits it up instead of pre-expanding the command.
My understanding is that any of our substitutions should be processed before the changes in this pr get to it, or if you want to use bash-like things like
$(cmd args ...)
then you need to useshell=True
and then this change has no effect.And yes, there are cases where the user will just need to quote things to avoid them being split.
Also, I'll mention that the argument parsing that happens in xml/yaml is still very fragile, and I would love to improve the robustness of that, for example something like the xml
<executable cmd="python3 -c "print('$(var some_launch_config)')"" ... />
will break the parser because it tries toshlex.split()
the stringpython3 -c "print('
and then crashes because of unmatched quotes. But this is a pre-existing issue that this pr neither made better or worse. However, I didn't have time to fully track down a solution to that. It's a tricky problem for sure.Similarly, we might want to set comments=True on shlex.
I actually considered that, but I thought that might interfere with arguments that contain
#
, but then they could just be quoted and/or escaped, so maybe it would be good to do that, but on the other hand, why include comments in the command like that? I mean I guess it might be from user input or something, but there really shouldn't be comments in the cmd and if there are, then for them to be process, you should useshell=True
. I don't think it's the responsibility/scope of thesplit_arguments
option/idea to handle everything a shell might do.So I'm kind of 👎 on turning on
comments=True
forshlex.split()
, but I think I could be convinced otherwise.
No issues on both of those!
I was thinking comments in the command that's passed could be used as comments. But all contexts in which you'd conceivably use this would also have the ability to have comments, so there's no need for that.
Looking at the windows changes
Yeah, there's still an issue on Windows. I'm in the process of spinning up a workspace on my Windows VM to debug it.
We already do this for the prefix filter, and it should not break any existing uses, because of howshlex.split
works.Just blindly applying
shlex.split
will break things, due to complex combinations, so adding an argument to opt into the behavior, and make it possible to set it via launch configuration to support some cases where changing the executable entry in the launch file isn't convenient.This option was needed because the existing behavior sometimes prevents passing multiple command line arguments as a single launch configuration, because they would be grouped into a single
argv
entry in the main function of the program it was passed to whenshell=False
is used with subprocess.fixes https://github.com/ros2/launch/issues/710
A little more detail on the original problem and how this fixes it...
The tests added in this pr are good examples of the root problem and how this fixes it, but to summarize briefly here:
There's an
arg_echo.py
script I use in the tests which just prints the arguments received and exits with the count as the return code.Then consider this test case that was added:
You can see that without the use of
split_arguments=True
(in this case being set from aLaunchConfiguration
) the script interprets the last argument as'--some-arg "some string"'
as a single contiguous argument, which breaks some argument parsers, like the one used in gazebo classic for example. But if you enable the new feature, then it splits that argument into--some-arg
andsome string
, which is what we want.This is important because if you cannot use
shell=True
, which we cannot (see https://github.com/ros-simulation/gazebo_ros_pkgs/pull/1376), and you have to pass multiple arguments in a single substitution (e.g. https://github.com/ros-simulation/gazebo_ros_pkgs/blob/df368e60598aa82940bffc7b0db46420b04577a6/gazebo_ros/launch/gzserver.launch.py#L71) or if you need to pass arguments which contain whitespace in a single substitution (e.g. workedaround here https://github.com/ros-simulation/gazebo_ros_pkgs/pull/1502), then you need something like this.I also considered ways in which a substitution could return a list of substitutions instead of a string, but that was just too big of a change and conceptually had too many issues. I also considered a special syntax or substitution type that the
ExecuteLocal
action could handle in a specific way, but that also seemed clunky and after trying to implement it I couldn't get something satisfactory, and so I ended up with the current approach, which certainly isn't perfect.Also, if you want more context on the interactions between cmd as a sequence (list) vs cmd as a string vs cmd as a sequence containing a single string and settings like
shell=True
, the subprocess documentation in python is helpful: https://docs.python.org/3/library/subprocess.html#subprocess.Popen (search forshell=True
and read around those sections)Hopefully that's enough context, but please let me know if there are more questions.