[plugins] Unclear how to succinctly incorporate plotjugger data

[EricCousineau-TRI]

I am trying to use plotjugger in our internal codebase (Anzu). I envisioned doing something like this in a BUILD.bazel:

anzu_ros_sh_alias(
    name = "plotjuggler",
    data = [
        # Ensures we can access the ROS 2 subscriber topic.
        "@ros2//:plotjuggler_ros_cc",
    ],
    # Provides access to custom-generated messages.
    py_interface_deps = ["//:ros_msgs_all_py"],
    target = "@ros2//:plotjuggler_plotjuggler",
)

However, to make this work, I had to do hack our our ros2/repository.bzl (
click to expand)

_ROS_PACKAGES = [
...
    "plotjuggler",
    "plotjuggler_ros",
...
]

_BUILD_EXTRA_TPL = """
# Extra content for Anzu.

cc_library(
    name = "plotjuggler_ros_cc",
    data = [
        ":plotjuggler_ros_share",
        ":plotjuggler_ros_transitive_py",
    ] + glob(
        ["${workspace_prefix}/lib/plotjuggler_ros/lib*.so*"],
        allow_empty=False,
    ),
)
"""

def _append_to_file(repo_ctx, file, content):
    orig_content = repo_ctx.read(file)
    new_content = orig_content + "\n" + content
    repo_ctx.file(file, new_content)

def _anzu_ros2_local_repository_impl(repo_ctx):
...

    base_ros2_repository(repo_ctx, workspaces_in_sandbox)

    workspace_local_path = repo_ctx.attr.workspaces[0].replace("/", "_")
    build_extra = _BUILD_EXTRA_TPL.replace(
        "${workspace_prefix}",
        workspace_local_path,
    )
    _append_to_file(repo_ctx, "BUILD.bazel", build_extra)

Questions

1. Why did I have to inject my own plotjuggler_ros_cc library? I would've thought this would be caught by scanning.
2. Why did I have to include plotjuggler_ros_transitive_py? I would've thought plotjuggler_ros_share should have covered it.

[adityapande-1995]

Observations so far :

• For Q1, the package.xml in plotjuggler_ros is the problem. It contains the conditional tags that are not parsing correctly : https://github.com/facontidavide/PlotJuggler/blob/d16637fa9d47208c30e448d64c7a15ee2f85d83f/package.xml#L40.

The REP that allows those : https://ros.org/reps/rep-0149.html#id20

Removing those fixed the problem for me. I'll look at fixing the parsing logic : https://github.com/RobotLocomotion/drake-ros/pull/336/files

• However, if I look at bazel query --output=build @ros2//:plotjuggler_ros_cc, which was generated by removing the "condition tag", I get this rule, which is different from the one you posted above :

cc_library(
  name = "plotjuggler_ros_cc",
  srcs = [],
  hdrs = [],
  linkopts = [],
  includes = [],
  defines = [],
  copts = [],
  deps = ["@ros2//:rclcpp_cc", "@ros2//:rcpputils_cc", "@ros2//:rosbag2_cc", "@ros2//:rosbag2_transport_cc", "@ros2//:tf2_msgs_cc", "@ros2//:tf2_ros_cc"],
  data = ["@ros2//:plotjuggler_ros_share"],
)

[adityapande-1995]

• For Q2, I see the _transitive_py as a dep only applies to py packages or executables, and not to our cc rules. Looking into the _ros__share rule.

• The plotjuggler_ros_share seems to be a filegroup rule with srcs as package_run_dependencies. As per the README :

`<package_name>_share` filegroup for files in the `<package_prefix>/share` directory 
but excluding those files that are build system specific.

and for _transitive_py :

`<package_name>_transitive_py` Python library if the package does not
 install any Python libraries but it depends on (and it is a dependency of)
 packages that do. This helps maintain the dependency graph (as Python library
 targets can only depend on other Python library targets).

As per the rules here that behavior seems consistent to me :

drake-ros/bazel_ros2_rules/ros2/resources/ros2bzl/templates.py

Lines 37 to 48 in 4c17af0

	def configure_package_share_filegroup(name, metadata, sandbox):
	target_name = share_name(name, metadata)
	shared_directories = [sandbox(metadata['share_directory'])]
	if 'ament_index_directory' in metadata:
	shared_directories.append(sandbox(metadata['ament_index_directory']))
	return (
	target_name,
	load_resource('templates/package_share_filegroup.bazel.tpl'),
	to_starlark_string_dict({
	'name': target_name, 'share_directories': shared_directories
	})
	)

?

The _transitive_py targets become deps only to executables, or package py libs :

drake-ros/bazel_ros2_rules/ros2/resources/ros2bzl/templates.py

Line 170 in 4c17af0

deps.append(meta_py_label(dependency_name, dependency_metadata))

, and not to _share targets.

• I also see similar thing in rclcpp_share and rclcpp_transitive_py. Share does not depend on transitive, so not a plotjuggler specific behavior.
• Checking reverse deps shows we only use these for _py or other transitive_py targets, as expected :

$ bazel query --infer_universe_scope  "allrdeps(@ros2//:rclcpp_transitive_py)"
@ros2//:message_filters_py
@ros2//:plotjuggler_ros_transitive_py
@ros2//:rclcpp_action_transitive_py
@ros2//:rclcpp_components_transitive_py
@ros2//:rclcpp_transitive_py
@ros2//:ros2bag_py
@ros2//:rosbag2_compression_transitive_py
@ros2//:rosbag2_compression_zstd_transitive_py
@ros2//:rosbag2_cpp_transitive_py
@ros2//:rosbag2_py_py
@ros2//:rosbag2_transitive_py
@ros2//:rosbag2_transport_transitive_py
@ros2//:tf2_ros_transitive_py
Loading: 0 packages loaded

[EricCousineau-TRI]

Q1

Hm... Why does the conditional parsing cause the _cc target to not be emitted?
I would have thought that the package.xml would indirectly be parsed by CMake + ament logic.
Do you know how that gets parsed via normal colcon invocation, and where we may be deviating?

Ideally, we should fix the root issue rather than eliminating the condition for this and other packages (as you indicated in
#336).

Q2

Sorry, my question was less "why am I using Python stuff", but rather "what files were in this target that was not already present in the _cc / _share target"?
Without that target, I was unable to have a Bazel wrapped binary of plotjugger load the ROS 2 topic plugin.
I think you answered the _share question, but I'm still not sure why I couldn't load plugins with just _cc.

[adityapande-1995]

Q1

Hm... Why does the conditional parsing cause the _cc target to not be emitted? I would have thought that the package.xml would indirectly be parsed by CMake + ament logic. Do you know how that gets parsed via normal colcon invocation, and where we may be deviating?

Ideally, we should fix the root issue rather than eliminating the condition for this and other packages (as you indicated in #336).

For Q1 :
In drake-ros, we're parsing the package xml here :

drake-ros/bazel_ros2_rules/ros2/resources/ros2bzl/scraping/metadata.py

Line 23 in 4c17af0

build_type = tree.find('./export/build_type').text

. tree.find() does not care about the condition, and just returns the first entry that it can find. In the case of plotjuggler_ros, that is catkin, and since that does not contain the keyword cmake, the cc target is never generated :

drake-ros/bazel_ros2_rules/ros2/generate_build_file.py

Lines 121 to 126 in 4c17af0

	if 'cmake' in metadata.get('build_type'):
	properties = collect_ament_cmake_package_direct_properties(
	name, metadata, direct_dependencies, cache
	)
	_, template, config = configure_package_cc_library(

The substitution is handled here in ROS : https://github.com/ros-infrastructure/catkin_pkg/blob/74d072fb2e4b8aa4637aec68542a8cddd1775bde/src/catkin_pkg/condition.py#L26-L35

[EricCousineau-TRI]

If the version constraints are spelled consistently, I think we should update our parser to check if the condition implies ROS 2, e.g.,

def _is_most_likely_ros2(node):
    condition = build_type.get("condition")
    if not condition:
        return True
    elif condition == "$ROS_VERSION == 2":  # or something like this
        return True
    else:
        return False

def get_ros2_config_nodes(nodes):
    return [_is_most_likely_ros2(node) for node in nodes]

build_types = get_ros2_config_nodes(tree.findall("./export/build_type"))
assert len(build_types) > 0
build_type = build_types[0]

If there is super complex options, then we should just vendor in that parsing logic you quoted.