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

Add support for GMock #708

Closed
wants to merge 1 commit into from
Closed

Add support for GMock #708

wants to merge 1 commit into from

Conversation

kyrofa
Copy link

@kyrofa kyrofa commented Jan 13, 2015

Catkin currently is hard-coded to use the source-only install of GTest if find_package(GTest) fails. This makes it difficult to use the source-only install of GMock, which bundles and builds its own copy of GTest.

This PR resolves #699 by making Catkin check for the presence of a GMock source-only install before checking for GTest. This is a backward-compatible change.

@kyrofa kyrofa changed the title Add support for GMock. Add support for GMock Jan 13, 2015
@kyrofa
Copy link
Author

kyrofa commented Jan 14, 2015

@dirk-thomas This resolves the discussion you and I had here. It gives us what we need (support for source-only GMock installs) while hopefully satisfying your desire for a stable, backward-compatible change. It doesn't require GMock, but will use it if it's there.

@dirk-thomas
Copy link
Member

Thank you for the patch!

I will have to test this very thoroughly before merging it. That might take a while due to other tasks.

One thing I noticed: in the package.xml the dependency on gtest should be replaced by gmock.

@kyrofa
Copy link
Author

kyrofa commented Jan 15, 2015

Please do take it for a spin! With how this patch was written, the catkin dependency on gtest doesn't need to change unless you'd like it to. It will simply use gmock and its gtest if it's there, and use the standalone gtest if its not. All the defines are the same and it should be seamless. I can revamp and remove the standalone gtest if you wish, but that is sort of a potentially far-reaching move. Your call!

@kyrofa
Copy link
Author

kyrofa commented Jan 23, 2015

@dirk-thomas Did you want me to pull the standalone gtest support? Or leave it backward-compatible?

@dirk-thomas
Copy link
Member

The gtest dependency should be replaced with a gmock dependency. Otherwise the Debian package won't install gmock making it not usable to the users.

I didn't have the time to test it yet. It will likely take another week or two until I will get to it.

@kyrofa
Copy link
Author

kyrofa commented Jan 23, 2015

@dirk-thomas I understand that gmock wouldn't be installed by default (only gtest), but the patch was written to handle that. My concern stems from your comment here, where you said that replacing gtest with gmock could be considered for Jade, but not for Indigo. Since my team needs this patch in Indigo, it was written specifically not to replace gtest-- merely to support gmock-- to satisfy your stability concerns.

If I replace gtest with gmock, does geting this into Indigo become an impossibility? If so, can we leave this PR as-is? Then, if you like, when it's approved I can submit another PR to replace gtest with gmock for Jade.

@dirk-thomas
Copy link
Member

Let's see how testing goes and how confident we are. Then decide on the dependency used in indigo.

@kyrofa
Copy link
Author

kyrofa commented Jan 23, 2015

@dirk-thomas Good deal-- thanks for your time! I'll leave it as-is for now, until I hear otherwise.

@kyrofa
Copy link
Author

kyrofa commented Feb 4, 2015

@dirk-thomas Hey, I don't meant to be a bother but I wanted to ping you real quick on this. Have you had a chance to play with it at all?

@robobuilder
Copy link

@dirk-thomas: Has there been any progress with this pull request? My team will find this patch very useful!

@kyrofa
Copy link
Author

kyrofa commented Feb 19, 2015

@robobuilder I've setup an Ubuntu PPA containing this patch: https://launchpad.net/~ausl/+archive/ubuntu/ros . Just add it to your sources and install updates and you'll get the patched version of Catkin compatible with GMock.

Hopefully I won't have to maintain the fork for long. @dirk-thomas can you give any sort of estimate?

Search for GMock (and its bundled GTest) before searching for
standalone GTest.

This resolves #699.

Signed-off-by: Kyle Fazzari <kyle.fazzari@navy.mil>
@kyrofa
Copy link
Author

kyrofa commented Mar 11, 2015

@dirk-thomas I refactored this slightly to make the change even smaller. Please take a look as soon as you can-- this is a critical item for us.

@dirk-thomas
Copy link
Member

I have rebased your PR to head: https://github.com/ros/catkin/tree/gmock_support
I also added the dependency on gmock in the package.xml file and contributed missing rules for other platforms (ros/rosdistro#7926).

But I think this still needs further changes / testing especially to work on other platforms. The assumption that gmock always comes with gtest embedded is only true on Ubuntu. On other platforms these are two separate packages. Therefore searching for gtest only within gmock (https://github.com/ros/catkin/pull/708/files#diff-84d1585b7758a3723f688bcff3f8602cR145) won't work on those.

It might also be useful to provide some GMock specific CMake functions to simplify the usage for the users (similar to catkin_add_gtest).

@mkoval
Copy link
Contributor

mkoval commented Jul 6, 2015

Has there been a resolution to this? Is the blocking issue that this assumes that GMock includes GTest?

I am also trying to use GMock in a Catkin package and ended up in the same fight against Catkin including a binary version of GTest. It would be nice to get this merged add the additional features (e.g. helper macros) in a future pull request.

@dirk-thomas
Copy link
Member

There has been no progress since the last comments and the necessary changes are still pending.

@mkoval
Copy link
Contributor

mkoval commented Jul 6, 2015

@dirk-thomas I'm willing to do the legwork to get this merged. Is this the only blocking issue?

The assumption that gmock always comes with gtest embedded is only true on Ubuntu. On other platforms these are two separate packages.

@dirk-thomas
Copy link
Member

Yes, I think this is currently the only regression. I will only have time to review changes and can't contribute to writing them. Therefore any help is greatly appreciated.

I think it also needs to be tested on various platforms. We can likely ask users which have contributed to this part before to recheck if the changes still work for them.

@kyrofa
Copy link
Author

kyrofa commented Jul 15, 2015

@mkoval this sat here for months before it was given any attention, and by then I had stopped using ROS. I'm sorry, but I no longer have the necessary environment setup to develop this further. Please take ownership of it if you're willing.

Although, you should probably make a new MR at this point.

@kyrofa kyrofa closed this Jul 15, 2015
cwecht pushed a commit to cwecht/catkin that referenced this pull request Mar 20, 2018
Provide a descriptive StatusResponse msg field consisting of
an gRPC-like StatusCode and message string to the service caller.

Implements [RFC 13](https://github.com/googlecartographer/rfcs/blob/master/text/0013-improve-ros-service-responses.md).
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