Can one of the admins verify this patch?

I like the general idea, but I'm hesitant about the use of eval(), as it could encourage the embedding of arbitrary Python code as strings into what is otherwise an XML file. That's very hard to test and would allow users to introduce nasty bugs (plus code injection security holes, if we're worried about that).

I'm planning to do something sort of similar, by adding boolean prefix operators in a manner that's similar to the existing substitution syntax, e.g.:

$(and $(arg foo) $(or $(arg bar) $(arg bat)))

It wouldn't get the completely general (in the sense of using Python arbitrarily) comparisons that you have here, but would it handle the use cases you have in mind? Maybe we could also add non-boolean operators, like $(eq val1 val2).

@gerkey You might want to look at #382.

@gerkey Your comment is very valid. We took some counter measures in xacro to forbid execution of arbitrary python code. I added the same measures here too.
We aware, that there are several usages of eval() all over in ros_comm! Somebody should protect all of them, when you agreed on a common method.

Regarding your alternative syntax: I'm always hesitant to reinvent the wheel. Python already does a great job in evaluating expressions and we should use it. Reimplementing that functionality will be error-prone and incomplete.

@rhaschke That's interesting; I didn't know about the new Python eval support in xacro (ros/xacro#54). What you're proposing here isn't any worse or more dangerous than that.

I added on top of @rhaschke's work in ubi-agni#1.

@gerkey Thanks for extending this work with lots of tests. I can understand your caution regarding python evaluation, thus introducing the pyeval attribute sounds reasonable.
I didn't have issues with True and False not being defined in the dict. Maybe that's a python 2 vs 3 issue? Do you use python 3?
I rebased this PR onto latest indigo-devel and subsequently added your changes on top of it.

@rhaschke Regarding the inclusion of True and False, here's what I see with Python 2.7.6:

$ python
Python 2.7.6 (default, Jun 22 2015, 17:58:13) 
[GCC 4.8.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> eval('True', dict(), None)
True
>>> eval('True', dict(__builtins__={}), None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<string>", line 1, in <module>
NameError: name 'True' is not defined
>>> eval('True', dict(__builtins__={}, True=True), None)
True

If __builtins__={}, then I have to explicitly add back True and False, I guess because they're builtins?

Oh, I see; you must be on python3, which doesn't need the explicit inclusion:

$ python3
Python 3.4.0 (default, Jun 19 2015, 14:20:21) 
[GCC 4.8.2] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> eval('True', dict(__builtins__={}), None)
True

...and python3 won't accept the explicit inclusion:

>>> eval('True', dict(__builtins__={}, True=True), None)
  File "<stdin>", line 1
SyntaxError: keyword can't be an expression

So I guess that we need to condition the contents of _eval_dict based on python version; can you make that change?

Btw, in addition to being cautious with regard to Python evaluation, it occurred to me that without the ability to disable it, you wouldn't be able to assign certain string values to arguments, e.g.:

<arg name="mycrazystring" value="1 == 1"/>

That's a bad example, but there are likely some useful argument values that people want to use that happen to be legal Python expressions.

Good example. In xacro, evaluation was explicitly triggered by ${} syntax.
Hence we need something similar here too. For me, the pyeval syntax is somewhat clumsy. What about reusing xacro's ${} syntax?

Regarding the True/False issue: I use python 2.7 too. However, I tested for
eval('1==1', dict(__builtins__={}), None) only, which worked.
If found a version-agnostic way to define True and False in _eval_dict and committed that change (94df268).

Do you think that we could extend this feature to encompass the functionality being sought in #382?

If users could somehow refer to $(find ...) and $(arg ...) values from within the Python expression that's being evaluated, then they could combine them in whatever manner they like, with the full power of Python, rather than relying on roslaunch to correctly parse nested expressions.

Edit: e.g., we could insert in the __builtins__ functions like find() and arg() that call back into roslaunch's existing substitution mechanisms (we might need to lightly wrap the existing functions to simplify the calling convention). Then this example from #382:

<include file="$(find $(arg robot)description)/robots/upload$(arg robot).launch" />

could be done in a manner similar to this:

<include file="find(arg('robot')+'_description') + '/robots/upload' + arg('robot') + '.launch'" pyeval="true"/>

Hmm, then we'd also need to enable pyeval for other tags, as you suggested originally.

With regard to using the ${} syntax: that sounds like a good idea. In addition to not requiring the clumsy use of pyeval, it allows you to control Python evaluation on each attribute (right now, if you set pyeval="true" on an arg tag, then Python evaluation will happen for both the if/unless and value attributes, which might not be what you want).

Thinking that proposal through:

• Do we expect anybody to be using ${} for a different purpose in attribute values? If so, then one mitigation strategy would be to add a command-line option to roslaunch to enable/disable Python evaluation. The most conservative approach would be to default it to disabled.
• Can ${} be used more than once in a single attribute value? Concatenated? Nested?

Of course, defining python functions find and arg will work. But this new syntax dramatically differs from the existing $() one, thus breaking existing roslaunch files. Nevertheless I like the idea.
Definitely we will need some trigger to enable python evaluation. Using some ${} syntax will can enable this whereever we want - without the need for an explicit pyeval attribute.

If we are going to introduce new syntax, we should maybe stick with the exising $() syntax instead of introducing a completely new ${} one. That is, the example from #382 could be:

<include file="$(eval find(arg('robot')+'_description') + '/robots/upload_' + arg('robot') + '.launch')"/>

This has several advantages:

• it would elegantly delegate the nested parsing to python.
• $() was already kind of reserved, thus clashes with existing launch files using ${} are less probable
• Old syntax $(arg ...) and $(find ...) will still work.

Answering your questions:

• Using $(eval ...), we wouldn't need a command-line option anymore to mitigate existing code.
• Allowing concatenation of several $(eval) expressions would be nice to have:

<include file="$(eval find(arg('robot')+'_description'))/robots/upload_$(arg robot).launch"/>

• However, allowing this will hinder parsing: It becomes difficult to determine the closing bracket, considering that there might be some quoted string containing parentheses too. But, I didn't extensively look into the capabilities of pyparsing.
• Nesting isn't necessary if we have find + arg as functions. We don't have (and don't need) nesting of ${} in xacro as well.

I like the $(eval) idea a lot. A further advantage is that the implementation should be less intrusive. We just need a new _eval() method added to substitution_args.py, plus the logic to dispatch to that method from resolve_args() in the same file. There's nothing tag- or attribute-specific about it.

If concatenation causes parsing problems, then we can just document that you need to commit to computing an entire attribute value within one $(eval). And I agree on not needing nesting; if someone does that, then we'll just let the Python eval throw an error on that invalid input.

I will be happy to support on that implementation. It will increase the power of roslaunch significantly.
However, before starting, we should get a thumbs up from some maintainers, e.g. @dirk-thomas?

Dear @gerkey,

although I haven't seen a thumbs up from @dirk-thomas yet, I started to implement an initial version of the $(eval ...) substitution arg idea. Unfortunately, the new resolve method cannot be seamlessly integrated into the existing resolve_args mechanism (as you suggested), because the existing code uses the simple parser in _collect_args, which cannot handle parentheses within $(xxx ) expressions. Hence, in my branch https://github.com/ubi-agni/ros_comm/tree/roslaunch-eval, I handle $(eval ...) expressions in a specific fashion before resorting to the other resolve args.

The extension branch https://github.com/ubi-agni/ros_comm/tree/implicit-arg-access comprises a simplification to allow implicit access to arg values. Hence, instead of writing
$(eval arg('my_arg') == 42)
one can directly write:
$(eval my_arg == 42)

What do you think about that implementation?

@gerkey friendly ping.

Discussion from today: let's do the implementation, as @rhaschke suggests, merge it into jade, then decide on other distros from there.

@ros-pull-request-builder retest this please

@rhaschke Is the new implementation of this feature ready for review, or still in progress?

@gerkey There exists a first proof-of-concept implementation as announced before.
However, I just rebased the new implementation onto a recent indigo-devel, removing the old pyeval-attribute-based implementation. You can find that as a new PR in #762.

Closing in favor of #762.