rclcpp logging still uses fprintf all over the place. by guillaumeautran · Pull Request #439 · ros2/rclcpp

guillaumeautran · 2018-02-17T04:54:02Z

Remove all printf log lines and replace with RCLUTILS_LOG_XXX macros.

Issue: #438

mikaelarguedas

Thanks @guillaumeautran for the patch.
This looks good to me except some style comments that will make out linter tests fail. You can run the tests of a given package by using ament test --only <PKG_NAME>

mikaelarguedas · 2018-02-17T06:48:03Z

rclcpp/src/rclcpp/utilities.cpp

 #include "rmw/error_handling.h"
 #include "rmw/rmw.h"

+#include "rcutils/logging_macros.h"


Nit: Header included twice?

mikaelarguedas · 2018-02-17T06:49:32Z

rclcpp/src/rclcpp/publisher.cpp


+#include "rcutils/logging_macros.h"
+
+


mikaelarguedas · 2018-02-17T06:50:23Z

rclcpp/src/rclcpp/executor.cpp


+#include "rcutils/logging_macros.h"
+
+


single vertical whitespace

mikaelarguedas · 2018-02-17T07:49:07Z

rclcpp/src/rclcpp/utilities.cpp

-        fprintf(stderr,
-          "[rclcpp::error] failed to trigger guard condition: %s\n", rcl_get_error_string_safe());
+        RCUTILS_LOG_ERROR_NAMED(
+        "rclcpp",


Nit: this should be indented

mikaelarguedas · 2018-02-17T07:54:18Z

rclcpp/include/rclcpp/client.hpp

 #include "rmw/error_handling.h"
 #include "rmw/rmw.h"

+#include "rcutils/logging_macros.h"


Nit: we encourage the use of single vertical space

guillaumeautran · 2018-02-18T13:35:28Z

@mikaelarguedas I'm having problem running the tests on my system due to some gmock cmake non-sense. Any idea?

-- Found gmock sources under '/usr/src/gmock': C++ tests using 'Google Mock' will be built
CMake Error at /usr/src/gtest/cmake/internal_utils.cmake:130 (add_library):
  add_library cannot create target "gtest" because another target with the
  same name already exists.  The existing target is a static library created
  in source directory "/usr/src/gtest".  See documentation for policy CMP0002
  for more details.
Call Stack (most recent call first):
  /usr/src/gtest/cmake/internal_utils.cmake:153 (cxx_library_with_type)
  /usr/src/gtest/CMakeLists.txt:70 (cxx_library)


CMake Error at /usr/src/gtest/cmake/internal_utils.cmake:130 (add_library):
  add_library cannot create target "gtest_main" because another target with
  the same name already exists.  The existing target is a static library
  created in source directory "/usr/src/gtest".  See documentation for policy
  CMP0002 for more details.
Call Stack (most recent call first):
  /usr/src/gtest/cmake/internal_utils.cmake:153 (cxx_library_with_type)
  /usr/src/gtest/CMakeLists.txt:71 (cxx_library)


CMake Error at /usr/src/gtest/CMakeLists.txt:72 (target_link_libraries):
  Attempt to add link library "gtest" to target "gtest_main" which is not
  built in this directory.

Remove all printf log lines and replace with RCLUTILS_LOG_XXX macros. Issue: ros2#438

wjwwood · 2018-02-22T03:04:08Z

@guillaumeautran please include more context for that error, ideally all of the build output in a gist.github.com or something.

guillaumeautran · 2018-02-23T22:11:58Z

Ok, I sorted out my issue. Caused by: https://cmake.org/cmake/help/v3.0/policy/CMP0002.html since my cmake version is 3.5.1
But I took care of it and am now able to address the review comments. Patch coming up soon.

dhood · 2018-02-23T22:43:08Z

CI:

Linux
Linux-aarch64
macOS
Windows

dhood · 2018-02-24T01:24:27Z

This looks good thanks @guillaumeautran

I was going to suggest the use of ROS_PACKAGE_NAME for the logger name but it's provided by ament_cmake_ros which isn't be used by this package yet (#444)

Signed-off-by: cevans87 <c.d.evans87@gmail.com>

guillaumeautran force-pushed the issue-438 branch from ad9f93b to 6d43a0d Compare February 17, 2018 04:57

mikaelarguedas reviewed Feb 17, 2018

View reviewed changes

rclcpp logging still uses fprintf all over the place.

6ef1494

Remove all printf log lines and replace with RCLUTILS_LOG_XXX macros. Issue: ros2#438

guillaumeautran force-pushed the issue-438 branch from 6d43a0d to 6ef1494 Compare February 18, 2018 13:42

dhood added the in review Waiting for review (Kanban column) label Feb 20, 2018

fixup include order

5ca8325

dhood merged commit 0e79842 into ros2:master Feb 24, 2018

dhood removed the in review Waiting for review (Kanban column) label Feb 24, 2018

dhood mentioned this pull request Apr 27, 2018

rclcpp logging still uses fprintf #438

Closed

mikaelarguedas mentioned this pull request Apr 27, 2018

update to rcutils logging in commented code as well #466

Merged

nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022

Fix leaks in test_wait.cpp (ros2#439)

0c4c743

Signed-off-by: cevans87 <c.d.evans87@gmail.com>

Conversation

guillaumeautran commented Feb 17, 2018

Uh oh!

mikaelarguedas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guillaumeautran commented Feb 18, 2018

Uh oh!

wjwwood commented Feb 22, 2018

Uh oh!

guillaumeautran commented Feb 23, 2018

Uh oh!

dhood commented Feb 23, 2018

Uh oh!

dhood commented Feb 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants