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

Unnecessary build dependencies and CMake requirements for interfaces since DP3 #63

Closed
travipross opened this issue Apr 14, 2023 · 4 comments
Assignees
Labels
verify to close Waiting on confirm issue is resolved

Comments

@travipross
Copy link

The isaac_ros_tensor_list_interfaces package (likely among others), since the DP3 update, now has a dependency on isaac_ros_common, and similarly now requires a minimum CMake version of 3.23.2 (presumably both of these changes have something to do with trying to load common CMake arguments as per this file).

This introduces a challenging breaking change to resolve for those looking to develop lightweight nodes who depend solely on these interfaces. I'd argue that there is no compelling reason to make this interface package depend on the isaac_ros_common package (and therefore its upstream dependencies), as it's solely providing some .msg definitions. It also does not warrant such a high CMake version requirement.

Some specific scenarios/challenges that I've encountered when trying to develop a node that subscribes to the a TensorList topic as published by isaac_ros_dnn_inference running a Triton Inference Server:

  1. CMake minimum version: The official ROS2 Humble docker image ships with Ubuntu 22.04, and the default repositories only supply up to CMake version 3.22.1 as the most recent version. In order to meet this CMake version requirement, one would need to build CMake from source (assuming they're not using an Nvidia-provided base image including a newer version, which would otherwise be unnecessary).
  2. Dependency on isaac_ros_common: There should be no need for isaac_ros_common to be a dependency of this interfaces package, as all defined message types contained within isaac_ros_tensor_list_interfaces can safely be compiled without this dependency. I feel the build dependencies should be strictly used only when necessary, and this particular subpackage doesn't seem to fit that criteria.
@jaiveersinghNV jaiveersinghNV self-assigned this Apr 17, 2023
@jaiveersinghNV jaiveersinghNV added the coming soon! Expected in upcoming release label Apr 17, 2023
@jaiveersinghNV
Copy link
Contributor

Thank you for the feedback!

We initially set the minimum CMake version to 3.23.2 in order to ensure ROS 2 could properly be built inside our container. Since we provide this CMake version in our containers, we made it the standard in all of our packages. However, we will look into lowering this down to what's available in Ubuntu 22.04 (3.22.1) and pushing out changes to all of the CMake files.

As for your second point, we agree that the *_interfaces packages do not need to have a dependency on isaac_ros_common. We'll push out a fix for this as well.

@alexMarFar
Copy link

Hi! I am running into the error:

--- stderr: isaac_ros_common                                                             
CMake Error at CMakeLists.txt:9 (cmake_minimum_required):
  CMake 3.23.1 or higher is required.  You are running version 3.22.1
---

I think it is related with the comment @travipross did above on:

CMake minimum version: The official ROS2 Humble docker image ships with Ubuntu 22.04, and the default repositories only supply up to CMake version 3.22.1 as the most recent version. In order to meet this CMake version requirement, one would need to build CMake from source (assuming they're not using an Nvidia-provided base image including a newer version, which would otherwise be unnecessary).

I was wondering if this has this been addressed without building CMake from source? Thank you in advance!

@jaiveersinghNV
Copy link
Contributor

We're testing this right now, and we'll report back once we've validated the downgraded version of CMake. Thanks for your patience!

@hemalshahNV hemalshahNV added verify to close Waiting on confirm issue is resolved and removed coming soon! Expected in upcoming release labels Oct 19, 2023
@hemalshahNV
Copy link
Contributor

Fix in Isaac ROS 2.0.0 available now. Thanks for flagging this! Please verify and close as appropriate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
verify to close Waiting on confirm issue is resolved
Projects
None yet
Development

No branches or pull requests

4 participants