ros2 / launch

Tools for launching multiple processes and for writing tests involving multiple processes.
Apache License 2.0
124 stars 139 forks source link

Using submodules in python eval #687

Open zflat opened 1 year ago

zflat commented 1 year ago

Bug report

Required Info:

Steps to reproduce issue

I want to use the eval python substitution with a module that is within a package or parent module. For example, when I add os.path as a module to import, then I should be able to use a function from that module in the python eval substitution. However, since path in this case is a submodule of os, the parent module os is not available for performing substitutions when only passing "os.path" as the second argument to eval.

Here is a failing test case where only "os.path" is passed. (added to launch/test/launch/frontend/test_substitutions.py)

def test_eval_subst_submodule():
    # Case where a submodule is used
    subst = parse_substitution(
        r'$(eval "os.path.exists(\'/\')" "os.path")')
    assert len(subst) == 1
    expr = subst[0]
    assert isinstance(expr, PythonExpression)
    assert expr.perform(LaunchContext())

Here is a passing test case where "os, os.path" is passed.

def test_eval_subst_submodule():
    # Case where a submodule is used
    subst = parse_substitution(
        r'$(eval "os.path.exists(\'/\')" "os, os.path")')
    assert len(subst) == 1
    expr = subst[0]
    assert isinstance(expr, PythonExpression)
    assert expr.perform(LaunchContext())

Expected behavior

I should be able to pass "os.path" as the second argument to eval to use methods from os.path.

Actual behavior

I must pass "os, os.path" as the second argument to eval to use methods from os.path. Otherwise I get an error:

NameError: name 'os' is not defined

Additional information


Feature request

Feature description

I don't know if importing all parent modules is desired, but the error I get when only providing "os.path" is not easy to understand. I think the feature of being able to specify submodule will be easier to use if we import and add the parent modules of a specified submodule.

Implementation considerations

Update launch/launch/substitutions/python_expression.py class PythonExpression method perform to build out a list of all modules and parent modules from self.python_modules. So if "os.path" is given, then the list of modules to import using importlib.import_module would be ["os", "os.path"].

zflat commented 1 year ago

@RobertBlakeAnderson Thanks for adding the ability to specify modules to use in the python eval launch substitution in https://github.com/ros2/launch/pull/655! I'm wondering if you considered the case that I mention in this issue and if you have any thoughts on how to address the issue?