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

enforce unique node names #86

Merged
merged 2 commits into from
Feb 27, 2019
Merged

enforce unique node names #86

merged 2 commits into from
Feb 27, 2019

Conversation

Karsten1987
Copy link
Collaborator

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 Karsten1987 self-assigned this Feb 6, 2019
@Karsten1987
Copy link
Collaborator Author

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

@Karsten1987
Copy link
Collaborator Author

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

@jacobperron
Copy link
Member

@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
Copy link
Member

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
Copy link
Collaborator Author

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
Copy link
Member

jacobperron commented Feb 6, 2019

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
Copy link
Collaborator Author

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

@Karsten1987
Copy link
Collaborator Author

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.

@Karsten1987 Karsten1987 merged commit 1a3782d into master Feb 27, 2019
@Karsten1987 Karsten1987 deleted the fix_rcl_375 branch February 27, 2019 21:23
james-rms added a commit to james-rms/rosbag2 that referenced this pull request Nov 28, 2022
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
james-rms added a commit that referenced this pull request Nov 29, 2022
* 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>
james-rms added a commit to james-rms/rosbag2 that referenced this pull request Nov 29, 2022
* 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>
james-rms added a commit to james-rms/rosbag2 that referenced this pull request Nov 29, 2022
…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>
james-rms added a commit to james-rms/rosbag2 that referenced this pull request Nov 29, 2022
…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>
james-rms added a commit that referenced this pull request Dec 1, 2022
… (#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>
mergify bot pushed a commit that referenced this pull request Dec 7, 2022
… (#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
james-rms added a commit that referenced this pull request Dec 7, 2022
… (#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>
james-rms added a commit that referenced this pull request Dec 7, 2022
… (#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>
ricardo-manriquez pushed a commit to ricardo-manriquez/rosbag2 that referenced this pull request Dec 7, 2022
* 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>
MichaelOrlov pushed a commit that referenced this pull request May 23, 2023
… (#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>
MichaelOrlov pushed a commit that referenced this pull request May 23, 2023
…#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>
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.

3 participants