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.

Sign up for GitHub

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

Jump to bottom

Improve CMake usage #1550

Merged
merged 5 commits into from
Sep 6, 2022
Merged

Improve CMake usage #1550

merged 5 commits into from
Sep 6, 2022

Conversation

sjahr
Copy link
Contributor

@sjahr sjahr commented Sep 5, 2022

Description

Apply some CMake recommendations from @ChrisThrasher + Update RUNTIME DESTINATION for some packages to bin
as recommended by the Ament-CMake docs

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@sjahr sjahr changed the title Pr update cmake version Improve CMake usage Sep 5, 2022
@sjahr sjahr requested a review from tylerjw September 5, 2022 09:24
@codecov
Copy link

codecov bot commented Sep 5, 2022 •
edited
Loading

Codecov Report

Merging #1550 (5b6201e) into main (3db960a) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1550      +/-   ##
==========================================
- Coverage   51.11%   51.10%   -0.00%     
==========================================
  Files         380      380              
  Lines       31802    31802              
==========================================
- Hits        16252    16249       -3     
- Misses      15550    15553       +3
Impacted Files Coverage Δ
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp 75.36% <0.00%> (-1.07%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ChrisThrasher
ChrisThrasher reviewed Sep 5, 2022
View reviewed changes
moveit_core/collision_detection_bullet/CMakeLists.txt Show resolved Hide resolved
moveit/CMakeLists.txt Outdated
@@ -1,4 +1,4 @@
cmake_minimum_required(VERSION 3.10.2)
cmake_minimum_required(VERSION 3.16.3)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the oldest Ubuntu LTS release we're expected to support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a REP about ROS 2 Releases and Target Platforms . I've chosen the minimal version based on that

Copy link
Contributor

@ChrisThrasher ChrisThrasher Sep 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

A single ROS 2 distribution will only have full Tier 1 support for a single Ubuntu LTS. The rationale is that fully supporting two LTS versions - which means 2-year-different versions of upstream dependencies - is a tremendous overhead and sometimes even impossible.

If we're only supporting Ubuntu 22 with this code then you can raise that CMake requirement to 3.22. I wasn't aware of us still doing any active maintenance on releases that target Ubuntu 20 except for maybe bug fix backports.

Copy link
Contributor Author

@sjahr sjahr Sep 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is actually a good point, I'll bump the version to 3.22. I don't think that we support focal with main anymore @henningkayser @vatanaksoytezer @JafarAbdi any objections?

Copy link
Member

Choose a reason for hiding this comment

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

You can safely bump this to 3.22, we don't support Focal on main or humble.

Copy link
Contributor

Choose a reason for hiding this comment

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

We almost only backport non API breaking bug fixes to Focal, I'm also happy with 3.22.

@henningkayser henningkayser added the backport-humble Mergify label that triggers a PR backport to Humble label Sep 6, 2022
ChrisThrasher
ChrisThrasher approved these changes Sep 6, 2022
View reviewed changes
Copy link
Contributor

@ChrisThrasher ChrisThrasher left a comment

Choose a reason for hiding this comment

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

Really glad to see modern CMake being used in MoveIt!

@vatanaksoytezer vatanaksoytezer merged commit 0faa583 into moveit:main Sep 6, 2022
@mergify mergify bot mentioned this pull request Sep 6, 2022
Merged
mergify bot pushed a commit that referenced this pull request Sep 6, 2022
@sjahr @mergify
Improve CMake usage (#1550)
00029f1 
(cherry picked from commit 0faa583)
@tylerjw tylerjw deleted the pr-update_cmake_version branch September 6, 2022 18:29
tylerjw pushed a commit that referenced this pull request Sep 6, 2022
@mergify @sjahr
Improve CMake usage (#1550) (#1555)
5a589fc 
Co-authored-by: Sebastian Jahr <sebastian.jahr@picknik.ai>
@ChrisThrasher ChrisThrasher mentioned this pull request Feb 22, 2023
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gavanderhoorn gavanderhoorn gavanderhoorn left review comments

@tylerjw tylerjw tylerjw approved these changes

@henningkayser henningkayser henningkayser approved these changes

@vatanaksoytezer vatanaksoytezer vatanaksoytezer approved these changes

@ChrisThrasher ChrisThrasher ChrisThrasher approved these changes

Assignees
No one assigned
Labels
backport-humble Mergify label that triggers a PR backport to Humble
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@sjahr @tylerjw @gavanderhoorn @henningkayser @vatanaksoytezer @ChrisThrasher