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

Fix ABI checker with testing module #427

Merged
merged 2 commits into from
Aug 30, 2022

Conversation

mjcarroll
Copy link
Contributor

@mjcarroll mjcarroll commented Aug 19, 2022

Signed-off-by: Michael Carroll [email protected]

🦟 Bug fix

Fixes issue brought up in comment here: #411 (comment)

Summary

The header AutoLogFixture.hh is available to be used as part of a library or executable that links against gtest. We don't want to introduce a hard dependency on gtest in common::testing for just this header, but it seems to annoy the ABI checker.

This adds a private include so that the gtest header may be resolved, even though downstream users of the testing component will never see it.

This puts the AutoLogFixture within a define guard GTEST_API to guarantee that gtest is available before using the autologfixture.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@mjcarroll mjcarroll self-assigned this Aug 19, 2022
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Aug 19, 2022
@codecov
Copy link

codecov bot commented Aug 19, 2022

Codecov Report

Merging #427 (9946a08) into gz-common5 (5d576ba) will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           gz-common5     #427   +/-   ##
===========================================
  Coverage       80.38%   80.38%           
===========================================
  Files              85       85           
  Lines            9944     9944           
===========================================
  Hits             7993     7993           
  Misses           1951     1951           

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

@mjcarroll
Copy link
Contributor Author

@j-rivero so it seems I can't manipulate the ABI checker via CMake.

The situation here is that the AutoLogFixture.hh header is intended to be used only by people who already have linkage/includes against gtest.

Is there a way to exclude this from the ABI checker? Or is there a better way to indicate this?

testing/src/CMakeLists.txt Outdated Show resolved Hide resolved
@mjcarroll mjcarroll force-pushed the mjcarroll/fix_testing_include branch from 9a5f346 to 341a94b Compare August 23, 2022 19:36
@mjcarroll mjcarroll added the bug Something isn't working label Aug 23, 2022
@mjcarroll
Copy link
Contributor Author

@osrf-jenkins retest this please

Signed-off-by: Michael Carroll <[email protected]>

Co-authored-by: Jenn Nguyen <[email protected]>
@mjcarroll
Copy link
Contributor Author

The abi checker is actually having issues with the destination branch in this case. This PR would need to be merged so that the destination branch (gz-common5) is fixed. I would expect the ABI checker to fail on this PR but succeed on future PRs.

@mjcarroll
Copy link
Contributor Author

The error in question:

The GCC parameters:
  gcc -fdump-lang-raw -fkeep-inline-functions -c -x c++ -fpermissive -w  -std=c++17 "/tmp/4wmT6ajeTj/dump1.h"  -I/usr/local/destination_branch/include/gz/common5 -I/usr/include/gz/utils2 -I/usr/include/gz/math7

In file included from /usr/local/destination_branch/include/gz/common5/gz/common/testing.hh:23,
                 from /tmp/4wmT6ajeTj/dump1.h:83:
/usr/local/destination_branch/include/gz/common5/gz/common/testing/AutoLogFixture.hh:20:10: fatal error: gtest/gtest.h: No such file or directory
   20 | #include <gtest/gtest.h>
      |          ^~~~~~~~~~~~~~~
compilation terminated.

@chapulina chapulina merged commit aec75f5 into gz-common5 Aug 30, 2022
@chapulina chapulina deleted the mjcarroll/fix_testing_include branch August 30, 2022 21:55
@mjcarroll mjcarroll mentioned this pull request Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants