Skip to content

Factor out basic cmake common code.#536

Merged
2 commits merged intomasterfrom
unknown repository
Feb 5, 2019
Merged

Factor out basic cmake common code.#536
2 commits merged intomasterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Jan 25, 2019

Basic Info

Info Please fill out this column
Ticket(s) this addresses
Primary OS tested on Ubuntu
Robotic platform tested on NA

Description of contribution in a few bullet points

  • There is duplicated code across CMakeLists.txt files throughout the project. It will get even worse as we add code coverage capabilities. This PR creates a package where we can put common cmake functions and macros to reuse across packages.
  • I factored out the common preamble code across all the packages as an inital use of this package.
  • The dynamic_parameters package had a special case pre-amble that disabled the sign-compare warning. I re-enabled that warning and fixed the sign-compare issues in the code.
  • The costmap_2d package had a special case pre-amble that disabled all warnings. I re-enabled the warnings, however, there are more complicated fixes in the code, so I disabled the specific warnings that were coming up, leaving the actual fix for a future PR.

@sloretz sloretz mentioned this pull request Jan 28, 2019
Merged
mhpanah
mhpanah approved these changes Jan 29, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@mhpanah mhpanah left a comment

Choose a reason for hiding this comment

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

LGTM.

@orduno orduno self-requested a review January 29, 2019 23:50
orduno
orduno approved these changes Jan 29, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@orduno orduno left a comment

Choose a reason for hiding this comment

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

Looks good.

Alternatively to creating a new package we could have common functionality in navigation2 package.

@mkhansenbot
Copy link
Copy Markdown
Collaborator

mkhansenbot commented Jan 30, 2019

Just wondering if @SteveMacenski knows of any other way to do this besides creating a package just for common CMakeLists code?

@ghost
Copy link
Copy Markdown
Author

ghost commented Jan 30, 2019

@orduno We can't put this in the navigation2 package, or else we'd have circular dependencies.

@ghost
Copy link
Copy Markdown
Author

ghost commented Jan 31, 2019 •
edited by ghost
Loading

@mkhansen-intel Here are alternatives I can see:

  1. Don't factor out the common CMake code, and instead copy it in each package.
  2. Put the common code under /tools and then symlink it into each package.
  3. Refactor the common code out to some other package we depend on, eg one of the ament_cmake packages
  4. Refactor the common code out to a new package not associated with the nav2 stack.

Number 1 is a viable option. It trades off reduced package count against violating the DRY principle.

Number 2 is viable on Linux but will make it very difficult to ever checkout and build on Windows.

Number 3 is an option so long as our changes are not very Nav2 specific, but depends on convincing OSRF to accept them upstream.

Number 4 is not fundamentally different than this PR, except it moves the new package to another repo to hide it, and adds in the complexity of having another repo to manage and get integrated into the ROS2 release.

I believe I can now make the code coverage changes without actually changing the CMakeLists.txt, so I can close this and we can kick the can down the road till the next time something comes up.

@ghost ghost merged commit 3f65576 into ros-navigation:master Feb 5, 2019
@MostafaGomaa MostafaGomaa mentioned this pull request Jan 17, 2023
Closed
7 tasks
savalena pushed a commit to savalena/navigation2 that referenced this pull request Jul 5, 2024
Factor out basic cmake common code. (ros-navigation#536)
8eddeed 
* Factor out basic cmake common code.

* Fixing the package name so it can include other common things in the future.
mini-1235 pushed a commit to mini-1235/navigation2 that referenced this pull request Feb 5, 2025
@FiIipe @FilipeCerveira
Update the documentation to use composable nodes with keepout filters (…
a2cd220 
…ros-navigation#536)

Co-authored-by: Filipe Pinto <filipe.pinto@globallogic.com>
Forsyth-Creations pushed a commit to Forsyth-Creations/navigation2 that referenced this pull request Feb 19, 2025
@dependabot
Bump actions/checkout from 1 to 3 (ros-navigation#536)
cfaa708 
Bumps [actions/checkout](https://github.com/actions/checkout) from 1 to 3.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v1...v3)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mkhansenbot @orduno @mhpanah