-
Notifications
You must be signed in to change notification settings - Fork 5
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 gazebo7 to travis #44
Conversation
639338d
to
7a6b374
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that there are still compilation problems in Gazebo 7, see https://travis-ci.com/robotology/gazebo-fmi/jobs/152525711#L6251 .
Scanning dependencies of target FMIActuatorPluginTest
[100%] Building CXX object plugins/actuator/test/CMakeFiles/FMIActuatorPluginTest.dir/FMIActuatorPluginTest.cc.o
/home/travis/build/robotology/gazebo-fmi/plugins/actuator/test/FMIActuatorPluginTest.cc: In member function 'void FMIActuatorPluginTest::PluginTestHelper(const string&, const string&, const FMIActuatorPluginTest::PluginTestHelperOptions&)':
/home/travis/build/robotology/gazebo-fmi/plugins/actuator/test/FMIActuatorPluginTest.cc:45:54: error: 'class gazebo::physics::World' has no member named 'Physics'
gazebo::physics::PhysicsEnginePtr physics = world->Physics();
^
/home/travis/build/robotology/gazebo-fmi/plugins/actuator/test/FMIActuatorPluginTest.cc:52:23: error: 'class gazebo::physics::World' has no member named 'ModelByName'
auto model = world->ModelByName("pendulum_with_base");
^
plugins/actuator/test/CMakeFiles/FMIActuatorPluginTest.dir/build.make:62: recipe for target 'plugins/actuator/test/CMakeFiles/FMIActuatorPluginTest.dir/FMIActuatorPluginTest.cc.o' failed
make[2]: *** [plugins/actuator/test/CMakeFiles/FMIActuatorPluginTest.dir/FMIActuatorPluginTest.cc.o] Error 1
CMakeFiles/Makefile2:851: recipe for target 'plugins/actuator/test/CMakeFiles/FMIActuatorPluginTest.dir/all' failed
make[1]: *** [plugins/actuator/test/CMakeFiles/FMIActuatorPluginTest.dir/all] Error 2
Makefile:138: recipe for target 'all' failed
make: *** [all] Error 2
@@ -6,6 +6,8 @@ | |||
# https://www.gnu.org/licenses/old-licenses/lgpl-2.1.html | |||
# at your option. | |||
|
|||
add_compile_options(-std=c++11) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you just remove this line
add_compile_options(-std=c++11) |
and just add a
set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
in the main CMakeLists.txt
? See https://crascit.com/2015/03/28/enabling-cxx11-in-cmake/ for more discussion. Setting directly the compile options is not ideal because it could create problems if at some point Gazebo uses a newer version of the standard (such as C++17) and so both -std=c++11
and -std=c++17
and the actual standard used by the compile will may be non properly defined.
plugins/actuator/CMakeLists.txt
Outdated
@@ -6,6 +6,8 @@ | |||
# https://www.gnu.org/licenses/old-licenses/lgpl-2.1.html | |||
# at your option. | |||
|
|||
add_compile_options(-std=c++11) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add_compile_options(-std=c++11) |
as suggested in the libraries/private-utils/CMakeLists.txt
comment
Friendly ping @triccyx . |
Ok done but can't try on gazebo 7 |
plugins/actuator/CMakeLists.txt
Outdated
@@ -6,7 +6,8 @@ | |||
# https://www.gnu.org/licenses/old-licenses/lgpl-2.1.html | |||
# at your option. | |||
|
|||
add_compile_options(-std=c++11) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is is still the add_compile_options(-std=c++11)
in libraries/private-utils/CMakeLists.txt
. Can you remove this from here and move the set(CMAKE_CXX_STANDARD
in the main CMakeLists.txt
, so we do not need to duplicate it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can accept by suggest change just with a click, see https://help.github.com/articles/incorporating-feedback-in-your-pull-request/ .
See #44 (comment) . |
5eff40a
to
076395d
Compare
424d50d
to
20612ef
Compare
The last remaining problem is some linker error when linking the tests with Gazebo 7 ( https://travis-ci.com/robotology/gazebo-fmi/jobs/196013982 ) :
The issue is probably in Gazebo, and related to how the Gazebo Server fixture is exported. As the problem is not present in later Gazebo versions, I think it is safe to just test normal compilation on Gazebo7, and disable test for that specific build. |
Add Gazebo7 to Travis, and fix Gazebo 7 compilation problems Tests are disabled in Gazebo7 due to upstream linking problems.
36855be
to
db2244a
Compare
Fixed and rebased. |
Execute Trevis before merge the branch, as at the moment I can't compile with Gazebo7