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

fix .travis.rosinstal to use fkanehiro/hrpsys-base #248

Closed

Conversation

Naoki-Hiraoka
Copy link
Contributor

Currently rtmros_gazebo is tested with https://github.com/start-jsk/hrpsys, which is a deprecated repository.

.travis.rosinstal is fixed to use https://github.com/fkanehiro/hrpsys-base.

@pazeshun pazeshun self-requested a review April 9, 2020 05:02
Copy link
Collaborator

@pazeshun pazeshun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I overlooked this issue.
Please also update openhrp3 like https://github.com/start-jsk/rtmros_common/blob/master/.travis.rosinstall.kinetic#L1-L4, because https://github.com/start-jsk/openhrp3 also seems deprecated.

In addition, could you comment out the following allow_failures:

allow_failures:
- env: ROS_DISTRO=hydro ROSWS=wstool BUILDER=catkin USE_DEB=false
- env: ROS_DISTRO=indigo ROSWS=wstool BUILDER=catkin USE_DEB=false
- env: ROS_DISTRO=kinetic ROSWS=wstool BUILDER=catkin USE_DEB=false

like https://github.com/jsk-ros-pkg/jsk_pr2eus/blob/master/.travis.yml#L34-L35?
This PR may fix failure of those USE_DEB=false tests.

@Naoki-Hiraoka
Copy link
Contributor Author

It takes too long time to test with USE_DEB=false USE_TRAVIS=true.
We have to use USE_DEB=false USE_JENKINS=true instead.

I found a bug on using ' in BEFORE_SCRIPT on jenkins.
jsk-ros-pkg/jsk_travis#171 (comment)
I had to avoid using ' in BEFORE_SCRIPT
9ca04fa

@Naoki-Hiraoka
Copy link
Contributor Author

rtmros_gazebo and rtmros_tutorials depend on each other.
It is problematic.

I changed only rtmros_tutorials to be dependent on rtmros_gazebo.
78cbeb5
Naoki-Hiraoka/rtmros_tutorials@3c23bc4

- git:
uri: https://github.com/start-jsk/rtmros_tutorials
local-name: rtm-ros-robotics/rtmros_tutorials
# rtmros_tutorials depends on rtmros_gazebo. So rtmros_tutorials is not included here.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rtmros_tutorials is added at 2a8b8a4.
And I think rtmros_tutorials should be here, because when we want to change base packages in rtmros_gazebo (e.g., hrpsys_gazebo_general), we have to check this with rtmros_tutorials, which is what we use.
rtmros_common has tests from this thought: https://github.com/start-jsk/rtmros_common/blob/master/.travis.yml#L31

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I don't know that rtmros_gazebo/hrpsys_gazebo_atlas depends on rtmros_tutorials/hrpsys_ros_bridge_tutorials...
My previous comment is still effective, but I also think this dependency should be resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think when we want to change base packages in rtmros_gazebo (e.g., hrpsys_gazebo_general), we only have to check this with SampleRobot, which can be simulated without rtmros_tutorials ???

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I forgot about SampleRobot.
And I also forgot that HRP2 is closed robot and cannot be tested in this repository.

So problem of removing rtmros_tutorials seems mainly about hrpsys_gazebo_atlas.
You are preventing hrpsys_gazebo_atlas from being built, aren't you?
I'm against this idea, because we will hardly notice when we break
hrpsys_gazebo_atlas until we explicitly check travis of rtmros_tutorials.
I think this makes travis meaningless.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about leaving rtmros_tutorials for now, and fixing hrpsys_gazebo_atlas not to depend on rtmros_tutorials in anorther PR? (that PR can be postponed)
Or that solution cannot work on melodic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are preventing hrpsys_gazebo_atlas from being built, aren't you?

In this commit, hrpsys_gazebo_atlas is built, but not tested.
In this commit,
BUILD_PKGS="",
TEST_PKGS="eusgazebo hrp2jsk_moveit_config hrp2jsknt_moveit_config hrp2jsknts_moveit_config hrp2w_moveit_config hrpsys_gazebo_msgs hrpsys_gazebo_general samplerobot_moveit_config staro_moveit_config".

In travis.sh,

catkin build $BUILD_PKGS #build all packages
catkin build --catkin-make-args run_tests -- -i --no-deps --no-status $TEST_PKGS #not test hrpsys_gazebo_atlas

fixing hrpsys_gazebo_atlas not to depend on rtmros_tutorials

It sounds good. I fixed this in #249 ( 06a78e1 )

@Naoki-Hiraoka
Copy link
Contributor Author

This PR is included in #249

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