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

rosidl_adapter/cmake/rosidl_adapt_interfaces.cmake: Make ament free #709

Merged

Conversation

yashi
Copy link
Contributor

@yashi yashi commented Oct 1, 2022

The module FindPython3 already provides the interpreter executable path via Python3_EXECUTABLE. No need to use ament.

Less loaded module means faster execution. And easier for out of tree build system to reuse this function.

Signed-off-by: Yasushi SHOJI yashi@spacecubics.com

The module FindPython3 already provides the interpreter executable
path via Python3_EXECUTABLE.  No need to use ament.

Less loaded module means faster execution.  And easier for out of tree
build system to reuse this function.

Signed-off-by: Yasushi SHOJI <yashi@spacecubics.com>
@aditya2592
Copy link

Hi, thanks for raising this PR. Can it be merged and backported to humble? We are having issues with building ROS2 on top of a custom python installation because of this line get_executable_path(python_interpreter Python3::Interpreter CONFIGURE) which is removed in this PR

@yashi
Copy link
Contributor Author

yashi commented Jan 19, 2023

Hi, I'm not clear what I'm asked to do. The PR is obviously not merged. But are you trying to use it on Humble? Am I asked to test it or port it on Humble? How can I reproduce the problem you are seeing here?

@aditya2592
Copy link

I think I just wanted to ask in general how to get this merged. Do you know the maintainers for this repo at all? Maybe we could tag them?

@aditya2592
Copy link

@clalancette could you help us figure out what we need to do in order to get this PR merged? It would help us in our project

@sloretz
Copy link
Contributor

sloretz commented Feb 2, 2023

We are having issues with building ROS2 on top of a custom python installation because of this line get_executable_path(python_interpreter Python3::Interpreter CONFIGURE) which is removed in this PR

@aditya2592 This PR is getting the path to the Python interpreter from FindPython3 with one less step, but I would not expect a difference in the path returned. That is I would assume the path returned by get_executable_path(... Python3::Interpreter CONFIGURE) is always the same as Python3_EXECUTABLE. If your build is finding the wrong version of python then this PR is probably not going to fix it. The solution you're probably looking for are the HINT variables use by the FindPython3 module.

Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

LGTM with full CI

@sloretz
Copy link
Contributor

sloretz commented Feb 2, 2023

And easier for out of tree build system to reuse this function.

@yashi if you're looking to convert .msg, .srv, or .action files to .idl without using ament_cmake then I'd recommend the rosidl translate command.

Here's a quick manual example:

sloretz@rolling-jammy:~$ cd /opt/ros/rolling/share/
sloretz@rolling-jammy:/opt/ros/rolling/share$ rosidl translate --to idl sensor_msgs std_msgs/msg/Header.msg -o /tmp/
Reading input file: /opt/ros/rolling/share/std_msgs/msg/Header.msg
Writing output file: /tmp/std_msgs/msg/Header.idl
sloretz@rolling-jammy:/opt/ros/rolling/share$ cat /tmp/std_msgs/msg/Header.idl 
// generated from rosidl_adapter/resource/msg.idl.em
// with input from sensor_msgs/std_msgs/msg/Header.msg
// generated code does not contain a copyright notice

#include "builtin_interfaces/msg/Time.idl"

module sensor_msgs {
  module msg {
    @verbatim (language="comment", text=
      "Standard metadata for higher-level stamped data types." "\n"
      "This is generally used to communicate timestamped data" "\n"
      "in a particular coordinate frame.")
    struct Header {
      @verbatim (language="comment", text=
        "Two-integer timestamp that is expressed as seconds and nanoseconds.")
      builtin_interfaces::msg::Time stamp;

      @verbatim (language="comment", text=
        "Transform frame with which this data is associated.")
      string frame_id;
    };
  };
};

For a real example, see how it's used to build ROS 2 interfaces with bazel: There's an example that uses it for building interfaces with bazel here: https://github.com/RobotLocomotion/drake-ros/blob/7c1429806547dadc7209f3784eecf3cf5106c45f/bazel_ros2_rules/ros2/rosidl.bzl#L117

…b.com:yashi/rosidl into yashi-rosidl_adapt_interfaces-cmake-make-ament-free

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
@sloretz sloretz force-pushed the rosidl_adapt_interfaces-cmake-make-ament-free branch from 48cb9ba to 377007a Compare February 2, 2023 18:57
@sloretz
Copy link
Contributor

sloretz commented Feb 2, 2023

CI (repos file build: everything test: everything)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link

@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.

CI can't find service_msgs on jammy it seems ?

@clalancette
Copy link
Contributor

CI can't find service_msgs on jammy it seems ?

For the Rpr job, yeah, that is going to be the case currently. We need to do a round of releases on Rolling (hopefully this week).

@sloretz sloretz merged commit ed099a2 into ros2:rolling Feb 6, 2023
@yashi yashi deleted the rosidl_adapt_interfaces-cmake-make-ament-free branch February 7, 2023 04:32
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.

None yet

5 participants