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

Do not add_library() gmock and gtest if targets already exist #927

Merged
merged 2 commits into from
Mar 21, 2018

Conversation

ojura
Copy link
Contributor

@ojura ojura commented Mar 20, 2018

Fixes build problems with Google Cartographer (cartographer-project/cartographer_ros#776), which provides its own FindGMock.cmake.

Fixes build problems with Google Cartographer (cartographer-project/cartographer_ros#776), which provides its own `FindGMock.cmake`.
@ojura ojura changed the title Do not add_library() gmock and gmock_main() if already existing Do not add_library() gmock and gtest if already existing Mar 21, 2018
@ojura ojura changed the title Do not add_library() gmock and gtest if already existing Do not add_library() gmock and gtest if targets already exist Mar 21, 2018
@dirk-thomas
Copy link
Member

And I was just about to ask for the gtest case. But here it is already: 54fdf04

Thank you for the patch.

@dirk-thomas dirk-thomas merged commit 38b85c6 into ros:kinetic-devel Mar 21, 2018
@ojura ojura deleted the patch-1 branch March 22, 2018 05:22
@ojura
Copy link
Contributor Author

ojura commented Mar 23, 2018

@dirk-thomas is there a chance you could make a release? CI for cartographer_ros is currently kind of stuck because of this.

@dirk-thomas
Copy link
Member

A release especially in ROS distros with a lot of packages is a significant effort (requiring to rebuild all packages across all targeted platforms as well as all users downloading new Debian packages). Therefore low level packages like catkin are not released very often. What ROS distro are you targeting cartographer_ros for?

@ojura
Copy link
Contributor Author

ojura commented Mar 23, 2018

cartographer_ros targets indigo, kinetic and lunar.

@dirk-thomas
Copy link
Member

The gmock support is not part of Indigo (we usually don't backport new features in to older ROS distros) so this change doesn't apply to the indigo-devel branch. You might want to test Indigo separately.

For Kinetic and higher and will try to get this released but it might be a couple of weeks from now.

@tfoote
Copy link
Member

tfoote commented Mar 23, 2018

@ojura There are other blockers for older targets due to incompatible versions of ceres. cartographer-project/cartographer_ros#158 I believe that there is a goal to get it working on Kinetic if we exclude Jessie, but won't be able to get it onto indigo target platforms. @mikaelarguedas FYI

@ojura
Copy link
Contributor Author

ojura commented Mar 23, 2018

@tfoote Sure. I am not a Cartographer maintainer though, just a third-party contributor. So, I am not involved much with releasing to ROS. My interest here was to get cartographer_ros's CI unblocked so we can merge PRs which have piled up.

@mikaelarguedas
Copy link
Member

There are other blockers for older targets due to incompatible versions of ceres. cartographer-project/cartographer_ros#158 I believe that there is a goal to get it working on Kinetic if we exclude Jessie, but won't be able to get it onto indigo target platforms

Regarding debs:
I haven't tried cartographer 0.3.0 on kinetic/lunar yet, but I expect the protobuf version to be a blocker. to release anything later than 0.2.0 in these distros. I don't think this PR is necessary to release 0.2.0 in kinetic/lunar.

0.3.0 has been released successfully in melodic (ros/rosdistro#17242), but this PR will be required to enable the release of cartographer_ros.

@ojura
Copy link
Contributor Author

ojura commented Mar 26, 2018

We have just merged a mitigation for this in libcartographer (cartographer-project/cartographer#1011), so it's not a pressing issue anymore for cartographer_ros master :)

@mikaelarguedas
Copy link
Member

thanks @ojura for the pointer to the mitigation PR 👍, I'll use that for releasing pending the release of this fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants