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

Include gtest source directly #28

Merged
merged 1 commit into from
Jul 29, 2014
Merged

Include gtest source directly #28

merged 1 commit into from
Jul 29, 2014

Conversation

trainman419
Copy link
Contributor

No description provided.

trainman419 added a commit that referenced this pull request Jul 29, 2014
Include gtest source directly
@trainman419 trainman419 merged commit 5a4320b into hydro-devel Jul 29, 2014
@tfoote
Copy link
Member

tfoote commented Jul 29, 2014

@trainman419 why are you including this directly instead of using the gtest commands?

@trainman419
Copy link
Contributor Author

Several of the diagnostics nodes are explicitly for use in testing other packages, and they're part of the package API contract. Therefore, they must always be built and installed, even when the system version of gtest may not be available.

This is also the recommended way to use gtest, and was recommended by @dirk-thomas on ROS Answers: http://answers.ros.org/question/186969/installing-binaries-linked-against-gtest/

I've chosen to include the gtest sources and compile them in directly so that I don't have to wrestle with the hassle and confusion of installing a separate gtest library that is used only by a few nodes.

@trainman419
Copy link
Contributor Author

@dirk-thomas
Copy link
Member

It is definitely not a good idea to embed the gtest source code into this repository - especially doin it twice(?). I didn't recommend that in the referenced ROS answer. I would highly recommend to revert this patch.

You can easily rely on the sources being available by having a dependency on the appropriate package as catkin does it too. Please reconsider / rediscuss this. We should wait to merge the corresponding rosdistro pull requests until this has been figured out.

@trainman419
Copy link
Contributor Author

The recommendation on ROS Answers was to install my own copy of the gtest library if I need to depend on it. I've chosen to include the source and build directly against the gtest sources to avoid issues like #27

When compiling with -DCATKIN_ENABLE_TESTING=0, it's obvious from #27 that gtest is not always available.

I've included the source once in each package that uses it, because it must be available at build time on the farm, when the other packages in the stack are stripped away, and I don't want my packages to install the gtest headers, libraries, or depend on a system version of gtest.

Embedding the gtest code in a repository is also the recommended practice from the gtest project itself: https://code.google.com/p/googletest/wiki/FAQ#Why_is_it_not_recommended_to_install_a_pre-compiled_copy_of_Goog

I'm open to other solutions, but I think it will be difficult to find something that simultaneously satisfies #16, #24 and #27

@dirk-thomas
Copy link
Member

Why don't you want to use the system installed gtest headers?

@dirk-thomas
Copy link
Member

The FAQ entry you referenced says:

each project should compile Google Test itself such that it can be sure that the same flags are used for both Google Test and the tests

It does not recommend to embed the source code in each and every project.

You could do the folllowing (which has nothing to do with catkin):

  • build depend on gtest
  • get the location of the gtest headers / source in CMake
  • compile your code against them as you already do

@trainman419
Copy link
Contributor Author

The previous version (1.8.4) did exactly that, and #27 demonstrates that it doesn't work in all cases.

@bulwhan: this fixes diagnostics on your system. Care to weigh in on why the gtest library isn't available on your particular setup?

@dirk-thomas
Copy link
Member

Issue #27 is related to using the catkin gtest functions when catkin testing is disabled which is not supported.

As I mentioned in my previous comment: you should not rely on any catkin stuff for that.

@bulwahn
Copy link
Contributor

bulwahn commented Jul 31, 2014

The requirement that diagnostic_aggregator also compiles with -DCATKIN_ENABLE_TESTING=0 is motivated by the packaging with OpenEmbedded (the meta-ros layer for OpenEmbedded), in which we only want to configure, compile and package the shipped binaries/libraries/scripts, and not the tests.

The gtest sources are not available in my build environment, because so far, if CATKIN_ENABLE_TESTING=0, no package ever requires it to be available in the build environment.
As far as I see it, if the gtest sources are not in the diagnostics repository (which is a clean solution IMHO), the package.xml should mention that the gtest sources are required to be installed during the build, and the catkin_make command should fail during configure if the package is not found. I recall that both these requirements are not met in version 1.8.4.

For the meta-ros layer, I will figure out how to make the gtest sources available in the build environment.

@dirk-thomas
Copy link
Member

I think I have clearly described how these packages can achieve that in my previous comments. If anything is unclear how this is done in detail please ask specific questions.

@bulwahn
Copy link
Contributor

bulwahn commented Jul 31, 2014

@dirk-thomas I agree. I just have to try how that will work with OpenEmbedded, and then we are set.

@trainman419
Copy link
Contributor Author

I've put together https://github.com/ros/diagnostics/tree/gtest_depend , but it fails to build when CATKIN_ENABLE_TESTING is 0 because catkin does not find gtest in that scenario, and no cmake FindGtest module is provided by catkin or gtest.

@dirk-thomas
Copy link
Member

You can not rely on gtest to magically be found. You have to find it yourself.

@trainman419
Copy link
Contributor Author

Yeah. I think the proper solution is #30, but I've run out of employer-sponsored time to work on this for the next few months.

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