Use frontend group dependency & explicit dependencies in ros2launch #256

christophebedard · 2021-07-15T15:27:10Z

Closes #255

As suggested/discussed under ros2/launch#516 (comment), this uses a group dependency for the launch frontends and declares explicit dependencies on launch_xml and launch_yaml for ros2launch.

Using both a group dependency and hard/explicit dependencies allows this to work fine with binaries (with bloom) and also allows us to include any future launch frontend (or any user's custom frontend) in source builds. This is similar to how rcl depends on both the rcl_logging_packages group and the default logging implementation explicitly.

Signed-off-by: Christophe Bedard bedard.christophe@gmail.com

christophebedard · 2021-07-15T15:42:12Z

@hidmic @wjwwood here's (along with ros2/launch#520) the solution I'm proposing based on your comments.

wjwwood

lgtm

wjwwood · 2021-07-15T16:51:59Z

CI (--packages-above-and-dependencies launch_testing launch_xml launch_yaml ros2launch, cyclone only):

Linux
Linux-aarch64
macOS
Windows

wjwwood · 2021-07-15T21:50:34Z

Anyone have any idea why we have failures on Windows now? I didn't investigate, but it could be to new missing dependencies?

hidmic · 2021-07-16T15:06:31Z

Anyone have any idea why we have failures on Windows now? I didn't investigate, but it could be to new missing dependencies?

Some rclpy library dependencies are not getting picked up 🤔

hidmic · 2021-07-16T16:12:15Z

Update: I cannot reproduce it on a plain Windows VM. Something's up with the CI Windows container.

christophebedard · 2021-07-16T16:27:49Z

could this just be a flake?

wjwwood · 2021-07-16T18:33:58Z

I don't see how. If it is a flake, then it is a bug underneath.

wjwwood · 2021-07-16T18:34:38Z

I'll run CI again, just to check:

Linux
Linux-aarch64
macOS
Windows

hidmic · 2021-07-16T21:54:39Z

Same issue. The fun part is that other packages with tests that depend on rclpy, like test_communication, don't seem to have any issues. Let's see what happens on master:

Windows

christophebedard · 2021-07-17T12:41:12Z

The fun part is that other packages with tests that depend on rclpy, like test_communication, don't seem to have any issues.

yeah and launch_xml by itself doesn't depend (indirectly or directly) on rclpy:

$ colcon list -n --packages-up-to launch_xml
ament_copyright
ament_flake8
ament_index_python
ament_lint
ament_pep257
launch
launch_xml
osrf_pycommon

this is a bit confusing.

hidmic · 2021-07-19T17:57:01Z

yeah and launch_xml by itself doesn't depend (indirectly or directly) on rclpy:

It does indirectly through launch_ros, when launch loads all the extensions it knows about:

test\launch_xml\test_executable.py:62: in test_executable_wrong_subtag
    parser.parse_description(root_entity)
C:\ci\ws\install\Lib\site-packages\launch\frontend\parser.py:117: in parse_description
    actions = [self.parse_action(child) for child in entity.children]
C:\ci\ws\install\Lib\site-packages\launch\frontend\parser.py:117: in <listcomp>
    actions = [self.parse_action(child) for child in entity.children]
C:\ci\ws\install\Lib\site-packages\launch\frontend\parser.py:93: in parse_action
    self.load_launch_extensions()
C:\ci\ws\install\Lib\site-packages\launch\frontend\parser.py:77: in load_launch_extensions
    entry_point.load()
c:\python38\lib\importlib\metadata.py:77: in load
    module = import_module(match.group('module'))
c:\python38\lib\importlib\__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
<frozen importlib._bootstrap>:1014: in _gcd_import
    ...
C:\ci\ws\install\Lib\site-packages\launch_ros\__init__.py:17: in <module>
    from . import actions
C:\ci\ws\install\Lib\site-packages\launch_ros\actions\__init__.py:17: in <module>
    from .composable_node_container import ComposableNodeContainer
C:\ci\ws\install\Lib\site-packages\launch_ros\actions\composable_node_container.py:27: in <module>
    from .node import Node
C:\ci\ws\install\Lib\site-packages\launch_ros\actions\node.py:61: in <module>
    from rclpy.validate_namespace import validate_namespace
C:\ci\ws\install\Lib\site-packages\rclpy\validate_namespace.py:15: in <module>
    from rclpy.exceptions import InvalidNamespaceException
C:\ci\ws\install\Lib\site-packages\rclpy\exceptions.py:15: in <module>
    from rclpy.impl.implementation_singleton import rclpy_implementation as _rclpy
C:\ci\ws\install\Lib\site-packages\rclpy\impl\implementation_singleton.py:32: in <module>
    rclpy_implementation = import_c_library('._rclpy_pybind11', package)
C:\ci\ws\install\Lib\site-packages\rpyutils\import_c_library.py:39: in import_c_library
    return importlib.import_module(name, package=package)
c:\python38\lib\importlib\__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
E   ImportError: DLL load failed while importing _rclpy_pybind11: The specified module could not be found.
E   The C extension 'C:\ci\ws\install\Lib\site-packages\rclpy\_rclpy_pybind11.cp38-win_amd64.pyd' failed to be imported while being present on the system. Please refer to 'https://index.ros.org/doc/ros2/Troubleshooting/Installation-Troubleshooting/#import-failing-even-with-library-present-on-the-system' for possible solutions

christophebedard · 2021-07-19T18:43:31Z

It does indirectly through launch_ros, when launch loads all the extensions it knows about:

yeah, in practice, so isn't that kind of problematic/isn't that the reason why the test is failing?

launch_xml doesn't depend on rclpy, either directly or indirectly, according to the package.xml files. This means that, when the test gets run, PYTHONPATH doesn't contain the path to rclpy's build directory or the path to rclpy's lib/python3.8/site-packages directory. It only contains the paths to its declared direct/indirect dependencies' build/lib directories.

I don't understand why it only fails on Windows and why it seems to be working fine without this PR, and maybe I'm misunderstanding something, but this "undeclared" dependency seems problematic to me.

Note that launch_xml doesn't depend on launch_ros either. If we didn't build launch_ros in the CI run, I'm guessing it wouldn't be found by launch's extension search when running launch_xml tests, and so it wouldn't try to find rclpy libs, and thus it wouldn't fail, right?

christophebedard · 2021-07-19T18:52:01Z

Note that launch_xml doesn't depend on launch_ros either. If we didn't build launch_ros in the CI run, I'm guessing it wouldn't be found by launch's extension search when running launch_xml tests, and so it wouldn't try to find rclpy libs, and thus it wouldn't fail, right?

I re-ran the original Windows job with this PR and the launch PR, but with --packages-up-to/--packages-select launch_xml launch_yaml. launch_ros isn't built since I removed ros2launch from the build args. It runs fine:

Windows

christophebedard · 2021-07-19T19:01:15Z

This might be related to PATH instead of PYTHONPATH, but I assume it's the same problem https://github.com/ros2/rpyutils/blob/3e41fe4d71bc16f5a727a010f835fb49376b52b3/rpyutils/import_c_library.py#L35-L39

hidmic · 2021-07-19T19:24:22Z

launch_xml doesn't depend on rclpy, either directly or indirectly, according to the package.xml files. This means that, when the test gets run, PYTHONPATH doesn't contain the path to rclpy's build directory or the path to rclpy's lib/python3.8/site-packages directory. It only contains the paths to its declared direct/indirect dependencies' build/lib directories.

You're absolutely right.

Note that launch_xml doesn't depend on launch_ros either. If we didn't build launch_ros in the CI run, I'm guessing it wouldn't be found by launch's extension search when running launch_xml tests, and so it wouldn't try to find rclpy libs, and thus it wouldn't fail, right?

That's also right!

This might be related to PATH instead of PYTHONPATH,

It could be. We need a Windows-specific explanation as to why launch_ros ends up in the PYTHONPATH for launch_xml while rclpy does not (neither a dependency of launch_xml as far as package.xml metadata implies).

christophebedard · 2021-07-19T22:36:26Z

It could be. We need a Windows-specific explanation as to why launch_ros ends up in the PYTHONPATH for launch_xml while rclpy does not (neither a dependency of launch_xml as far as package.xml metadata implies).

I'll try to make time to look into it this week.

wjwwood · 2021-07-30T01:46:20Z

Has anyone had a chance to look into this?

christophebedard · 2021-07-30T01:51:21Z

I haven't had time yet unfortunately. I'll try to look into it tomorrow.

wjwwood · 2021-07-30T02:00:18Z

Yeah, no pressure, just curious.

christophebedard · 2021-08-02T14:06:21Z

I tried to reproduce and I couldn't either. I haven't had time to dig deeper.

jacobperron · 2021-08-03T01:42:36Z

This might be related to PATH instead of PYTHONPATH, but I assume it's the same problem https://github.com/ros2/rpyutils/blob/3e41fe4d71bc16f5a727a010f835fb49376b52b3/rpyutils/import_c_library.py#L35-L39

For some context, since Python 3.8, PATH (and the current working directory) are no longer used for DLL resolution on Windows: https://docs.python.org/3/whatsnew/3.8.html#bpo-36085-whatsnew

To work around this, we introduced that utility in rpyutils, which explicitly add directories from PATH to the search path when importing a module. (TBH, the whole thing seems to subvert the security measure taken by Python, but I'm not sure how we might handle it differently for ROS 2).

I guess what's happening in this case is that the rclpy DLL directory is not in PATH at test time for launch_xml. I'm not sure why, but maybe because it doesn't have launch_ros in it's dependency tree (as pointed out earlier).

Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>

christophebedard · 2021-11-11T19:39:09Z

Thanks for the info @jacobperron.

I once again tried to reproduce and couldn't. Without being able to reproduce on my local machine, I'm not really sure how to proceed, especially since this is quite time-consuming for such a minor change.

wjwwood · 2021-11-11T19:48:19Z

Well, let's just see what CI says again, since you rebased (I'm assuming that's all):

Linux
Linux-aarch64
macOS
Windows

christophebedard · 2021-11-11T22:19:07Z

Yeah I simply rebased.

Those CI jobs don't build/test the same packages as the jobs that failed above, like this one: https://ci.ros2.org/job/ci_windows/15054/

christophebedard · 2021-11-11T22:22:19Z

Here is a run with the same packages:

Linux
Linux-aarch64
macOS
Windows

wjwwood · 2021-11-11T23:00:44Z

Oh sorry, I forgot to add ros2/launch#520.

christophebedard · 2021-11-12T14:48:55Z

CI is green, so... I guess it's fixed‽

The issue, as I understood it, was slightly concerning, so I hope this doesn't come back to bite us.

jacobperron · 2021-11-12T22:40:24Z

I think the issue was the same as ros2/rclpy#831
Which we recently fixed in #278

wjwwood approved these changes Jul 15, 2021

View reviewed changes

wjwwood mentioned this pull request Jul 30, 2021

End to end test osrf/ros2launch_security#4

Merged

2 tasks

christophebedard mentioned this pull request Jul 30, 2021

Suggest adding exec_depend on ros2launch for packages with launch files ros2/ros2_documentation#1786

Merged

christophebedard added 3 commits November 11, 2021 08:25

Switch to package format 3

b0994e5

Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>

Declare dependency on launch_{xml,yaml} in ros2launch

fd2b964

Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>

Declare a 'launch_frontend_packages' group and use in ros2launch

66c6d7e

Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>

christophebedard force-pushed the launch-frontend-dependencies branch from dbe2ac4 to 66c6d7e Compare November 11, 2021 16:25

jacobperron approved these changes Nov 12, 2021

View reviewed changes

jacobperron merged commit b84b727 into ros2:master Nov 12, 2021

christophebedard deleted the launch-frontend-dependencies branch November 12, 2021 22:44

christophebedard mentioned this pull request Nov 16, 2021

Declare frontend group dependency & use explicit dependencies in launch_testing ros2/launch#520

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use frontend group dependency & explicit dependencies in ros2launch #256

Use frontend group dependency & explicit dependencies in ros2launch #256

christophebedard commented Jul 15, 2021 •

edited

Loading

christophebedard commented Jul 15, 2021

wjwwood left a comment

wjwwood commented Jul 15, 2021 •

edited

Loading

wjwwood commented Jul 15, 2021

hidmic commented Jul 16, 2021 •

edited

Loading

hidmic commented Jul 16, 2021

christophebedard commented Jul 16, 2021

wjwwood commented Jul 16, 2021

wjwwood commented Jul 16, 2021

hidmic commented Jul 16, 2021 •

edited

Loading

christophebedard commented Jul 17, 2021 •

edited

Loading

hidmic commented Jul 19, 2021

christophebedard commented Jul 19, 2021

christophebedard commented Jul 19, 2021

christophebedard commented Jul 19, 2021

hidmic commented Jul 19, 2021 •

edited

Loading

christophebedard commented Jul 19, 2021

wjwwood commented Jul 30, 2021

christophebedard commented Jul 30, 2021

wjwwood commented Jul 30, 2021

christophebedard commented Aug 2, 2021

jacobperron commented Aug 3, 2021

christophebedard commented Nov 11, 2021

wjwwood commented Nov 11, 2021

christophebedard commented Nov 11, 2021

christophebedard commented Nov 11, 2021

wjwwood commented Nov 11, 2021

christophebedard commented Nov 12, 2021

jacobperron commented Nov 12, 2021

Use frontend group dependency & explicit dependencies in ros2launch #256

Use frontend group dependency & explicit dependencies in ros2launch #256

Conversation

christophebedard commented Jul 15, 2021 • edited Loading

christophebedard commented Jul 15, 2021

wjwwood left a comment

Choose a reason for hiding this comment

wjwwood commented Jul 15, 2021 • edited Loading

wjwwood commented Jul 15, 2021

hidmic commented Jul 16, 2021 • edited Loading

hidmic commented Jul 16, 2021

christophebedard commented Jul 16, 2021

wjwwood commented Jul 16, 2021

wjwwood commented Jul 16, 2021

hidmic commented Jul 16, 2021 • edited Loading

christophebedard commented Jul 17, 2021 • edited Loading

hidmic commented Jul 19, 2021

christophebedard commented Jul 19, 2021

christophebedard commented Jul 19, 2021

christophebedard commented Jul 19, 2021

hidmic commented Jul 19, 2021 • edited Loading

christophebedard commented Jul 19, 2021

wjwwood commented Jul 30, 2021

christophebedard commented Jul 30, 2021

wjwwood commented Jul 30, 2021

christophebedard commented Aug 2, 2021

jacobperron commented Aug 3, 2021

christophebedard commented Nov 11, 2021

wjwwood commented Nov 11, 2021

christophebedard commented Nov 11, 2021

christophebedard commented Nov 11, 2021

wjwwood commented Nov 11, 2021

christophebedard commented Nov 12, 2021

jacobperron commented Nov 12, 2021

christophebedard commented Jul 15, 2021 •

edited

Loading

wjwwood commented Jul 15, 2021 •

edited

Loading

hidmic commented Jul 16, 2021 •

edited

Loading

hidmic commented Jul 16, 2021 •

edited

Loading

christophebedard commented Jul 17, 2021 •

edited

Loading

hidmic commented Jul 19, 2021 •

edited

Loading