Add definition of ROS_PACKAGE_NAME by dhood · Pull Request #2 · ros2/ament_cmake_ros

dhood · 2017-10-23T20:34:09Z

~~This is needed for using a package's name as the default logger/logger prefix for ROS_INFO etc.~~ no longer needed for ros2/rclcpp#389

Putting it in this package as opposed to rclcpp (where the logging macros would make use the package name) means that packages below rclcpp e.g. rmw can also use this in their logging calls to rcutils directly.

However, it will require all packages e.g. demos to use ament_cmake_ros where they currently use ament_cmake, but I understand that we should be encouraging that anyway, correct?

dirk-thomas · 2017-10-23T20:39:56Z

Should the patch keep the copyright notice?

dhood · 2017-10-23T20:53:16Z

around the codebase I've seen "generated files don't have a copyright notice" in place of licenses; please advise

dhood · 2017-10-23T20:58:46Z

I can't find an example now so you might not know what it is I'm referring to. It was probably a specific case. I'll revert 57ffcb8

dirk-thomas · 2017-10-23T21:03:56Z

around the codebase I've seen "generated files don't have a copyright notice" in place of licenses; please advise

In the case of templates which are expanded with user provided information (e.g. message code) we commonly don't include a copyright notice. For other files I don't think we are specifically strict abour it and often don't cover the expanded files with the linters (which would complain if a file doesn't have any copyright line).

This reverts commit 57ffcb8.

dhood · 2017-10-23T21:30:54Z

OK, thanks for the explanation. What I've understood is that since this isn't being templated with any user-provide info then it should be treated as usual: 8e0e75b

dirk-thomas · 2017-10-23T21:33:30Z

ament_cmake_ros/ament_cmake_ros-extras.cmake.in

@@ -15,3 +15,4 @@
 # copied from ament_cmake_ros/ament_cmake_ros-extras.cmake


This line should still say "generated from ... .cmake.in".

sorry @dirk-thomas, I should have caught that myself. da58e48

dirk-thomas · 2017-10-23T21:38:49Z

ament_cmake_ros/ament_cmake_ros-extras.cmake.in

+# generated from ament_cmake_ros/ament_cmake_ros-extras.cmake.in

 include("${ament_cmake_ros_DIR}/build_shared_libs.cmake")
+add_definitions(-DROS_PACKAGE_NAME=\"${PROJECT_NAME}\")


Out of curiosity: why do the quotes need to be escaped when the overall argument is not wrapped in quotes?

Should this be PROJECT_NAME or the name extracted from the package.xml? Does ament assert that they are the same? In either case might the package xml name make more sense?

I don't see the reason myself; I was copying ros/ros_comm@a6a6f71, but maybe it was just to not assume anything about what is or isn't in the project name..?

wjwwood

other than one comment, lgtm

wjwwood · 2017-10-23T21:46:34Z

ament_cmake_ros/ament_cmake_ros-extras.cmake.in

+# generated from ament_cmake_ros/ament_cmake_ros-extras.cmake.in

 include("${ament_cmake_ros_DIR}/build_shared_libs.cmake")
+add_definitions(-DROS_PACKAGE_NAME=\"${PROJECT_NAME}\")


Should this be PROJECT_NAME or the name extracted from the package.xml? Does ament assert that they are the same? In either case might the package xml name make more sense?

dirk-thomas · 2017-10-23T22:46:00Z

ament_cmake already ensures that the package name equals the project name. Therefore I would avoid the extra code here and just use PROJECT_NAME.

dhood · 2017-10-23T22:54:43Z

Since there's been discussion before about whether that was a reasonable thing for ament_cmake to enforce, and given @wjwwood 's point about the name in package.xml better representing what we consider the package name, I think using ament_package_xml is both more explicit and robust to any changes. But, it's not necessary today, so we can skip it, and if anything changes in the future we can just remember to come back and update this.

This reverts commit 244cfc1.

dhood · 2017-11-14T17:14:42Z

this isn't required for ros2/rclcpp#389 anymore: do we think it's better to merge it so users can have access to ROS_PACKAGE_NAME, or close this out so we don't have to maintain it?

dirk-thomas · 2017-11-14T17:17:27Z

If we don't use it anywhere I would lean towards not merging it. But maybe we should keep the PR open for a bit since it looks like the gtests results would benefit from grouping them by the project name. So maybe we have a use case for this soon.

dhood · 2017-11-14T17:31:37Z

OK, sounds good to me!

mikaelarguedas · 2017-11-15T20:30:43Z

If we don't use it anywhere I would lean towards not merging it. But maybe we should keep the PR open for a bit since it looks like the gtests results would benefit from grouping them by the project name.

Should we move this out of review then ?

dhood · 2017-11-15T20:56:51Z

detached from ros2/rclcpp#389 and removed review label

dirk-thomas · 2017-11-15T21:56:56Z

I used a different approach to group the gtest results (see ament/ament_cmake#117). Therefore we can close this. (I will do so, please feel free to reopen if you think we should keep this.)

dhood · 2017-11-17T00:27:45Z

We won't necessarily be using this for rclcpp's logging.

However, currently logging calls lower in the stack e.g. in rcl are using hard-coded package names:
https://github.com/ros2/rcl/blob/ddc172a7e6624dd2c1d3359cdf6780ae3c597e0d/rcl/src/rcl/node.c#L311

If we are going to add more logging e.g. for debugging, it seems useful for core packages that are using ament_cmake_ros anyway to put ROS_PACKAGE_NAME instead of a hardcoded string, do you agree?

dirk-thomas · 2017-11-17T02:20:33Z

I would say it depends on how many packages would use the feature. If it e.g. is only rcl I would argue that the package could that itself. The point being anything we "expose" as API like this needs to have a good amount of documentation, examples, etc. in the future.

dhood · 2017-11-17T05:43:23Z

Yeah, I appreciate the point about considering the documentation effort when features aren't strictly necessary (in general).. I think in this particular case the documentation won't bring much work (especially since it's more of an internal feature as opposed to something people will use everyday).

Debug logging will be added to not just rcl but also rcutils, rclcpp, rmw, rmw_implementation, N rmw implementation packages, type support packages, etc. Considering that we sometimes shift code from one package to another I think we'll benefit in the future from not adding code with hard coded logger names. That's not to say that we have to use this PR to achieve it, but it seems like the most straightforward way to me (rather than having each package set something equivalent themselves just to avoid making this feature public).

Unless I've overlooked something in the reasoning on these points I'll go ahead and reopen this PR tomorrow

dhood added 2 commits October 20, 2017 15:56

Specify ROS_PACKAGE_NAME definition for use in logging macros

2de8f82

remove license of generated extras file

57ffcb8

dhood added the in review Waiting for review (Kanban column) label Oct 23, 2017

dhood self-assigned this Oct 23, 2017

Revert "remove license of generated extras file"

8e0e75b

This reverts commit 57ffcb8.

dirk-thomas reviewed Oct 23, 2017

View reviewed changes

Fix comment on file source

da58e48

dirk-thomas approved these changes Oct 23, 2017

View reviewed changes

wjwwood approved these changes Oct 23, 2017

View reviewed changes

wjwwood mentioned this pull request Oct 23, 2017

Prototype use of rclpy logging for callback_group ros2/examples#186

Closed

Get package name from package.xml

244cfc1

Revert "Get package name from package.xml"

164ae0b

This reverts commit 244cfc1.

dhood mentioned this pull request Oct 24, 2017

Implement rclcpp-specific logging macros [taking name not object] ros2/rclcpp#389

Merged

dhood added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Nov 15, 2017

dirk-thomas closed this Nov 15, 2017

dirk-thomas deleted the ros_package_name branch November 15, 2017 21:56

dirk-thomas removed the in progress Actively being worked on (Kanban column) label Nov 15, 2017

dhood restored the ros_package_name branch November 17, 2017 16:37

dhood reopened this Nov 20, 2017

dhood added the in progress Actively being worked on (Kanban column) label Nov 20, 2017

This was referenced Nov 20, 2017

Switch rcl_lifecyle to ament_cmake_ros ros2/rcl#186

Merged

Add debug logging ros2/rcl#187

Merged

dhood merged commit efe7732 into master Nov 22, 2017

dhood deleted the ros_package_name branch November 22, 2017 01:23

dhood removed the in progress Actively being worked on (Kanban column) label Nov 22, 2017

		@@ -15,3 +15,4 @@
		# copied from ament_cmake_ros/ament_cmake_ros-extras.cmake

Conversation

dhood commented Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dirk-thomas commented Oct 23, 2017

Uh oh!

dhood commented Oct 23, 2017

Uh oh!

dhood commented Oct 23, 2017

Uh oh!

dirk-thomas commented Oct 23, 2017

Uh oh!

dhood commented Oct 23, 2017

Uh oh!

dirk-thomas Oct 23, 2017

Choose a reason for hiding this comment

Uh oh!

dhood Oct 23, 2017

Choose a reason for hiding this comment

Uh oh!

dirk-thomas Oct 23, 2017

Choose a reason for hiding this comment

Uh oh!

wjwwood Oct 23, 2017

Choose a reason for hiding this comment

Uh oh!

dhood Oct 23, 2017

Choose a reason for hiding this comment

Uh oh!

wjwwood left a comment

Choose a reason for hiding this comment

Uh oh!

wjwwood Oct 23, 2017

Choose a reason for hiding this comment

Uh oh!

dirk-thomas commented Oct 23, 2017

Uh oh!

dhood commented Oct 23, 2017

Uh oh!

dhood commented Nov 14, 2017

Uh oh!

dirk-thomas commented Nov 14, 2017

Uh oh!

dhood commented Nov 14, 2017

Uh oh!

mikaelarguedas commented Nov 15, 2017

Uh oh!

dhood commented Nov 15, 2017

Uh oh!

dirk-thomas commented Nov 15, 2017

Uh oh!

dhood commented Nov 17, 2017

Uh oh!

dirk-thomas commented Nov 17, 2017

Uh oh!

dhood commented Nov 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dhood commented Oct 23, 2017 •

edited

Loading