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 <[email protected]>

mcap_storage: 'none' is a valid storage preset profile (ros2#86)

Signed-off-by: James Smith <[email protected]>

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 <[email protected]>

mcap_storage: 'none' is a valid storage preset profile (#86)

Signed-off-by: James Smith <[email protected]>

bloom: add changelog changes

0.6.0

* ci: include rosbag2_storage_mcap

Signed-off-by: James Smith <[email protected]>

* package.xml: include ROS Tooling WG maintainers

Signed-off-by: James Smith <[email protected]>

* rosbag2_storage_mcap: update readme after move

Signed-off-by: James Smith <[email protected]>

Signed-off-by: James Smith <[email protected]>
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 <[email protected]>

mcap_storage: 'none' is a valid storage preset profile (ros2#86)

Signed-off-by: James Smith <[email protected]>

bloom: add changelog changes

0.6.0

* ci: include rosbag2_storage_mcap

Signed-off-by: James Smith <[email protected]>

* package.xml: include ROS Tooling WG maintainers

Signed-off-by: James Smith <[email protected]>

* rosbag2_storage_mcap: update readme after move

Signed-off-by: James Smith <[email protected]>

Signed-off-by: James Smith <[email protected]>
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 <[email protected]>

mcap_storage: 'none' is a valid storage preset profile (ros2#86)

Signed-off-by: James Smith <[email protected]>

bloom: add changelog changes

0.6.0

* ci: include rosbag2_storage_mcap

Signed-off-by: James Smith <[email protected]>

* package.xml: include ROS Tooling WG maintainers

Signed-off-by: James Smith <[email protected]>

* rosbag2_storage_mcap: update readme after move

Signed-off-by: James Smith <[email protected]>

Signed-off-by: James Smith <[email protected]>
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 <[email protected]>

mcap_storage: 'none' is a valid storage preset profile (ros2#86)

Signed-off-by: James Smith <[email protected]>

bloom: add changelog changes

0.6.0

* ci: include rosbag2_storage_mcap

Signed-off-by: James Smith <[email protected]>

* package.xml: include ROS Tooling WG maintainers

Signed-off-by: James Smith <[email protected]>

* rosbag2_storage_mcap: update readme after move

Signed-off-by: James Smith <[email protected]>

Signed-off-by: James Smith <[email protected]>
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 <[email protected]>

mcap_storage: 'none' is a valid storage preset profile (#86)

Signed-off-by: James Smith <[email protected]>

bloom: add changelog changes

0.6.0

* ci: include rosbag2_storage_mcap

Signed-off-by: James Smith <[email protected]>

* package.xml: include ROS Tooling WG maintainers

Signed-off-by: James Smith <[email protected]>

* rosbag2_storage_mcap: update readme after move

Signed-off-by: James Smith <[email protected]>

Signed-off-by: James Smith <[email protected]>

* zstd_vendor: do not remove zstd_errors.h

Signed-off-by: James Smith <[email protected]>

Signed-off-by: James Smith <[email protected]>
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 <[email protected]>

mcap_storage: 'none' is a valid storage preset profile (#86)

Signed-off-by: James Smith <[email protected]>

bloom: add changelog changes

0.6.0

* ci: include rosbag2_storage_mcap

Signed-off-by: James Smith <[email protected]>

* package.xml: include ROS Tooling WG maintainers

Signed-off-by: James Smith <[email protected]>

* rosbag2_storage_mcap: update readme after move

Signed-off-by: James Smith <[email protected]>

Signed-off-by: James Smith <[email protected]>

* zstd_vendor: do not remove zstd_errors.h

Signed-off-by: James Smith <[email protected]>

Signed-off-by: James Smith <[email protected]>
(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 <[email protected]>

mcap_storage: 'none' is a valid storage preset profile (#86)

Signed-off-by: James Smith <[email protected]>

bloom: add changelog changes

0.6.0

* ci: include rosbag2_storage_mcap

Signed-off-by: James Smith <[email protected]>

* package.xml: include ROS Tooling WG maintainers

Signed-off-by: James Smith <[email protected]>

* rosbag2_storage_mcap: update readme after move

Signed-off-by: James Smith <[email protected]>

Signed-off-by: James Smith <[email protected]>

* zstd_vendor: do not remove zstd_errors.h

Signed-off-by: James Smith <[email protected]>

Signed-off-by: James Smith <[email protected]>
(cherry picked from commit 953c8ed)
Signed-off-by: James Smith <[email protected]>
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 <[email protected]>

mcap_storage: 'none' is a valid storage preset profile (#86)

Signed-off-by: James Smith <[email protected]>

bloom: add changelog changes

0.6.0

* ci: include rosbag2_storage_mcap

Signed-off-by: James Smith <[email protected]>

* package.xml: include ROS Tooling WG maintainers

Signed-off-by: James Smith <[email protected]>

* rosbag2_storage_mcap: update readme after move

Signed-off-by: James Smith <[email protected]>

Signed-off-by: James Smith <[email protected]>

* zstd_vendor: do not remove zstd_errors.h

Signed-off-by: James Smith <[email protected]>

Signed-off-by: James Smith <[email protected]>
(cherry picked from commit 953c8ed)
Signed-off-by: James Smith <[email protected]>
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 <[email protected]>

mcap_storage: 'none' is a valid storage preset profile (ros2#86)

Signed-off-by: James Smith <[email protected]>

bloom: add changelog changes

0.6.0

* ci: include rosbag2_storage_mcap

Signed-off-by: James Smith <[email protected]>

* package.xml: include ROS Tooling WG maintainers

Signed-off-by: James Smith <[email protected]>

* rosbag2_storage_mcap: update readme after move

Signed-off-by: James Smith <[email protected]>

Signed-off-by: James Smith <[email protected]>
Signed-off-by: Ricardo Manríquez <[email protected]>
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 <[email protected]>

mcap_storage: 'none' is a valid storage preset profile (#86)

Signed-off-by: James Smith <[email protected]>

bloom: add changelog changes

0.6.0

* ci: include rosbag2_storage_mcap

Signed-off-by: James Smith <[email protected]>

* package.xml: include ROS Tooling WG maintainers

Signed-off-by: James Smith <[email protected]>

* rosbag2_storage_mcap: update readme after move

Signed-off-by: James Smith <[email protected]>

Signed-off-by: James Smith <[email protected]>

* zstd_vendor: do not remove zstd_errors.h

Signed-off-by: James Smith <[email protected]>

Signed-off-by: James Smith <[email protected]>
(cherry picked from commit 953c8ed)
Signed-off-by: James Smith <[email protected]>
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 <[email protected]>

mcap_storage: 'none' is a valid storage preset profile (#86)

Signed-off-by: James Smith <[email protected]>

bloom: add changelog changes

0.6.0

* ci: include rosbag2_storage_mcap

Signed-off-by: James Smith <[email protected]>

* package.xml: include ROS Tooling WG maintainers

Signed-off-by: James Smith <[email protected]>

* rosbag2_storage_mcap: update readme after move

Signed-off-by: James Smith <[email protected]>

Signed-off-by: James Smith <[email protected]>

* zstd_vendor: do not remove zstd_errors.h

Signed-off-by: James Smith <[email protected]>

Signed-off-by: James Smith <[email protected]>
(cherry picked from commit 953c8ed)
Signed-off-by: James Smith <[email protected]>

* rosbag2_storage_mcap: foxy now creates bag dir

Signed-off-by: James Smith <[email protected]>

* Use mcap tarball rather than git clone (#1200)

This avoids git lfs quota issues

Signed-off-by: Michael Carroll <[email protected]>

Signed-off-by: Michael Carroll <[email protected]>

* mcap_vendor: install only public headers

Signed-off-by: James Smith <[email protected]>

move

---------

Signed-off-by: James Smith <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
Co-authored-by: james-rms <[email protected]>
Co-authored-by: Michael Carroll <[email protected]>
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