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

Factor out basic cmake common code. #536

Merged
merged 2 commits into from
Feb 5, 2019

Conversation

crdelsey
Copy link
Contributor

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.

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

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

@crdelsey
Copy link
Contributor Author

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

@crdelsey
Copy link
Contributor Author

crdelsey commented Jan 31, 2019

@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.

@crdelsey crdelsey merged commit 3f65576 into ros-navigation:master Feb 5, 2019
savalena pushed a commit to savalena/navigation2 that referenced this pull request Jul 5, 2024
* Factor out basic cmake common code.

* Fixing the package name so it can include other common things in the future.
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.

4 participants