Skip to content

Fix compiler warnings related to gtest/gmock#408

Merged
clalancette merged 4 commits intoament:rollingfrom
rhaschke:fix-compiler-warnings
Jan 6, 2023
Merged

Fix compiler warnings related to gtest/gmock#408
clalancette merged 4 commits intoament:rollingfrom
rhaschke:fix-compiler-warnings

Conversation

@rhaschke
Copy link
Copy Markdown
Contributor

@rhaschke rhaschke commented Nov 1, 2022

With clang and rather strict settings for compiler warnings, downstream projects fail to build due to issues in gtest/gmock:
https://github.com/ubi-agni/moveit_task_constructor/actions/runs/3372692498/jobs/5596418220

This PR extends #94 introduced by @dirk-thomas by:

  • adding -Wno-deprecated-copy to the _gmock_compile_flags
  • declaring gtest/gmock include dirs as SYSTEM, such that compilers ignore issues in those headers

@rhaschke
Suppress compiler warnings when building gmock
a40b9c0 
definition of implicit copy constructor ... is deprecated because it has a user-declared copy assignment operator [-Wdeprecated-copy]

Signed-off-by: Robert Haschke <rhaschke@techfak.uni-bielefeld.de>
@rhaschke
Declare gtest/gmock include dirs as SYSTEM PRIVATE for test targets
14b9031 
Signed-off-by: Robert Haschke <rhaschke@techfak.uni-bielefeld.de>
@rhaschke
Declare gtest include dirs as SYSTEM PUBLIC for gtest targets
acb3a55 
Signed-off-by: Robert Haschke <rhaschke@techfak.uni-bielefeld.de>
@rhaschke rhaschke force-pushed the fix-compiler-warnings branch from 161989d to acb3a55 Compare November 1, 2022 22:28
@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Nov 4, 2022

Rather than disabling more warnings, I wonder if it would help to upgrade to gmock 1.11.0, which would match what is in Ubuntu 22.04 anyway. Can you give that a try and see if it resolves these warnings?

@rhaschke
Copy link
Copy Markdown
Contributor Author

rhaschke commented Nov 4, 2022

You are right, according to this, gtest/gmock 1.11 should resolve the clang compiler warnings.

However, `ament-cmake-gtest` / `ament-cmake-gmock` have issues building this version. 
CMake Error at /usr/src/googletest/googletest/cmake/internal_utils.cmake:158 (add_library):
  add_library cannot create target "gtest" because another target with the
  same name already exists.  The existing target is a static library created
  in source directory "/opt/ros/rolling/src/gtest_vendor".  See documentation
  for policy CMP0002 for more details.
Call Stack (most recent call first):
  /usr/src/googletest/googletest/cmake/internal_utils.cmake:215 (cxx_library_with_type)
  /usr/src/googletest/googletest/CMakeLists.txt:133 (cxx_library)

CMake Error at /usr/src/googletest/googletest/cmake/internal_utils.cmake:158 (add_library):

add_library cannot create target "gtest_main" because another target with

the same name already exists.  The existing target is a static library

created in source directory "/opt/ros/rolling/src/gtest_vendor".  See

documentation for policy CMP0002 for more details.

Call Stack (most recent call first):

/usr/src/googletest/googletest/cmake/internal_utils.cmake:215 (cxx_library_with_type)

/usr/src/googletest/googletest/CMakeLists.txt:136 (cxx_library)


CMake Error at /usr/src/googletest/googletest/CMakeLists.txt:149 (target_link_libraries):

Attempt to add link library "gtest" to target "gtest_main" which is not

built in this directory.


This is allowed only when policy CMP0079 is set to NEW.


CMake Error at /usr/src/googletest/googletest/cmake/internal_utils.cmake:158 (add_library):

add_library cannot create target "gmock" because another target with the

same name already exists.  The existing target is a static library created

in source directory "/usr/src/googletest/googlemock".  See documentation

for policy CMP0002 for more details.

Call Stack (most recent call first):

/usr/src/googletest/googletest/cmake/internal_utils.cmake:215 (cxx_library_with_type)

/usr/src/gmock/CMakeLists.txt:103 (cxx_library)


CMake Error at /usr/src/gmock/CMakeLists.txt:104 (target_link_libraries):

Attempt to add link library "gtest" to target "gmock" which is not built in

this directory.


This is allowed only when policy CMP0079 is set to NEW.


CMake Error at /usr/src/googletest/googletest/cmake/internal_utils.cmake:158 (add_library):

add_library cannot create target "gmock_main" because another target with

the same name already exists.  The existing target is a static library

created in source directory "/usr/src/googletest/googlemock".  See

documentation for policy CMP0002 for more details.

Call Stack (most recent call first):

/usr/src/googletest/googletest/cmake/internal_utils.cmake:215 (cxx_library_with_type)

/usr/src/gmock/CMakeLists.txt:106 (cxx_library)


CMake Error at /usr/src/gmock/CMakeLists.txt:107 (target_link_libraries):

Attempt to add link library "gmock" to target "gmock_main" which is not

built in this directory.


This is allowed only when policy CMP0079 is set to NEW.

I just force-removed the vendor packages like so:

dpkg -r --force-depends ros-rolling-gmock-vendor
dpkg -r --force-depends ros-rolling-gtest-vendor

and then built from scratch.

I don't have enough insight into these ament packages to fix this issue.

@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Nov 7, 2022

Can you try out ament/googletest#21 ?

@rhaschke
Remove warning relaxations for gmock
d605e34 
Signed-off-by: Robert Haschke <rhaschke@techfak.uni-bielefeld.de>
@rhaschke
Copy link
Copy Markdown
Contributor Author

rhaschke commented Nov 7, 2022

Can you try out ament/googletest#21?

Yes, that works: https://github.com/ubi-agni/moveit_task_constructor/actions/runs/3413457294/jobs/5680185989. Thanks!

Maybe we can remove those warning relaxation completely. I added an appropriate commit and the test succeeded as well.
In any case, I think includes should be declared as SYSTEM. So this PR is still relevant 😉

@rhaschke rhaschke mentioned this pull request Nov 10, 2022
Merged
@rhaschke
Copy link
Copy Markdown
Contributor Author

rhaschke commented Jan 5, 2023

@clalancette, any update on this? ament/googletest#21 fixes the warnings, but wasn't touched for 2 months.

@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Jan 5, 2023

@clalancette, any update on this? ament/googletest#21 fixes the warnings, but wasn't touched for 2 months.

Unfortunately, it introduces new warnings, and I haven't had time to look into them.

@rhaschke
Copy link
Copy Markdown
Contributor Author

rhaschke commented Jan 5, 2023

What about merging this PR then? gtest and gmock should be handled as external sources, i.e. using SYSTEM includes as proposed in this PR.

@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Jan 5, 2023

I do think we should upgrade gtest to version 1.11, but that doesn't block this PR. This looks OK, I'll run CI on it and see what comes back.

clalancette
clalancette approved these changes Jan 5, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@clalancette clalancette 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 with green CI.

@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Jan 5, 2023

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette clalancette merged commit c5489e8 into ament:rolling Jan 6, 2023
@rhaschke rhaschke deleted the fix-compiler-warnings branch January 6, 2023 08:09
@rhaschke rhaschke restored the fix-compiler-warnings branch January 8, 2023 08:14
@rhaschke rhaschke deleted the fix-compiler-warnings branch January 8, 2023 09:08
@clalancette clalancette mentioned this pull request Feb 27, 2023
Closed
@henningkayser henningkayser mentioned this pull request Aug 2, 2023
Closed
@atzaros atzaros mentioned this pull request Nov 24, 2023
Closed
atzaros pushed a commit to atzaros/ament_cmake that referenced this pull request Nov 24, 2023
@rhaschke @atzaros
Fix compiler warnings related to gtest/gmock (ament#408)
eb93883 
* Suppress compiler warnings when building gmock

definition of implicit copy constructor ... is deprecated because it has a user-declared copy assignment operator [-Wdeprecated-copy]

* Declare gtest/gmock include dirs as SYSTEM PRIVATE for test targets

Signed-off-by: Robert Haschke <rhaschke@techfak.uni-bielefeld.de>
atzaros pushed a commit to atzaros/ament_cmake that referenced this pull request Nov 24, 2023
@rhaschke @atzaros
Fix compiler warnings related to gtest/gmock (ament#408)
dbe21ba 
* Suppress compiler warnings when building gmock

definition of implicit copy constructor ... is deprecated because it has a user-declared copy assignment operator [-Wdeprecated-copy]

* Declare gtest/gmock include dirs as SYSTEM PRIVATE for test targets

Signed-off-by: Robert Haschke <rhaschke@techfak.uni-bielefeld.de>
Signed-off-by: atzaros <128592691+atzaros@users.noreply.github.com>
clalancette pushed a commit that referenced this pull request Dec 1, 2023
@atzaros @rhaschke
Fix compiler warnings related to gtest/gmock (#408) (#495)
960b8e8 
* Suppress compiler warnings when building gmock

definition of implicit copy constructor ... is deprecated because it has a user-declared copy assignment operator [-Wdeprecated-copy]

* Declare gtest/gmock include dirs as SYSTEM PRIVATE for test targets

Signed-off-by: Robert Haschke <rhaschke@techfak.uni-bielefeld.de>
Signed-off-by: atzaros <128592691+atzaros@users.noreply.github.com>
Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@clalancette clalancette clalancette approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rhaschke @clalancette