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

unify CMakeLists.txt files #381

Merged
merged 1 commit into from
Aug 6, 2024
Merged

Conversation

KmakD
Copy link
Contributor

@KmakD KmakD commented Aug 5, 2024

Description

[Summary of the changes]

Modifications

Summary by CodeRabbit

  • New Features

    • Consolidated package dependency management across multiple projects for improved maintainability.
    • Enhanced clarity in installation commands and structure for configuration files.
  • Bug Fixes

    • Resolved redundancy in dependency declarations, ensuring a more efficient build configuration.
  • Refactor

    • Renamed and restructured variables for consistency in dependency management throughout various projects.
    • Updated include directory specifications to enhance modularity and reusability in test targets.
  • Chores

    • Improved readability and organization of CMake configurations across projects.

Copy link
Contributor

coderabbitai bot commented Aug 5, 2024

Walkthrough

The recent changes across the panther project involve significant enhancements to the CMake configuration, focusing on streamlining package dependency management. This includes consolidating individual find_package calls into a single variable for dependencies, improving the organization of installation directives, and ensuring better visibility of include directories. Overall, these modifications enhance maintainability and clarity without altering the fundamental build processes.

Changes

Files Group Summary of Changes
panther_battery/CMakeLists.txt Introduced PACKAGE_DEPENDENCIES variable for consolidated dependency management, updated targets to use this variable.
panther_controller/CMakeLists.txt Changed the installation order of DIRECTORY parameters in the install command.
panther_description/CMakeLists.txt Added ament_package() to declare the package, improving integration within ROS.
panther_diagnostics/CMakeLists.txt Renamed PACKAGE_INCLUDE_DEPENDS to PACKAGE_DEPENDENCIES, modified dependency order, and enhanced target_include_directories.
panther_diagnostics/cmake/SuperBuild.cmake Reordered INSTALL_DIR declaration for better clarity in the ExternalProject_Add function.
panther_gazebo/CMakeLists.txt Consolidated dependency declarations into a loop, removed duplicate dependency in ament_target_dependencies.
panther_hardware_interfaces/CMakeLists.txt Renamed PACKAGE_INCLUDE_DEPENDS to PACKAGE_DEPENDENCIES, improved visibility of include directories for test targets.
panther_lights/CMakeLists.txt Consolidated dependency management using PACKAGE_DEPENDENCIES, structured installation commands for clarity.
panther_localization/CMakeLists.txt Swapped installation order of launch and config directories.
panther_manager/CMakeLists.txt Renamed and updated handling of dependencies, improved target_include_directories logic for flexibility.
panther_utils/CMakeLists.txt Streamlined dependency handling with PACKAGE_DEPENDENCIES, reordered installation commands for better organization.

Sequence Diagram(s)

sequenceDiagram
    participant CMake as CMakeLists.txt
    participant FindPkg as find_package
    participant Target as Target Executable
    participant Install as Install
    participant Include as Include Directories

    CMake->>FindPkg: Define PACKAGE_DEPENDENCIES
    loop for each dependency
        FindPkg->>CMake: find_package(dependency)
    end
    CMake->>Target: ament_target_dependencies(Target, PACKAGE_DEPENDENCIES)
    CMake->>Include: ament_export_include_directories(include)
    CMake->>Install: install(DIRECTORY ...)
Loading

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 5d210e9 and 68a51df.

Files selected for processing (11)
  • panther_battery/CMakeLists.txt (8 hunks)
  • panther_controller/CMakeLists.txt (1 hunks)
  • panther_description/CMakeLists.txt (1 hunks)
  • panther_diagnostics/CMakeLists.txt (2 hunks)
  • panther_diagnostics/cmake/SuperBuild.cmake (1 hunks)
  • panther_gazebo/CMakeLists.txt (2 hunks)
  • panther_hardware_interfaces/CMakeLists.txt (12 hunks)
  • panther_lights/CMakeLists.txt (3 hunks)
  • panther_localization/CMakeLists.txt (1 hunks)
  • panther_manager/CMakeLists.txt (6 hunks)
  • panther_utils/CMakeLists.txt (2 hunks)
Files skipped from review due to trivial changes (5)
  • panther_battery/CMakeLists.txt
  • panther_controller/CMakeLists.txt
  • panther_diagnostics/cmake/SuperBuild.cmake
  • panther_gazebo/CMakeLists.txt
  • panther_localization/CMakeLists.txt
Additional comments not posted (21)
panther_description/CMakeLists.txt (1)

11-11: LGTM! The addition of ament_package() is correct.

The inclusion of ament_package() enhances the package's integration within the ROS framework.

panther_diagnostics/CMakeLists.txt (4)

18-27: LGTM! The renaming and restructuring of dependencies are correct.

The changes improve clarity and maintainability by consolidating dependencies into a single variable.


29-30: LGTM! The update to the foreach loop is correct.

The loop now iterates over PACKAGE_DEPENDENCIES, aligning with the renamed variable.


46-46: LGTM! The update to ament_target_dependencies is correct.

The call now uses the renamed PACKAGE_DEPENDENCIES variable.


64-64: LGTM! The update to ament_target_dependencies in the test block is correct.

The call now uses the renamed PACKAGE_DEPENDENCIES variable.

panther_utils/CMakeLists.txt (3)

8-15: LGTM! The creation of PACKAGE_DEPENDENCIES is correct.

The consolidation of dependencies into a single variable improves clarity and maintainability.


17-19: LGTM! The update to the foreach loop is correct.

The loop now iterates over PACKAGE_DEPENDENCIES, aligning with the consolidated variable.


76-78: LGTM! The update to ament_python_install_package is correct.

The command now uses ${PROJECT_NAME}, enhancing flexibility and consistency.

panther_lights/CMakeLists.txt (3)

8-19: LGTM! Consolidation of package dependencies improves maintainability.

The use of a single variable to list all required packages enhances readability and maintainability.


21-23: LGTM! The foreach loop reduces redundancy.

Using a loop to call find_package for each package in PACKAGE_DEPENDENCIES enhances maintainability.


210-211: LGTM! Improved structure for export commands.

The structured format for exporting include directories and libraries enhances visibility and accessibility.

panther_hardware_interfaces/CMakeLists.txt (5)

Line range hint 18-37: LGTM! Renaming variable to reflect broader scope.

The renaming of PACKAGE_INCLUDE_DEPENDS to PACKAGE_DEPENDENCIES enhances clarity by reflecting a broader scope of dependencies.


38-39: LGTM! Consistent update of foreach loop.

Updating the foreach loop to iterate over PACKAGE_DEPENDENCIES ensures consistency with the new variable name.


42-42: LGTM! Ensuring necessary include paths.

Specifying include directories using include_directories(include) ensures that the necessary include paths are available during compilation.


67-67: LGTM! Correctly referencing updated dependency list.

Updating ament_target_dependencies to use PACKAGE_DEPENDENCIES ensures that the build system references the correct dependency list.


233-233: LGTM! Correctly referencing export dependencies.

Updating ament_export_dependencies to use PACKAGE_DEPENDENCIES ensures that the export dependencies are correctly referenced.

panther_manager/CMakeLists.txt (5)

Line range hint 8-21: LGTM! Renaming variable to reflect broader scope.

The renaming of PACKAGE_INCLUDE_DEPENDS to PACKAGE_DEPENDENCIES enhances clarity by reflecting a broader scope of dependencies.


22-23: LGTM! Consistent update of foreach loop.

Updating the foreach loop to iterate over PACKAGE_DEPENDENCIES ensures consistency with the new variable name.


60-60: LGTM! Correctly referencing updated dependency list.

Updating ament_target_dependencies to use PACKAGE_DEPENDENCIES ensures that the build system references the correct dependency list.


102-107: LGTM! Enhancing flexibility of include directories.

Modifying target_include_directories to include both build and install interface paths improves modularity and reusability.


157-157: LGTM! Correctly referencing updated dependency list for test targets.

Updating ament_target_dependencies to use PACKAGE_DEPENDENCIES ensures that the build system references the correct dependency list for the test targets.

Copy link
Contributor

@rafal-gorecki rafal-gorecki left a comment

Choose a reason for hiding this comment

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

PACKAGE_DEPENDENCIES in some files look like this:

dep1
dep2
dep3

And in other files:

dep1 dep2 dep3

@rafal-gorecki rafal-gorecki self-requested a review August 6, 2024 13:02
@rafal-gorecki rafal-gorecki merged commit de6d61a into ros2-devel Aug 6, 2024
@rafal-gorecki rafal-gorecki deleted the ros2-unify-cmake-files branch August 6, 2024 13:09
rafal-gorecki added a commit that referenced this pull request Aug 7, 2024
* New format of documentation  (#369)

* Change 3 package for demo

* Improve ROS_API

* fix links

* Update

* Update

* Table improvements

* Format

* Save work

* Save work

* update

* fix

* fix

* fix

* fix

* fix

* Add API warning

* Improve links

* lights simplify

* Create CONFIGURATION.md files

* Typos

* pre-commit

* Apply suggestions from code review

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* Save work

* Final unification

* Delete trash

* typos

* Update README.md

* Update ROS_API.md

* Update ROS_API.md

* Update README.md

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* Change initial warning to beta warning

* improve warn rendering

* rendering

* Update Diagram

* Add Dawid suggestions

* Dot

* Change diagram ext and typos

* Do not describe external nodes

* Add Dawid suggestons

* Add last Dawid suggestions

* Format

* Pawel suggestions

* Diagram improvements

* Update

* Diagram Visual

---------

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* Change scheme theme (#380)

* unify CMakeLists.txt files (#381)

* First working version

* Ros2 increase bt service timeout (#382)

* Parametrize and increase service timeout in managers

* Format panther API drawio file

* Add Estop GUI and docs

---------

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Paweł Irzyk <108666440+pawelirh@users.noreply.github.com>
Co-authored-by: Dawid Kmak <73443304+KmakD@users.noreply.github.com>
delihus added a commit that referenced this pull request Aug 28, 2024
* Implement testing POC

* Namespace refactor

* Add EStop to Gazebo

* unify CMakeLists.txt files

* Add dependencies

* Add remapping

* Rename files in panther_diagnostics package

* Update after changes in panther_diagnostics

* Rename config and launch file in manager package

* Correct include guards in manager package

* Restructure files tree in manager tests

* Ros2 estop sim gui (#384)

* New format of documentation  (#369)

* Change 3 package for demo

* Improve ROS_API

* fix links

* Update

* Update

* Table improvements

* Format

* Save work

* Save work

* update

* fix

* fix

* fix

* fix

* fix

* Add API warning

* Improve links

* lights simplify

* Create CONFIGURATION.md files

* Typos

* pre-commit

* Apply suggestions from code review

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* Save work

* Final unification

* Delete trash

* typos

* Update README.md

* Update ROS_API.md

* Update ROS_API.md

* Update README.md

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* Change initial warning to beta warning

* improve warn rendering

* rendering

* Update Diagram

* Add Dawid suggestions

* Dot

* Change diagram ext and typos

* Do not describe external nodes

* Add Dawid suggestons

* Add last Dawid suggestions

* Format

* Pawel suggestions

* Diagram improvements

* Update

* Diagram Visual

---------

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* Change scheme theme (#380)

* unify CMakeLists.txt files (#381)

* First working version

* Ros2 increase bt service timeout (#382)

* Parametrize and increase service timeout in managers

* Format panther API drawio file

* Add Estop GUI and docs

---------

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Paweł Irzyk <108666440+pawelirh@users.noreply.github.com>
Co-authored-by: Dawid Kmak <73443304+KmakD@users.noreply.github.com>

* Update panther_gazebo/panther_hardware_plugins.xml

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* Add david suggestion and change gui layout

* Typos in Readme + estop publish on service call

* Reorganize files in panther_lights

* UPdate include guards in panther_lights

* Reorganize files in panther_battery

* Move estop to plugins folder

* add nmea gps

* Rename battery driver files

* Rename shutdown hosts config

* Inherit from IgnitionSystem

* Change to Estop -> EStop

* Reorganize panther_hardware_interfaces files

* Dawid suggestions part 1

* Rename PantherSystem -> GzPantherSystem

* Update references to files

* Rename battery exec

* Fix links in documentations (#387)

* Refer to header files

* Update panther_gazebo/include/panther_gazebo/gz_panther_system.hpp

Co-authored-by: Dawid Kmak <73443304+KmakD@users.noreply.github.com>

* Update panther_gazebo/src/gz_panther_system.cpp

Co-authored-by: Dawid Kmak <73443304+KmakD@users.noreply.github.com>

* Update panther_gazebo/src/gz_panther_system.cpp

Co-authored-by: Dawid Kmak <73443304+KmakD@users.noreply.github.com>

* Dawid suggestions

* delete typo

* Minor modifications

* Move BT plugins to src directory

* Reorganize test utilities in hardware_interfaces

* Merge remote-tracking branch 'origin/ros2-devel' into ros2-testing-poc

* Add missing module configuration

* Update panther_gazebo/include/panther_gazebo/gz_panther_system.hpp

Co-authored-by: Dawid Kmak <73443304+KmakD@users.noreply.github.com>

* Dawid suggestions

* update docs

* Extend filesystem responsibility

* Update ROS_API.md

Co-authored-by: Paweł Irzyk <108666440+pawelirh@users.noreply.github.com>

* update names

* Add new common utility functions

* System monitor improvements

* Round temperature precision

* Implement filesystem unit tests

* Formatting

* Add integration tests condition

* Update ROS_API.md

* Review changes

* Add pre-commit workflow (#395)

---------

Co-authored-by: pawelirh <pawel.irzyk@husarion.com>
Co-authored-by: rafal-gorecki <rafal.gorecki@husarion.com>
Co-authored-by: Dawid <kmakd197@gmail.com>
Co-authored-by: rafal-gorecki <126687345+rafal-gorecki@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Paweł Irzyk <108666440+pawelirh@users.noreply.github.com>
Co-authored-by: Dawid Kmak <73443304+KmakD@users.noreply.github.com>
This pull request was closed.
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.

2 participants