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

update CMakeLists.txt to search for local gtest first #96

Merged
merged 1 commit into from
Mar 12, 2019

Conversation

kejxu
Copy link

@kejxu kejxu commented Feb 13, 2019

gtest 1.7.0 on Windows causes the following error:

C:\ros\catkin_ws\upstream\ros-desktop\src\diagnostics\diagnostic_aggregator\gtest-1.7.0\gtest-all.cc(1374): error C2011: 'testing::internal::AutoHandle': 'class' type redefinition
C:\opt\rosdeps\x64\include\gtest/internal/gtest-port.h(1511): note: see declaration of 'testing::internal::AutoHandle'
C:\ros\catkin_ws\upstream\ros-desktop\src\diagnostics\diagnostic_aggregator\gtest-1.7.0\gtest-all.cc(3381): error C2440: 'delete': cannot convert from 'const testing::internal::scoped_ptr<testing::internal::GTestFlagSaver>' to 'void*'
C:\ros\catkin_ws\upstream\ros-desktop\src\diagnostics\diagnostic_aggregator\gtest-1.7.0\gtest-all.cc(3381): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
C:\ros\catkin_ws\upstream\ros-desktop\src\diagnostics\diagnostic_aggregator\gtest-1.7.0\gtest-all.cc(3667): error C2511: 'testing::TestInfo::TestInfo(const std::string &,const std::string &,const char *,const char *,testing::internal::TypeId,testing::internal::TestFactoryBase *)': overloaded member function not found in 'testing::TestInfo'
C:\opt\rosdeps\x64\include\gtest/gtest.h(644): note: see declaration of 'testing::TestInfo'
C:\ros\catkin_ws\upstream\ros-desktop\src\diagnostics\diagnostic_aggregator\gtest-1.7.0\gtest-all.cc(3676): error C2550: 'testing::TestInfo::{ctor}': constructor initializer lists are only allowed on constructor definitions
C:\ros\catkin_ws\upstream\ros-desktop\src\diagnostics\diagnostic_aggregator\gtest-1.7.0\gtest-all.cc(3710): error C2661: 'testing::TestInfo::TestInfo': no overloaded function takes 6 arguments
C:\ros\catkin_ws\upstream\ros-desktop\src\diagnostics\diagnostic_aggregator\gtest-1.7.0\gtest-all.cc(3709): error C2789: 'test_info': an object of const-qualified type must be initialized
C:\ros\catkin_ws\upstream\ros-desktop\src\diagnostics\diagnostic_aggregator\gtest-1.7.0\gtest-all.cc(3709): note: see declaration of 'test_info'
C:\ros\catkin_ws\upstream\ros-desktop\src\diagnostics\diagnostic_aggregator\gtest-1.7.0\gtest-all.cc(7120): error C2079: 'testing::internal::WindowsDeathTest::write_handle_' uses undefined class 'testing::internal::AutoHandle'
C:\ros\catkin_ws\upstream\ros-desktop\src\diagnostics\diagnostic_aggregator\gtest-1.7.0\gtest-all.cc(7122): error C2079: 'testing::internal::WindowsDeathTest::child_handle_' uses undefined class 'testing::internal::AutoHandle'
C:\ros\catkin_ws\upstream\ros-desktop\src\diagnostics\diagnostic_aggregator\gtest-1.7.0\gtest-all.cc(7127): error C2079: 'testing::internal::WindowsDeathTest::event_handle_' uses undefined class 'testing::internal::AutoHandle'
C:\ros\catkin_ws\upstream\ros-desktop\src\diagnostics\diagnostic_aggregator\gtest-1.7.0\gtest-all.cc(7222): error C2672: 'testing::internal::StreamableToString': no matching overloaded function found
C:\ros\catkin_ws\upstream\ros-desktop\src\diagnostics\diagnostic_aggregator\gtest-1.7.0\gtest-all.cc(7728): error C2079: 'parent_process_handle' uses undefined class 'testing::internal::AutoHandle'
C:\ros\catkin_ws\upstream\ros-desktop\src\diagnostics\diagnostic_aggregator\gtest-1.7.0\gtest-all.cc(7728): error C2440: 'initializing': cannot convert from 'HANDLE' to 'int'
C:\ros\catkin_ws\upstream\ros-desktop\src\diagnostics\diagnostic_aggregator\gtest-1.7.0\gtest-all.cc(7728): note: There is no context in which this conversion is possible
C:\ros\catkin_ws\upstream\ros-desktop\src\diagnostics\diagnostic_aggregator\gtest-1.7.0\gtest-all.cc(9553): error C2065: 'defined_test_names_': undeclared identifier
C:\ros\catkin_ws\upstream\ros-desktop\src\diagnostics\diagnostic_aggregator\gtest-1.7.0\gtest-all.cc(9554): error C2065: 'defined_test_names_': undeclared identifier
C:\ros\catkin_ws\upstream\ros-desktop\src\diagnostics\diagnostic_aggregator\gtest-1.7.0\gtest-all.cc(9570): error C2065: 'defined_test_names_': undeclared identifier
C:\ros\catkin_ws\upstream\ros-desktop\src\diagnostics\diagnostic_aggregator\gtest-1.7.0\gtest-all.cc(9571): error C2065: 'defined_test_names_': undeclared identifier
NMAKE : fatal error U1077: 'C:\PROGRA~2\MIB055~1\Preview\ENTERP~1\VC\Tools\MSVC\1416~1.270\bin\Hostx64\x64\cl.exe' : return code '0x2'
Stop.
NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio\Preview\Enterprise\VC\Tools\MSVC\14.16.27023\bin\HostX64\x64\nmake.exe"' : return code '0x2'
Stop.
NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio\Preview\Enterprise\VC\Tools\MSVC\14.16.27023\bin\HostX64\x64\nmake.exe"' : return code '0x2'
Stop.

since a lot of ROS projects use gtest for testing, and gtest is already in a stable 1.8.x state, change to use global gtest and delete local gtest binaries

@trainman419
Copy link
Contributor

There was a big mess with this package depending on the system version of gtest a few years back.

If you're interested in working on this, I suggest you read through #30 . If you have time to implement the proposal there or propose something else that addresses those concerns and fixes the windows build, I would be happy to review it.

If upgrading to a newer version of gtest will fix this, then upgrading the version of gtest in this package would also be an acceptable solution.

@kejxu
Copy link
Author

kejxu commented Feb 14, 2019

There was a big mess with this package depending on the system version of gtest a few years back.

If you're interested in working on this, I suggest you read through #30 . If you have time to implement the proposal there or propose something else that addresses those concerns and fixes the windows build, I would be happy to review it.

If upgrading to a newer version of gtest will fix this, then upgrading the version of gtest in this package would also be an acceptable solution.

This makes sense, we expected the motivation to be something like this too, system version of gtest can never be expected. I took a look at your conclusion in #30, rewriting these in Python might fix the problem but that might be something to work on later. At this point, we are primarily trying to push forward the upstreaming process so that ROS could be built and run on Windows.

@kejxu
Copy link
Author

kejxu commented Feb 15, 2019

There was a big mess with this package depending on the system version of gtest a few years back.

If you're interested in working on this, I suggest you read through #30 . If you have time to implement the proposal there or propose something else that addresses those concerns and fixes the windows build, I would be happy to review it.

If upgrading to a newer version of gtest will fix this, then upgrading the version of gtest in this package would also be an acceptable solution.

I looked further into this, the error message indicates redefinition of the class AutoHandle. which made me realize that the local gtest-all.cc is using the wrong gtest.h. Have made corresponding changes, and updated the pull request.

Question regarding that, however, is this a custom version of gtest 1.7.0? asking because it's different from gtest github: https://github.com/google/googletest/tree/release-1.7.0. I figured if all class definitions are done correct, this error wouldn't happen since it could be prevented by header guards.

I found this method interesting, however it didn't work when I tested it out. gtest and gtest-main are already defined as targets and built in catkin. It could be a potential improvement (for CI with gtest) if any one of us has bandwidth for further investment.

@kejxu kejxu changed the title use global gtest library update CMakeLists.txt to search for local gtest first Feb 15, 2019
@kejxu
Copy link
Author

kejxu commented Feb 15, 2019

I looked further into this, the error message indicates redefinition of the class AutoHandle. which made me realize that the local gtest-all.cc is using the wrong gtest.h. Have made corresponding changes, and updated the pull request.

proper handling of include directories is already existing in self_test\CMakeLists.txt, haha =)

@kejxu
Copy link
Author

kejxu commented Mar 1, 2019

@trainman419, just a friendly ping =) any update on this change?

@trainman419 trainman419 merged commit f824f8f into ros:indigo-devel Mar 12, 2019
@trainman419
Copy link
Contributor

Sorry for the delay. Thanks!

@kejxu kejxu deleted the use_global_gtest_library branch March 12, 2019 18:24
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.

2 participants