Skip to content

set useful build-options from MoveIt also in MTC#386

Closed
v4hn wants to merge 17 commits intomoveit:masterfrom
v4hn:pr-master-melodic-standard
Closed

set useful build-options from MoveIt also in MTC#386
v4hn wants to merge 17 commits intomoveit:masterfrom
v4hn:pr-master-melodic-standard

Conversation

@v4hn
Copy link
Copy Markdown
Contributor

@v4hn v4hn commented Oct 22, 2022

This includes the workaround to specify c++14 explicitly for Ubuntu 18.04 (checking the gcc version is not a good way to do that that, but it works and getting it more accurate is a lot of work...)

This should implicitly address #385 and Robert's problem in #380 (comment) . Sorry for bypassing CI for this, as always a bad idea.

The actual problem is that Ubuntu 18.04 comes with a cmake-config/qt5 mkspecs configuration that forces c++11 by default although it's compatible with 14, which breaks the fact that c++14 is the default in compilers in 18.04 anyway...

Why do we want melodic/Ubuntu 18.04 support in the master branches again?

Fixes #385 .

@v4hn
set useful build-options from MoveIt also in MTC
2a1eebc 
This includes the workaround to specify c++14 explicitly for Ubuntu 18.04
(checking the gcc version is not a good way to do that that, but it works
and getting it more accurate is a lot of work...)

Why do we want melodic/Ubuntu 18.04 support in the master branches again?
@v4hn v4hn mentioned this pull request Oct 22, 2022
Merged
@rhaschke
Copy link
Copy Markdown
Contributor

rhaschke commented Oct 23, 2022

Unfortunately, this approach doesn't work with MoveIt melodic-devel branch: MoveIt doesn't provide the cmake function there.

Why do we want Melodic/Ubuntu 18.04 support in the master branches again?

I didn't know that we dropped Melodic support for MTC. I'm fine to drop support for MoveIt's melodic-devel branch though.
However, we should maintain compatibility with Melodic/Ubuntu 18.04, building MoveIt from master branch.

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 23, 2022 •
edited
Loading

Codecov Report

Base: 54.77% // Head: 54.04% // Decreases project coverage by -0.74% ⚠️

Coverage data is based on head (3cc959f) compared to base (8d0f398).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #386      +/-   ##
==========================================
- Coverage   54.77%   54.04%   -0.73%     
==========================================
  Files         102       80      -22     
  Lines        7865     7692     -173     
==========================================
- Hits         4307     4156     -151     
+ Misses       3558     3536      -22
Impacted Files Coverage Δ
...abilities/src/execute_task_solution_capability.cpp 78.58% <ø> (ø)
core/include/moveit/task_constructor/utils.h 100.00% <ø> (ø)
core/src/cost_terms.cpp 32.82% <ø> (-0.26%) ⬇️
core/src/solvers/cartesian_path.cpp 85.00% <ø> (+0.39%) ⬆️
core/src/solvers/joint_interpolation.cpp 87.76% <ø> (ø)
core/src/solvers/planner_interface.cpp 100.00% <ø> (ø)
core/src/stage.cpp 82.64% <ø> (-0.28%) ⬇️
core/src/stages/connect.cpp 88.62% <ø> (-0.89%) ⬇️
core/src/stages/fix_collision_objects.cpp 0.00% <ø> (-1.38%) ⬇️
core/src/stages/move_to.cpp 79.70% <ø> (-0.15%) ⬇️
... and 94 more

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rhaschke
Copy link
Copy Markdown
Contributor

rhaschke commented Oct 23, 2022

I am working on fixing the remaining compiler issues as well. Please wait a minute...

v4hn and others added 6 commits October 23, 2022 23:59
@v4hn
find_package eigen3 as SYSTEM
5f2a643
@rhaschke
Implicitly use clang in clang-tidy builds
43b030b
@rhaschke
Fix unused argument warning
c3b7932
@rhaschke
Limit strict compiler warnings to target workspace
1303d5f
@rhaschke
Update GHA versions
62a7901
@rhaschke
Remove -Werror for clang build
90597b0 
... due to (new) error in Eigen:
/usr/include/eigen3/Eigen/src/LU/arch/Inverse_SSE.h:142:45: error: cast from 'const unsigned int *' to 'float *' drops const qualifier [-Werror,-Wcast-qual]
@v4hn
Copy link
Copy Markdown
Contributor Author

v4hn commented Oct 23, 2022

I am working on fixing the remaining compiler issues as well. Please wait a minute...

Too late? :) Might be fixed now... Or not.. Looks like setting the eigen include in addition as system does not fix it yet.
Sorry, I got around to look into this after preparing for IROS today. I guess you are not in Kyoto? I met Luca already. :)

Why do we want Melodic/Ubuntu 18.04 support in the master branches again?

I didn't know that we dropped Melodic support for MTC. I'm fine to drop support for MoveIt's melodic-devel branch though.
However, we should maintain compatibility with Melodic/Ubuntu 18.04, building MoveIt from master branch.

Sorry, that was meant to read as "Why again do we [still] want 18.04 support". I like your proposal to drop the melodic-devel support. I'd like to drop 18.04 altogether, but building from MoveIt master on 18.04 is fine by me.

@rhaschke
Copy link
Copy Markdown
Contributor

rhaschke commented Oct 23, 2022

No, I'm not in Kyoto. I pushed all my fixes, which make it work w/o issues.
But, I'm not sure yet regarding the test failure. I didn't change anything to handle it, but it disappeared automatically?

Enjoy IROS and Kyoto!

@v4hn
Copy link
Copy Markdown
Contributor Author

v4hn commented Oct 23, 2022 •
edited
Loading

Can you enlighten me to why setting -Wall et al in TARGET_CMAKE_ARGS fixes the system include error triggered in the target workspace? Doesn't that accidentally deactivate all -W flags because they will be consumed by cmake instead of the compiler?

You just found it I guess. :)

@v4hn
Copy link
Copy Markdown
Contributor Author

v4hn commented Oct 23, 2022

I'm not sure yet regarding the test failure. I didn't change anything to handle it, but it disappeared automatically?

Yes, it seems we have a spurious failure in the test...

@rhaschke
Copy link
Copy Markdown
Contributor

rhaschke commented Oct 23, 2022

I finally fixed the unit test newly failing with clang. I cleaned up the history and pushed a new PR: #387. Closing here.

@rhaschke rhaschke closed this Oct 23, 2022
@v4hn v4hn mentioned this pull request Oct 24, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@jspricke jspricke jspricke approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Building using catkin_make failed in ros-melodic

3 participants

@v4hn @rhaschke @jspricke