optenv with single argument not supported in eval expression

[romainreignier]

According to the documentation, the optenv substitution argument can be used without the default_value supplied.
In an eval expression, the substitution arguments are converted into Python functions.
In rosmon, the optenv is defined as a function with two arguments using make_handler2 here:

rosmon/rosmon_core/src/launch/substitution_python.cpp

Line 114 in 4b00745

local["optenv"] = make_handler2(substitutions::optenv);

If optenv is used with a single argument, we have the following error:

Could not load launch file: /home/rre/open_mower_ros/src/open_mower/launch/include/_gps.launch:14: Substitution error: Caught Python exception while evaluating $(eval optenv('OM_GPS_BAUDRATE')!=''):
ArgumentError: Python argument types in
    None.None(str)
did not match C++ signature:
    None(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >)

This usage has been found in this open source project.

The following unit test allows to catch it:

diff --git a/rosmon_core/test/xml/test_subst.cpp b/rosmon_core/test/xml/test_subst.cpp
index 5044769..0ee92e3 100644
--- a/rosmon_core/test/xml/test_subst.cpp
+++ b/rosmon_core/test/xml/test_subst.cpp
@@ -302,6 +302,21 @@ TEST_CASE("eval", "[subst]")
                CHECK(value == 2*20);
        }
 
+       SECTION("optenv single argument")
+       {
+               LaunchConfig config;
+               config.parseString(R"EOF(
+                       <launch>
+                               <param name="test" value="$(eval 'test' + optenv('PATH'))"/>
+                       </launch>
+               )EOF");
+
+               config.evaluateParameters();
+
+               auto value = getTypedParam<std::string>(config.parameters(), "/test");
+               CHECK(value == std::string("test") + getenv("PATH"));
+       }
+
        SECTION("python errors")
        {
                using Catch::Matchers::Contains;

With the following output:

$ ./test_xml_loading eval
Filters: eval
Loaded launch file in 0.053113s
Loaded launch file in 0.057872s
Loaded launch file in 0.000252s
Loaded launch file in 0.000169s

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test_xml_loading is a Catch v2.13.7 host application.
Run with -? for options

-------------------------------------------------------------------------------
eval
  optenv single argument
-------------------------------------------------------------------------------
/home/rre/open_mower_ros/src/rosmon/rosmon_core/test/xml/test_subst.cpp:305
...............................................................................

/home/rre/open_mower_ros/src/rosmon/rosmon_core/test/xml/test_subst.cpp:305: FAILED:
due to unexpected exception with message:
  [string]:3: Substitution error: Caught Python exception while evaluating $
  (eval 'test' + optenv('PATH')):
  ArgumentError: Python argument types in
      None.None(str)
  did not match C++ signature:
      None(std::__cxx11::basic_string<char, std::char_traits<char>, std::
  allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>,
  std::allocator<char> >)

Loaded launch file in 0.000046s
===============================================================================
test cases:  1 |  0 passed | 1 failed
assertions: 13 | 12 passed | 1 failed

To resolve the issue, we need to find how to have a variable number of arguments for a Python function with Boost::Python, like *args.

Edit: Overloading and default arguments is actually part of the BoostPython tutorial.

[romainreignier]

This seems not so simple to replace the make_handler2 with a function with a default argument because we do not use the py::def() like in the Boost examples.

A basic workaround that works is to create a small wrapper in Python in the local context:

local["optenvcpp"] = make_handler2(substitutions::optenv);
py::exec("def optenv(env_var, default_value=''):\n"
	 "    return optenvcpp(env_var, default_value)", local);

I can make a PR with this change if you want.

[xqms]

Hey, thanks for the detailed diagnosis and proposed fix. I looked a bit into how overloads are handled in Boost::Python but it seems we cannot use that directly. To use def or even the more direct add_to_namespace, which handles overloads (https://github.com/boostorg/python/blob/4fc3afa3ac1a1edb61a92fccd31d305ba38213f8/include/boost/python/object/function_object.hpp#L31), we would need to change local to be a namespace object (and then pass its __dict__ to py::eval).

Another solution would be to use boost::python::raw_function.

But I'm fine with your proposed fix, it's easy to understand and a much smaller change. I'd welcome a PR!