enforce unique node names by Karsten1987 · Pull Request #86 · ros2/rosbag2

Karsten1987 · 2019-02-06T05:15:27Z

fixes the unique node problem referred to in ros2/rcl#375

the template function indeed creates a new static int counter per type and thus non-unique node names when different types are used.

Karsten1987 · 2019-02-06T06:16:10Z

Linux
Linux-aarch64
macOS
Windows

Karsten1987 · 2019-02-06T07:17:10Z

@jacobperron the current CI fails because of ament/ament_lint#116.
how do i ensure cppcheck is correctly invoked?

jacobperron · 2019-02-06T07:38:45Z

@jacobperron the current CI fails because of ament/ament_lint#116.
how do i ensure cppcheck is correctly invoked?

That's unfortunate, I ended up excluding passing include directories from external packages to cppcheck because of performance:

https://github.com/ament/ament_lint/blob/4f9bfc00106496eeafa592bc5dfbc33b052d643d/ament_cmake_cppcheck/cmake/ament_cmake_cppcheck_lint_hook.cmake#L44-L47

One way around it would be to manually invoke ament_cppcheck and tell it about the header files containing the macros in rclcpp (rather than relying on ament_lint_auto). This means that you'd also have to invoke the other desired linters manually too. I think the ament_cppcheck command would look something like this:

ament_cppcheck(INCLUDE_DIRS ${rclcpp_INCLUDE_DIRS})

I think this is another instance motivating ament/ament_lint#119, or for a feature enabling more control over the configurations for the linters invoked by ament.

dirk-thomas · 2019-02-06T16:55:30Z

Our package template already demonstrates how individual linters part of the common linters can be skipped: https://github.com/ros2/ros2cli/blob/5dd01716edbb899e9ee270e56cf47a77b94997a2/ros2pkg/ros2pkg/resource/ament_cmake/CMakeLists.txt.em#L92-L94

Karsten1987 · 2019-02-06T17:13:27Z

That's unfortunate, I ended up excluding passing include directories from external packages to cppcheck because of performance:

does that mean no external package can use ament_lint_auto? Or asked differently, which packages can use it? How is external defined here?
if every package has to manually exclude cppcheck then automatic lookup for linters defeats kind of the purpose, no?

jacobperron · 2019-02-06T17:19:58Z

does that mean no external package can use ament_lint_auto? Or asked differently, which packages can use it? How is external defined here?

To elaborate, to handle the type of error you're seeing I'd added support in ament/ament_lint#119 to pass a list of include directories to cppcheck so that it can resolve the "unknown macros". Unfortunately, a larger list of directories results in a very slow execution of cppcheck. So, when cppcheck is invoked automatically with ament_lint_auto, we limit the number of include directories to just those of the package being linted. An "external" package for the purpose of the automatic cppcheck, in this case, is anything that is not in rosbag2_transport.

Karsten1987 · 2019-02-27T19:56:17Z

Linux
Linux-aarch64
macOS
Windows

Karsten1987 · 2019-02-27T21:04:40Z

unrelated test errors/warnings. That means the cppcheck is correctly disabled as well as the node names are unique and don't produce any warnings.

Signed-off-by: James Smith <james@foxglove.dev> mcap_storage: 'none' is a valid storage preset profile (ros2#86) Signed-off-by: James Smith <james@foxglove.dev> bloom: add changelog changes 0.6.0

* rosbag2_storage_mcap: merge into ros2/rosbag2 Signed-off-by: James Smith <james@foxglove.dev> mcap_storage: 'none' is a valid storage preset profile (#86) Signed-off-by: James Smith <james@foxglove.dev> bloom: add changelog changes 0.6.0 * ci: include rosbag2_storage_mcap Signed-off-by: James Smith <james@foxglove.dev> * package.xml: include ROS Tooling WG maintainers Signed-off-by: James Smith <james@foxglove.dev> * rosbag2_storage_mcap: update readme after move Signed-off-by: James Smith <james@foxglove.dev> Signed-off-by: James Smith <james@foxglove.dev>

* rosbag2_storage_mcap: merge into ros2/rosbag2 Signed-off-by: James Smith <james@foxglove.dev> mcap_storage: 'none' is a valid storage preset profile (ros2#86) Signed-off-by: James Smith <james@foxglove.dev> bloom: add changelog changes 0.6.0 * ci: include rosbag2_storage_mcap Signed-off-by: James Smith <james@foxglove.dev> * package.xml: include ROS Tooling WG maintainers Signed-off-by: James Smith <james@foxglove.dev> * rosbag2_storage_mcap: update readme after move Signed-off-by: James Smith <james@foxglove.dev> Signed-off-by: James Smith <james@foxglove.dev>

…1163) * rosbag2_storage_mcap: merge into ros2/rosbag2 Signed-off-by: James Smith <james@foxglove.dev> mcap_storage: 'none' is a valid storage preset profile (ros2#86) Signed-off-by: James Smith <james@foxglove.dev> bloom: add changelog changes 0.6.0 * ci: include rosbag2_storage_mcap Signed-off-by: James Smith <james@foxglove.dev> * package.xml: include ROS Tooling WG maintainers Signed-off-by: James Smith <james@foxglove.dev> * rosbag2_storage_mcap: update readme after move Signed-off-by: James Smith <james@foxglove.dev> Signed-off-by: James Smith <james@foxglove.dev>

…os2#1163) * rosbag2_storage_mcap: merge into ros2/rosbag2 Signed-off-by: James Smith <james@foxglove.dev> mcap_storage: 'none' is a valid storage preset profile (ros2#86) Signed-off-by: James Smith <james@foxglove.dev> bloom: add changelog changes 0.6.0 * ci: include rosbag2_storage_mcap Signed-off-by: James Smith <james@foxglove.dev> * package.xml: include ROS Tooling WG maintainers Signed-off-by: James Smith <james@foxglove.dev> * rosbag2_storage_mcap: update readme after move Signed-off-by: James Smith <james@foxglove.dev> Signed-off-by: James Smith <james@foxglove.dev>

… (#1189) * [backport] rosbag2_storage_mcap: merge into rosbag2 repo (#1163) * rosbag2_storage_mcap: merge into ros2/rosbag2 Signed-off-by: James Smith <james@foxglove.dev> mcap_storage: 'none' is a valid storage preset profile (#86) Signed-off-by: James Smith <james@foxglove.dev> bloom: add changelog changes 0.6.0 * ci: include rosbag2_storage_mcap Signed-off-by: James Smith <james@foxglove.dev> * package.xml: include ROS Tooling WG maintainers Signed-off-by: James Smith <james@foxglove.dev> * rosbag2_storage_mcap: update readme after move Signed-off-by: James Smith <james@foxglove.dev> Signed-off-by: James Smith <james@foxglove.dev> * zstd_vendor: do not remove zstd_errors.h Signed-off-by: James Smith <james@foxglove.dev> Signed-off-by: James Smith <james@foxglove.dev>

… (#1189) * [backport] rosbag2_storage_mcap: merge into rosbag2 repo (#1163) * rosbag2_storage_mcap: merge into ros2/rosbag2 Signed-off-by: James Smith <james@foxglove.dev> mcap_storage: 'none' is a valid storage preset profile (#86) Signed-off-by: James Smith <james@foxglove.dev> bloom: add changelog changes 0.6.0 * ci: include rosbag2_storage_mcap Signed-off-by: James Smith <james@foxglove.dev> * package.xml: include ROS Tooling WG maintainers Signed-off-by: James Smith <james@foxglove.dev> * rosbag2_storage_mcap: update readme after move Signed-off-by: James Smith <james@foxglove.dev> Signed-off-by: James Smith <james@foxglove.dev> * zstd_vendor: do not remove zstd_errors.h Signed-off-by: James Smith <james@foxglove.dev> Signed-off-by: James Smith <james@foxglove.dev> (cherry picked from commit 953c8ed) # Conflicts: # .github/workflows/test.yml

… (#1189) * [backport] rosbag2_storage_mcap: merge into rosbag2 repo (#1163) * rosbag2_storage_mcap: merge into ros2/rosbag2 Signed-off-by: James Smith <james@foxglove.dev> mcap_storage: 'none' is a valid storage preset profile (#86) Signed-off-by: James Smith <james@foxglove.dev> bloom: add changelog changes 0.6.0 * ci: include rosbag2_storage_mcap Signed-off-by: James Smith <james@foxglove.dev> * package.xml: include ROS Tooling WG maintainers Signed-off-by: James Smith <james@foxglove.dev> * rosbag2_storage_mcap: update readme after move Signed-off-by: James Smith <james@foxglove.dev> Signed-off-by: James Smith <james@foxglove.dev> * zstd_vendor: do not remove zstd_errors.h Signed-off-by: James Smith <james@foxglove.dev> Signed-off-by: James Smith <james@foxglove.dev> (cherry picked from commit 953c8ed) Signed-off-by: James Smith <james@foxglove.dev>

* rosbag2_storage_mcap: merge into ros2/rosbag2 Signed-off-by: James Smith <james@foxglove.dev> mcap_storage: 'none' is a valid storage preset profile (ros2#86) Signed-off-by: James Smith <james@foxglove.dev> bloom: add changelog changes 0.6.0 * ci: include rosbag2_storage_mcap Signed-off-by: James Smith <james@foxglove.dev> * package.xml: include ROS Tooling WG maintainers Signed-off-by: James Smith <james@foxglove.dev> * rosbag2_storage_mcap: update readme after move Signed-off-by: James Smith <james@foxglove.dev> Signed-off-by: James Smith <james@foxglove.dev> Signed-off-by: Ricardo Manríquez <ricardo.manriquez+gh@gmail.com>

… (#1189) * [backport] rosbag2_storage_mcap: merge into rosbag2 repo (#1163) * rosbag2_storage_mcap: merge into ros2/rosbag2 Signed-off-by: James Smith <james@foxglove.dev> mcap_storage: 'none' is a valid storage preset profile (#86) Signed-off-by: James Smith <james@foxglove.dev> bloom: add changelog changes 0.6.0 * ci: include rosbag2_storage_mcap Signed-off-by: James Smith <james@foxglove.dev> * package.xml: include ROS Tooling WG maintainers Signed-off-by: James Smith <james@foxglove.dev> * rosbag2_storage_mcap: update readme after move Signed-off-by: James Smith <james@foxglove.dev> Signed-off-by: James Smith <james@foxglove.dev> * zstd_vendor: do not remove zstd_errors.h Signed-off-by: James Smith <james@foxglove.dev> Signed-off-by: James Smith <james@foxglove.dev> (cherry picked from commit 953c8ed) Signed-off-by: James Smith <james@foxglove.dev>

…#1198) * [Humble backport] rosbag2_storage_mcap: merge into rosbag2 repo (#1163) (#1189) * [backport] rosbag2_storage_mcap: merge into rosbag2 repo (#1163) * rosbag2_storage_mcap: merge into ros2/rosbag2 Signed-off-by: James Smith <james@foxglove.dev> mcap_storage: 'none' is a valid storage preset profile (#86) Signed-off-by: James Smith <james@foxglove.dev> bloom: add changelog changes 0.6.0 * ci: include rosbag2_storage_mcap Signed-off-by: James Smith <james@foxglove.dev> * package.xml: include ROS Tooling WG maintainers Signed-off-by: James Smith <james@foxglove.dev> * rosbag2_storage_mcap: update readme after move Signed-off-by: James Smith <james@foxglove.dev> Signed-off-by: James Smith <james@foxglove.dev> * zstd_vendor: do not remove zstd_errors.h Signed-off-by: James Smith <james@foxglove.dev> Signed-off-by: James Smith <james@foxglove.dev> (cherry picked from commit 953c8ed) Signed-off-by: James Smith <james@foxglove.dev> * rosbag2_storage_mcap: foxy now creates bag dir Signed-off-by: James Smith <james@foxglove.dev> * Use mcap tarball rather than git clone (#1200) This avoids git lfs quota issues Signed-off-by: Michael Carroll <michael@openrobotics.org> Signed-off-by: Michael Carroll <michael@openrobotics.org> * mcap_vendor: install only public headers Signed-off-by: James Smith <james@foxglove.dev> move --------- Signed-off-by: James Smith <james@foxglove.dev> Signed-off-by: Michael Carroll <michael@openrobotics.org> Co-authored-by: james-rms <james@foxglove.dev> Co-authored-by: Michael Carroll <michael@openrobotics.org>

Karsten1987 self-assigned this Feb 6, 2019

Karsten1987 added the in progress label Feb 6, 2019

Karsten1987 mentioned this pull request Feb 12, 2019

use globally defined varible for cppcheck include dirs ament/ament_lint#125

Merged

Karsten1987 added 2 commits February 27, 2019 11:29

enforce unique node names

ed8442c

fix cppcheck

5ddf9c0

Karsten1987 force-pushed the fix_rcl_375 branch from bdbb32b to 5ddf9c0 Compare February 27, 2019 19:31

Karsten1987 added in review and removed in progress labels Feb 27, 2019

jacobperron approved these changes Feb 27, 2019

View reviewed changes

Karsten1987 merged commit 1a3782d into master Feb 27, 2019

Karsten1987 deleted the fix_rcl_375 branch February 27, 2019 21:23

Karsten1987 removed the in review label Feb 27, 2019

jpsamper2009 mentioned this pull request Mar 20, 2019

Allow excluding checkers from ament_lint_auto_find_test_dependencies ament/ament_lint#132

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enforce unique node names#86

enforce unique node names#86
Karsten1987 merged 2 commits intomasterfrom
fix_rcl_375

Karsten1987 commented Feb 6, 2019

Uh oh!

Karsten1987 commented Feb 6, 2019

Uh oh!

Karsten1987 commented Feb 6, 2019

Uh oh!

jacobperron commented Feb 6, 2019

Uh oh!

dirk-thomas commented Feb 6, 2019

Uh oh!

Karsten1987 commented Feb 6, 2019

Uh oh!

jacobperron commented Feb 6, 2019 •

edited

Loading

Uh oh!

Karsten1987 commented Feb 27, 2019

Uh oh!

Karsten1987 commented Feb 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Karsten1987 commented Feb 6, 2019

Uh oh!

Karsten1987 commented Feb 6, 2019

Uh oh!

Karsten1987 commented Feb 6, 2019

Uh oh!

jacobperron commented Feb 6, 2019

Uh oh!

dirk-thomas commented Feb 6, 2019

Uh oh!

Karsten1987 commented Feb 6, 2019

Uh oh!

jacobperron commented Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Karsten1987 commented Feb 27, 2019

Uh oh!

Karsten1987 commented Feb 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jacobperron commented Feb 6, 2019 •

edited

Loading