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

Unused header cleanup #1627

Merged
merged 8 commits into from
Jul 21, 2024

Conversation

henrygerardmoore
Copy link
Contributor

@henrygerardmoore henrygerardmoore commented Jul 19, 2024

Follow-on for my other PR, another one just removing unused headers. Also removes some unused using declarations, and a test (test_transmission_parser.cpp) that wasn't being built and was including a file that was removed a while ago in #633. Also standardizes to use braced includes (e.g. <gmock/gmock.h>) as opposed to using quotes for gtest and gmock.

Tests pass locally, no noticeable build time improvements (so this is just a style change). The removed test accounts for the vast majority of the lines removed at 441 lines (don't worry 😄)

Copy link
Contributor

@olivier-stasse olivier-stasse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks fine, but before approving the 13 workflows should be approved by one of the maintainers to see what the builders say.

Copy link

codecov bot commented Jul 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.60%. Comparing base (b7bbdd6) to head (c9231ad).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1627   +/-   ##
=======================================
  Coverage   87.60%   87.60%           
=======================================
  Files         108      108           
  Lines        9918     9918           
  Branches      889      889           
=======================================
  Hits         8689     8689           
  Misses        912      912           
  Partials      317      317           
Flag Coverage Δ
unittests 87.60% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...lude/controller_interface/controller_interface.hpp 100.00% <ø> (ø)
...controller_interface/controller_interface_base.hpp 87.50% <ø> (ø)
controller_interface/src/controller_interface.cpp 75.00% <ø> (ø)
...roller_interface/src/controller_interface_base.cpp 86.27% <ø> (ø)
...rface/test/test_chainable_controller_interface.hpp 86.95% <ø> (ø)
...oller_interface/test/test_controller_interface.hpp 57.14% <ø> (ø)
...er_interface/test/test_controller_with_options.cpp 100.00% <ø> (ø)
...er_interface/test/test_controller_with_options.hpp 47.05% <ø> (ø)
...roller_interface/test/test_force_torque_sensor.hpp 100.00% <ø> (ø)
controller_interface/test/test_imu_sensor.hpp 100.00% <ø> (ø)
... and 64 more

Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the cleanup (again!). The failing tests are not related to this PR and already reported.

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the cleanup!!

Copy link
Contributor

@olivier-stasse olivier-stasse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@bmagyar bmagyar merged commit 6631270 into ros-controls:master Jul 21, 2024
18 of 19 checks passed
@christophfroehlich
Copy link
Contributor

christophfroehlich commented Jul 21, 2024

@henrygerardmoore @bmagyar Merging this breaks lots of packages in ros2_controllers and ros2_control_demos repo, because there are now lots of missing includes. I fix ros2_controllers, but we might break lots of other downstream packages.

@henrygerardmoore
Copy link
Contributor Author

@christophfroehlich I can make a PR for ros2_control_demos, are there any others that you know of?

@christophfroehlich
Copy link
Contributor

christophfroehlich commented Jul 21, 2024

@christophfroehlich I can make a PR for ros2_control_demos, are there any others that you know of?

I created one for ros2_controllers and for ros2_control_demos, the other packages of ros2_control stack build still fine. I only fear the next release on the build farm, but we'll see.

apt-cache rdepends ros-rolling-controller-interface
ros-rolling-controller-interface
Reverse Depends:
  ros-rolling-ur-controllers
  ros-rolling-robotiq-controllers
  ros-rolling-picknik-twist-controller
  ros-rolling-picknik-reset-fault-controller
  ros-rolling-canopen-ros2-controllers
apt-cache rdepends ros-rolling-hardware-interface
ros-rolling-hardware-interface
Reverse Depends:
  ros-rolling-ur-robot-driver
  ros-rolling-topic-based-ros2-control
  ros-rolling-kortex-driver
  ros-rolling-dynamixel-hardware
  ros-rolling-canopen-ros2-controllers
  ros-rolling-canopen-ros2-control

@henrygerardmoore
Copy link
Contributor Author

Sorry for the trouble, was there a better way I should have gone about the unused header cleanup?

@henrygerardmoore henrygerardmoore deleted the unused_header_cleanup branch July 21, 2024 18:33
@christophfroehlich
Copy link
Contributor

I think that you did a good job, the only easy thing would have been to compile the obvious dependencies (ros2_controllers for example).
Packages not following the "include what you use" paradigm will have problems now, this is fine for rolling now 😁

christophfroehlich added a commit that referenced this pull request Aug 12, 2024
* [ResourceManager] Make destructor virtual for use in derived classes (#1607)

* Fix typos in test_resource_manager.cpp (#1609)

* [CM] Remove support for the description parameter and use only `robot_description` topic (#1358)

---------

Co-authored-by: Felix Exner <[email protected]>
Co-authored-by: Christoph Froehlich <[email protected]>

* move critical variables to the private context (#1623)

* Fix controller starting with later load of robot description test (#1624)

* Fix the duplicated restart of the controller_manager services initialization

* Scope the ControllerManagerRunner to avoid malloc and other test issues

* reorder the tests for consistent log at the end

* Remove noqa (#1626)

* Unused header cleanup (#1627)

* Create debugging.rst (#1625)


---------

Co-authored-by: Sai Kishor Kothakota <[email protected]>
Co-authored-by: Christoph Froehlich <[email protected]>

* Update changelogs

* 4.14.0

* add missing rclcpp logging include for Humble compatibility build (#1635)

* [CM] Remove deprecated spawner args (#1639)

* Add a pytest launch file to test ros2_control_node (#1636)

* Fix rst markup (#1642)

* Fix rqt_cm paragraph

* Fix indent

* CM: Add missing includes (#1641)

* [RM] Add `get_hardware_info` method to the Hardware Components (#1643)

* Fix the namespaced controller_manager spawner + added tests (#1640)

* Bump version of pre-commit hooks (#1649)

Co-authored-by: christophfroehlich <[email protected]>

* Add missing include for executors (#1653)

* Update changelogs

* 4.15.0

* refactor SwitchParams to fix the incosistencies in the spawner tests (#1638)

* Modify test with missing CM to have a timeout

* Catch exception when CM services are not found

And print the error and exit in the application

* Exit with code 1 on unreached CM

---------

Co-authored-by: Sai Kishor Kothakota <[email protected]>
Co-authored-by: Parker Drouillard <[email protected]>
Co-authored-by: Dr. Denis <[email protected]>
Co-authored-by: Christoph Froehlich <[email protected]>
Co-authored-by: Christoph Fröhlich <[email protected]>
Co-authored-by: Henry Moore <[email protected]>
Co-authored-by: Lennart Nachtigall <[email protected]>
Co-authored-by: Sai Kishor Kothakota <[email protected]>
Co-authored-by: Bence Magyar <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: christophfroehlich <[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.

5 participants