Fix issue with custom messages

[ahcorde]

Related with this issue #311

---

This change is 

[adityapande-1995]

Reviewable status: 0 of 6 files reviewed, 7 unresolved discussions (waiting on @ahcorde)

---

ros2_example_bazel_installed/ros2_example_apps/BUILD.bazel line 205 at r3 (raw file):

ros_py_binary(
    name = "ap_custom_msg_rosbag",

We can name this wrapper target as "rosbag_record_utility" or something.

---

ros2_example_bazel_installed/ros2_example_apps/BUILD.bazel line 238 at r3 (raw file):

        # Binaries to run.
        ":simple_talker",
        "@ros2",

We could have this test depend on the above "ap_custom_msg_rosbag" target to get all these deps for free as transitive deps.

Code quote:

        "//ros2_example_common:ros2_example_common_msgs_sl_fastrtps_cpp",
        "//ros2_example_common:ros2_example_common_msgs_sl_introspection_cpp",
        "//ros2_example_common:ros2_example_common_msgs_sl_typesupport_cpp",
        "//ros2_example_apps:ros2_example_apps_msgs_sl_fastrtps_cpp",
        "//ros2_example_apps:ros2_example_apps_msgs_sl_introspection_cpp",
        "//ros2_example_apps:ros2_example_apps_msgs_sl_typesupport_cpp",

        # Binaries to run.
        ":simple_talker",
        "@ros2",

[adityapande-1995]

Reviewable status: 0 of 6 files reviewed, 8 unresolved discussions (waiting on @ahcorde)

---

ros2_example_bazel_installed/ros2_example_apps/BUILD.bazel line 215 at r3 (raw file):

        "//ros2_example_apps:ros2_example_apps_msgs_sl_typesupport_cpp",
        # Binaries to run.
        ":simple_talker",

The rosbag record utility here itself doesn't require the talker, so can get rid of that. It is meant to run as a separate target.

[EricCousineau-TRI]

Reviewable status: 0 of 6 files reviewed, 14 unresolved discussions (waiting on @adityapande-1995 and @ahcorde)

---

bazel_ros2_rules/ros2/rosidl.bzl line 1361 at r1 (raw file):

Previously, adityapande-1995 (Aditya Pande) wrote…

i.e. if fastrtps is not available, building the rosbag wrapper rule would fail saying the fastrtps rosidl typesupport lib does not exist, or there is no matching target for it.

We should expose a roll-up target, and only have the user depend on that.
We should avoid having the user depend on bespoke and confusing target names, ideally.

---

bazel_ros2_rules/ros2/rosidl.bzl line 1112 at r4 (raw file):

    ]

symlink = rule(

nit This should be hidden, and should have more purpose-scoped name.

e.g. _symlink_shared_library_for_package().

---

bazel_ros2_rules/ros2/tools/dload.bzl line 107 at r4 (raw file):

        ament_prefixes = ctx.attr.target[AggregatedAmentIndexes].prefixes

        lib_folders = []

nit Traceability. This is a workaround, but there are no comments indicating the motivation.

Ideally, we point to upstream bug (if we have it recorded).
Alternatively, we just cite our drake-ros issue.

---

bazel_ros2_rules/ros2/tools/dload.bzl line 123 at r4 (raw file):

        env_changes["LD_LIBRARY_PATH"].extend(lib_folders)

    envvars = env_changes.keys()

Duplicate line?

---

ros2_example_bazel_installed/ros2_example_apps/BUILD.bazel line 238 at r3 (raw file):

Previously, adityapande-1995 (Aditya Pande) wrote…

We could have this test depend on the above "ap_custom_msg_rosbag" target to get all these deps for free as transitive deps.

OK See above.

---

ros2_example_bazel_installed/ros2_example_apps/BUILD.bazel line 204 at r4 (raw file):

)

ros_py_binary(

Purpose of this binary is not immediately clear.

If it is a suggested entry point for users to call rosbag, I would instead recommend that we forward our own //tools:ros2 target with the appropriate dependencies, and tell users to follow suite.

---

ros2_example_bazel_installed/ros2_example_apps/BUILD.bazel line 228 at r4 (raw file):

    srcs = ["test/custom_message_rosbag_test.py"],
    data = [
        # "//ros2_example_common:ros2_example_common_msgs_sl_fastrtps_cpp",

These names are generated, and it may not be obvious to the user what to use and when.

We should instead provide a simple roll-up target, e.g. add it to {name}_defs or make a new target like {name}_typesupport.

---

ros2_example_bazel_installed/ros2_example_apps/rosbag_record_utility.py line 1 at r4 (raw file):

import subprocess

Per above, I think it would be simplest if we instead have this be a wrapper around ros2.

[EricCousineau-TRI]

Reviewable status: 0 of 6 files reviewed, 15 unresolved discussions (waiting on @adityapande-1995 and @ahcorde)

a discussion (no related file):
Working: I'm going to take a pass at addressing some of my comments.

---

[EricCousineau-TRI]

Reviewable status: 0 of 6 files reviewed, 16 unresolved discussions (waiting on @adityapande-1995 and @ahcorde)

---

bazel_ros2_rules/ros2/tools/dload.bzl line 107 at r4 (raw file):

        ament_prefixes = ctx.attr.target[AggregatedAmentIndexes].prefixes

        lib_folders = []

Working: From looking at this code, this appears redundant?

[EricCousineau-TRI]

Reviewable status: 0 of 6 files reviewed, 16 unresolved discussions (waiting on @adityapande-1995 and @ahcorde)

---

bazel_ros2_rules/ros2/tools/dload.bzl line 107 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: From looking at this code, this appears redundant?

i.e., the _defs part of the messages should already emit the AmentIndexes providers, so we shouldn't need to do additional work to load the /lib prefix.
Also, the actual libraries should exist on the correct LD_LIBRARY_PATH export.

[EricCousineau-TRI]

Reviewable status: 0 of 6 files reviewed, 17 unresolved discussions (waiting on @adityapande-1995 and @ahcorde)

---

bazel_ros2_rules/ros2/rosidl.bzl line 1345 at r4 (raw file):

        symlink(
            name = name + "_sl_introspection_c",

nit "sl" is ambiguous. Should prefer expanding to "symlink"

[EricCousineau-TRI]

+@EricCousineau-TRI on core approach. I've addressed my comments.

Reviewed 1 of 9 files at r5.
Reviewable status: 1 of 11 files reviewed, 9 unresolved discussions (waiting on @adityapande-1995)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: I'm going to take a pass at addressing some of my comments.

Done. Sorry, got caught in some meetings.

---

---

bazel_ros2_rules/ros2/rosidl.bzl line 1361 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

We should expose a roll-up target, and only have the user depend on that.
We should avoid having the user depend on bespoke and confusing target names, ideally.

Done.

---

bazel_ros2_rules/ros2/rosidl.bzl line 1112 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit This should be hidden, and should have more purpose-scoped name.

e.g. _symlink_shared_library_for_package().

Done.

---

bazel_ros2_rules/ros2/rosidl.bzl line 1345 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit "sl" is ambiguous. Should prefer expanding to "symlink"

Done.

---

bazel_ros2_rules/ros2/tools/dload.bzl line 107 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Traceability. This is a workaround, but there are no comments indicating the motivation.

Ideally, we point to upstream bug (if we have it recorded).
Alternatively, we just cite our drake-ros issue.

Done.

---

bazel_ros2_rules/ros2/tools/dload.bzl line 107 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

i.e., the _defs part of the messages should already emit the AmentIndexes providers, so we shouldn't need to do additional work to load the /lib prefix.
Also, the actual libraries should exist on the correct LD_LIBRARY_PATH export.

Done. It was necessary. Unclear why, but not curious enough to find out exactly why.

---

bazel_ros2_rules/ros2/tools/dload.bzl line 123 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Duplicate line?

Done.

---

ros2_example_bazel_installed/ros2_example_apps/BUILD.bazel line 208 at r1 (raw file):

Previously, ahcorde (Alejandro Hernández Cordero) wrote…

c7a84ff

Done. The py msg lib seems to be necessary for ros2 topic echo to work.
For simplicity, my recommendation is to just wrap the env using py_library and deps = ["//:ros_msgs_all_py"].

---

ros2_example_bazel_installed/ros2_example_apps/BUILD.bazel line 228 at r1 (raw file):

Previously, ahcorde (Alejandro Hernández Cordero) wrote…

c7a84ff

Done, via roll-up into msgs_py targets and //:ros_msgs_all_py example.

---

ros2_example_bazel_installed/ros2_example_apps/BUILD.bazel line 236 at r1 (raw file):

Previously, ahcorde (Alejandro Hernández Cordero) wrote…

c7a84ff

Done.

---

ros2_example_bazel_installed/ros2_example_apps/BUILD.bazel line 205 at r3 (raw file):

Previously, adityapande-1995 (Aditya Pande) wrote…

We can name this wrapper target as "rosbag_record_utility" or something.

Done. Now handled via //tools:ros2

---

ros2_example_bazel_installed/ros2_example_apps/BUILD.bazel line 215 at r3 (raw file):

Previously, adityapande-1995 (Aditya Pande) wrote…

The rosbag record utility here itself doesn't require the talker, so can get rid of that. It is meant to run as a separate target.

Done.

---

ros2_example_bazel_installed/ros2_example_apps/BUILD.bazel line 238 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

OK See above.

Done.

---

ros2_example_bazel_installed/ros2_example_apps/BUILD.bazel line 204 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Purpose of this binary is not immediately clear.

If it is a suggested entry point for users to call rosbag, I would instead recommend that we forward our own //tools:ros2 target with the appropriate dependencies, and tell users to follow suite.

Done. Now handled via //tools:ros2

---

ros2_example_bazel_installed/ros2_example_apps/BUILD.bazel line 228 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

These names are generated, and it may not be obvious to the user what to use and when.

We should instead provide a simple roll-up target, e.g. add it to {name}_defs or make a new target like {name}_typesupport.

Done.

---

ros2_example_bazel_installed/ros2_example_apps/rosbag_record_utility.py line 1 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Per above, I think it would be simplest if we instead have this be a wrapper around ros2.

Done.

---

ros2_example_bazel_installed/ros2_example_apps/rosbag_record_utility.py line 1 at r4 (raw file):

import subprocess

Working: Should remove.

[EricCousineau-TRI]

Dismissed @adityapande-1995 and @ahcorde from a discussion.
Reviewable status: 1 of 11 files reviewed, 8 unresolved discussions (waiting on @adityapande-1995 and @ahcorde)

---

ros2_example_bazel_installed/ros2_example_apps/ap_custom_msg_rosbag.py line 2 at r1 (raw file):

Previously, ahcorde (Alejandro Hernández Cordero) wrote…

c7a84ff

OK This is now resolved since the role is handled via //tools:ros2.

---

ros2_example_bazel_installed/ros2_example_apps/rosbag_record_utility.py line 1 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: Should remove.

Done.

[adityapande-1995]

@EricCousineau-TRI the problem here might be that the symlink rules need to be dependencies of the rollup target, otherwise they won't be executed (?) as nothing depends on them.

Reviewable status: 1 of 11 files reviewed, 8 unresolved discussions (waiting on @ahcorde and @EricCousineau-TRI)

[EricCousineau-TRI]

They are. I'll point it out where it happens.

Reviewable status: 1 of 11 files reviewed, 8 unresolved discussions (waiting on @adityapande-1995 and @ahcorde)

---

bazel_ros2_rules/ros2/rosidl.bzl line 1245 at r6 (raw file):

            **kwargs
        )
        data += [name + "_symlink_fastrtps_cpp"]

FYI Lines like this add to the data list.

---

bazel_ros2_rules/ros2/rosidl.bzl line 1445 at r6 (raw file):

        interfaces = interfaces,
        includes = [_make_public_label(dep, "_defs") for dep in deps],
        data = data,

FYI The typesupport symlinks are included here.

---

bazel_ros2_rules/ros2/rosidl.bzl line 1529 at r6 (raw file):

        interfaces = interfaces,
        # Add the C++ target so that we can support `ros2 bag record`.
        data = data + [cc_name],

FYI Both C++ and C / Python typesupport are now available here.

[EricCousineau-TRI]

Reviewable status: 1 of 11 files reviewed, 9 unresolved discussions (waiting on @adityapande-1995 and @ahcorde)

a discussion (no related file):
Working: CI failure, most likely requires #278.

On my machine:
Using Debians, the test passes.
Using archive, it fails in a non-obvious way.

@ahcorde Once you've posted the issue (per meeting notes), can you link it here?

---

[EricCousineau-TRI]

This now passes CI.
@ahcorde @adityapande-1995 Can I get y'all to sign off on this / merge if it meets your expectations?

Reviewable status: 1 of 11 files reviewed, 8 unresolved discussions (waiting on @adityapande-1995 and @ahcorde)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: CI failure, most likely requires #278.

On my machine:
Using Debians, the test passes.
Using archive, it fails in a non-obvious way.

@ahcorde Once you've posted the issue (per meeting notes), can you link it here?

Done.

---

[adityapande-1995]

Reviewed 9 of 9 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ahcorde)

Comments addressed

[adityapande-1995]

Thanks for iterating on this @EricCousineau-TRI ! Looks great !