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

[roslaunch] roslaunch-check doesn't respect if/unless #993

Closed
mikepurvis opened this issue Feb 18, 2017 · 2 comments
Closed

[roslaunch] roslaunch-check doesn't respect if/unless #993

mikepurvis opened this issue Feb 18, 2017 · 2 comments
Labels

Comments

@mikepurvis
Copy link
Member

mikepurvis commented Feb 18, 2017

We're having a case where we have some launch files which optionally include other launch files based on an environment variable being set. These are failing the test created by roslaunch_add_file_check, since it doesn't respect the conditional and attempts to resolve all include elements without regard for context.

This can be reproduced trivially by a launch file such as this, which launches just fine, but will (wrongly, in my opinion) fail the roslaunch check:

<launch>
  <include if="0" file="does/not/exist.launch" />
</launch>

Interestingly, roslaunch_add_file_check totally does resolve $(arg), $(env), $(optenv), etc, so this really does seem to be an issue with just the conditionals.

@mikepurvis
Copy link
Member Author

mikepurvis commented Feb 21, 2017

I believe the issue is here:

elif tag.tagName == 'include':
try:
sub_launch_file = resolve_args(tag.attributes['file'].value, context)
except KeyError as e:
raise RoslaunchDepsException("Cannot load roslaunch <%s> tag: missing required attribute %s.\nXML is %s"%(tag.tagName, str(e), tag.toxml()))

It needs logic to explicitly check for if/unless, similar to how the args work:

if tag.attributes.has_key('if'):
val = resolve_args(tag.attributes['if'].value, context)
if val == '1' or val == 'true':
return (name, _get_arg_value(tag, context))
elif tag.attributes.has_key('unless'):
val = resolve_args(tag.attributes['unless'].value, context)
if val == '0' or val == 'false':
return (name, _get_arg_value(tag, context))

What's mysterious to me is that this works correctly for a normal launch but not for roslaunch-check. Is there some preprocessing going on that strips out tags based on if/unless attributes?


Looks like for a normal launch, this is handled by xmlloader.py, and the @ifunless decorator:

def ifunless_test(obj, tag, context):
"""
@return True: if tag should be processed according to its if/unless attributes
"""
if_val, unless_val = obj.opt_attrs(tag, context, ['if', 'unless'])
if if_val is not None and unless_val is not None:
raise XmlParseException("cannot set both 'if' and 'unless' on the same tag")
if if_val is not None:
if_val = loader.convert_value(if_val, 'bool')
if if_val:
return True
elif unless_val is not None:
unless_val = loader.convert_value(unless_val, 'bool')
if not unless_val:
return True
else:
return True
return False
def ifunless(f):
"""
Decorator for evaluating whether or not tag function should run based on if/unless attributes
"""
def call(*args, **kwds):
#TODO: logging, as well as check for verbose in kwds
if ifunless_test(args[0], args[1], args[2]):
return f(*args, **kwds)
return call

@dirk-thomas It seems like there are some parallel implementation issues going on here, but I don't have the desire or energy to do a wholesale fixup of it. I'd prefer to fix the immediate bug as best I can and at least add a unit test which would help support a future refactoring. Unless you have some particular insight into what the relationship is between xmlloader and these other pieces?

@dirk-thomas
Copy link
Member

Addressed by #998. Thank you!

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

No branches or pull requests

2 participants