Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds support for plotjuggler & improves parsing logic. #336

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

adityapande-1995
Copy link
Collaborator

@adityapande-1995 adityapande-1995 commented Feb 22, 2024

Addresses first part of #333, by fixing the parsing logic for conditional tags. Also includes plotjuggler in ros2_examples.


This change is Reviewable

Signed-off-by: Aditya Pande <[email protected]>
Signed-off-by: Aditya Pande <[email protected]>
Signed-off-by: Aditya Pande <[email protected]>
@adityapande-1995
Copy link
Collaborator Author

adityapande-1995 commented Feb 23, 2024

Had to add an exception for plotjuggler due to this error :

--- captured stderr ---
CMake Error at CMakeLists.txt:19 (add_library):
  Target "empty_using_plotjuggler" links to target "Qt5::Widgets" but the
  target was not found.  Perhaps a find_package() call is missing for an
  IMPORTED target, or an ALIAS target is missing?


CMake Error at CMakeLists.txt:19 (add_library):
  Target "empty_using_plotjuggler" links to target "Qt5::Concurrent" but the
  target was not found.  Perhaps a find_package() call is missing for an
  IMPORTED target, or an ALIAS target is missing?


CMake Error at CMakeLists.txt:19 (add_library):
  Target "empty_using_plotjuggler" links to target "Qt5::Svg" but the target
  was not found.  Perhaps a find_package() call is missing for an IMPORTED
  target, or an ALIAS target is missing?

Otherwise I think this should be applicable to all packages. Also shows up in CI.

Signed-off-by: Aditya Pande <[email protected]>
Signed-off-by: Aditya Pande <[email protected]>
@adityapande-1995 adityapande-1995 marked this pull request as ready for review February 27, 2024 21:34
@adityapande-1995
Copy link
Collaborator Author

I did see libqt5concurrent5 libqt5svg5 libqt5svg5-dev libqt5widgets5 are already installed, and not able to find a target called empty_without_plotjuggler :/

@EricCousineau-TRI
Copy link
Collaborator

empty_using_{name} is generated by the following code:

def collect_ament_cmake_package_properties(name, metadata):
# NOTE(hidmic): each package properties are analyzed in isolation
# to preclude potential interactions if multiple packages were
# brought into the same CMake run. The latter could be done for
# speed
with TemporaryDirectory(dir=os.getcwd()) as project_path:
project_name = 'empty_using_' + name

To see the target, you would need to change the TemporaryDirectory, or add an option for "sandbox debugging" mode for the generated files.

My guess is that it indeed is missing find_package()?

@EricCousineau-TRI
Copy link
Collaborator

Adds plotjuggler and fixes parsing for it. [...]

Is this PR just meant to address Q1 of #333? Could you mention as much in the overview (just to make sure I've set expectations well)

If you're adding plotjuggler here, could you also add a plotjugger binary, and see what is necessary to ensure that the plugins are loadable via Bazel (without a ROS 2 workspace sourced)?

@adityapande-1995 adityapande-1995 changed the title Removes ROS 1 conditional tags for plotjuggler Removes ROS 1 conditional tags for plotjuggler_ros Feb 28, 2024
Signed-off-by: Aditya Pande <[email protected]>
Signed-off-by: Aditya Pande <[email protected]>
@adityapande-1995
Copy link
Collaborator Author

adityapande-1995 commented Mar 11, 2024

Working on colcon based CI here, though I was able to run plotjuggler with its plugins using bazel run @ros2//plotjuggler_plotjuggler , without ros plugins that is. Adding an example for ros2.

@adityapande-1995 adityapande-1995 marked this pull request as draft March 20, 2024 00:24
@adityapande-1995
Copy link
Collaborator Author

adityapande-1995 commented Mar 20, 2024

The problem here is the extra plugin libs that are not a part of any target and are tricky to add:

"${workspace_prefix}/lib/plotjuggler_ros/lib*.so*"

which would have to live entrirely in bazel_ros2_rules.

@EricCousineau-TRI
Copy link
Collaborator

EricCousineau-TRI commented Mar 20, 2024

can we just generate a {package}_plugins target that will glob patterns like that?

@adityapande-1995
Copy link
Collaborator Author

adityapande-1995 commented Mar 26, 2024

Okay, after a bit of investigation : plotjuggler-ros uses a Qt based plugin system instead of pluginlib, which ROS uses. This is causing problems with the parsing of plugins, and hence we do not have the plugins bundled up into a package. We do have a mechanism for parsing plugins, but that assumes we're doing things the ROS way. (

if 'plugin_libraries' in metadata:
)

Nothing in the package metadata specifies that we have plugins in plotjuggler-ros.
Plotjuggler itself just hardcodes what the path for the plugins is : https://github.com/facontidavide/PlotJuggler/blob/15dce41f598daed841bf2093aa10ebdf2aa1052f/plotjuggler_app/mainwindow.cpp#L560-L566.

i.e. we need some special rule tweaking here just for plotjuggler_ros.

Signed-off-by: Aditya Pande <[email protected]>
Signed-off-by: Aditya Pande <[email protected]>
@adityapande-1995
Copy link
Collaborator Author

adityapande-1995 commented Mar 26, 2024

Alright, we have a rollup target now that can load the required plugins :

cd ros2_examples_bazel_installed
bazel run //ros2_example_apps:plotjuggler

@adityapande-1995 adityapande-1995 changed the title Removes ROS 1 conditional tags for plotjuggler_ros Adds support for plotjuggler & improves parsing logic. Mar 26, 2024
@adityapande-1995 adityapande-1995 marked this pull request as ready for review March 26, 2024 21:26
Signed-off-by: Aditya Pande <[email protected]>
Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i.e. we need some special rule tweaking here just for plotjuggler_ros.

gotcha, special-case makes sense then, thanks!

Alright, we have a rollup target now that can load the required plugins :lgtm:

can you post a screenshot confirming that it has loaded the ROS 2 topic plugin?

Reviewed 2 of 3 files at r1, 1 of 1 files at r2, 2 of 3 files at r3, 1 of 3 files at r4, 1 of 2 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @adityapande-1995 and @ahcorde)


bazel_ros2_rules/ros2/resources/ros2bzl/templates.py line 111 at r7 (raw file):

            for library in metadata['plugin_libraries']
        )
    # Add an exception for plotjuggler-ros, as it does not

nit Per your investigation above, it would help future readers to include more detail.

Can you mention it is using Qt plugins, and perhaps provide a link to plotjuggler code that details on this?


bazel_ros2_rules/ros2/resources/ros2bzl/templates.py line 116 at r7 (raw file):

        prefix = "_opt_ros_humble/lib/plotjuggler_ros/"
        data.extend([
            prefix + "libDataLoadROS2.so",

nit These values may change.

Is it possible to simply glob these files?


ros2_example_bazel_installed/WORKSPACE line 84 at r7 (raw file):

    # "rmw_fastrtps_cpp",
] + [
    # Extra packages

The notion of "extra" may be unclear.

I recommend putting these in the first list starting on L67.


ros2_example_bazel_installed/ros2_example_apps/plotjuggler.py line 1 at r6 (raw file):

Previously, ahcorde (Alejandro Hernández Cordero) wrote…
#!/usr/bin/env python3
"""

We actually do not use shebangs inside Python files that will be executed via Bazel, as these files are not intended to be independently executable.

Aditya, can you remove the shebang?

Signed-off-by: Aditya Pande <[email protected]>
Copy link
Collaborator Author

@adityapande-1995 adityapande-1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 6 of 9 files reviewed, 3 unresolved discussions (waiting on @ahcorde and @EricCousineau-TRI)


ros2_example_bazel_installed/WORKSPACE line 84 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

The notion of "extra" may be unclear.

I recommend putting these in the first list starting on L67.

Done.


ros2_example_bazel_installed/ros2_example_apps/plotjuggler.py line 1 at r6 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

We actually do not use shebangs inside Python files that will be executed via Bazel, as these files are not intended to be independently executable.

Aditya, can you remove the shebang?

Done.

Copy link
Collaborator Author

@adityapande-1995 adityapande-1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image.png

Reviewable status: 6 of 9 files reviewed, 3 unresolved discussions (waiting on @ahcorde and @EricCousineau-TRI)

Signed-off-by: Aditya Pande <[email protected]>
Copy link
Collaborator Author

@adityapande-1995 adityapande-1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 6 of 9 files reviewed, 2 unresolved discussions (waiting on @ahcorde and @EricCousineau-TRI)


bazel_ros2_rules/ros2/resources/ros2bzl/templates.py line 116 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit These values may change.

Is it possible to simply glob these files?

Done

Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dismissed @ahcorde from a discussion.
Reviewable status: 6 of 9 files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI)


ros2_example_bazel_installed/WORKSPACE line 84 at r7 (raw file):

Previously, adityapande-1995 (Aditya Pande) wrote…

Done.

Now the list isn't sorted anymore ("Please keep this list sorted")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants