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

Improvement ideas from MoveIt! maintainers #75

Closed
130s opened this issue Jul 12, 2016 · 8 comments
Closed

Improvement ideas from MoveIt! maintainers #75

130s opened this issue Jul 12, 2016 · 8 comments

Comments

@130s
Copy link
Member

130s commented Jul 12, 2016

Thorough feedback from @davetcoleman moveit/moveit_core#309 (comment)

  • it is too verbose and would timeout Travis builds, so it was unusable for MoveIt!
  • it has become really bloated with unnecessary features such as abstracting catkin_tools and removing antiquated manifest.xml files
  • still had code from hydro
  • used a weird folding style that was not as easy to read as regular travis files
  • required long build times in Docker

(links are lost so better to look at the original post).

He had to come up with MoveIt! original CI configs since industrial_ci didn't meet its need :/ but understand that.

All of these sound reasonable, and I believe many of them overlap the existing tickets. I just opened this ticket as a pointer.
Travis' output seems higher priority.

@mathias-luedtke
Copy link
Member

I think it should be possible to combine both again.
The only moveit-specific part of https://github.com/ros-planning/moveit_ci is the hard-coded docker repository name https://github.com/ros-planning/moveit_ci/blob/master/travis.sh#L40
This could be a customization option.

And the prerelease test could be branched of at https://github.com/ros-planning/moveit_ci/blob/master/travis.sh#L25

So we could use moveit_ci as a basis and add all relevant industrial_ci features.

I'd propose the following structure (names may vary):

  • ci_main.sh: test branching and docker wrapping (with docker build as fallback)
  • util.sh: travis folding etc
  • check_clang_format.sh: as-is
  • ros_pre-release.sh: as-is
  • test_source.sh actual source test, run in docker
  • travis.sh entry point and compatibility layer

@130s, @davetcoleman: what do you think?

@davetcoleman
Copy link
Contributor

I agree re-combining the moveit fork of industrial_ci is probably a good idea so that we can combine our efforts. I forked ours because I identified too many issues that we needed addressed quickly for MoveIt! and I thought it would be easier to move quickly in a fork.

Making moveit_ci project-agnostic would have downsides though - we've encoded various simplifying assumptions such as using catkin_tools and only supporting currently supported ROS versions (I/J/K). There are probably others I'm not thinking of.

Finally if we consolidated it makes sense to me it would live on ros-industrial or, preferably, some "ros"-all location. But I would like some of the moveit maintainers to have access to review and merge changes.

@mathias-luedtke
Copy link
Member

I agree re-combining the moveit fork of industrial_ci is probably a good idea so that we can combine our efforts.

And other bigger projects might benefit from providing customized docker images as well.

Making moveit_ci project-agnostic would have downsides though - we've encoded various simplifying assumptions such as using catkin_tools and only supporting currently supported ROS versions (I/J/K). There are probably others I'm not thinking of.

I don't think that this is a big issue. At a first glance catkin_make support might be broken anyway.

@130s
Copy link
Member Author

130s commented Dec 19, 2016

+1 for merging the best from both. moveit_ci is much more sophisticated thanks to Dave while industrial_ci retains verbose features.

I'm afraid consolidation could likely lead to slower review process. I ask to add some of you to co-maintain if possible. Both MoveIt! and ROS-i enforce +1-before-merge rule, no culture shock :)

Making moveit_ci project-agnostic would have downsides though - we've encoded various simplifying assumptions such as using catkin_tools and only supporting currently supported ROS versions (I/J/K). There are probably others I'm not thinking of.

I don't think that this is a big issue. At a first glance catkin_make support might be broken anyway.

Finally if we consolidated it makes sense to me it would live on ros-industrial or, preferably, some "ros"-all location. But I would like some of the moveit maintainers to have access to review and merge changes.

+1

For hosting location, I don't know. Some thoughts:

  • It's ros-i community that contributed the most to this package so I would like to stick here.
  • While industrial_ci isn't limited to ros-i projects and has been used from many non ros-i repos (to some of which I opened PRs to use industrial_ci to admittedly), I'm also afraid that this package residing in ros-i org and its package name may have given people a false impression that it's limited to industrial context (despite readme mentioning there's no limitation to ros-i).
  • If we change the location, URL would change so all "client" repos need to update the URL. Or industrial_ci can just redirect so URL is not an issue.

@davetcoleman
Copy link
Contributor

Seems like we should start a discussion on Discourse about a standard ROS CI configuration... it could live in https://github.com/ros-infrastructure/ @wjwwood

@wjwwood
Copy link

wjwwood commented Jan 3, 2017

You can put stuff in ros-infrastructure if it would work for any ROS repository and it is specific to the ROS infrastructure (rospkg, rosdep, rosdistro, etc.).

There are quite a few options already: http://wiki.ros.org/CIs

@davetcoleman
Copy link
Contributor

+1 to adding it to the ros-infrastructure
The options on there already include this repo - industrial_ci - and we are advocating we just move/rename it.
I looked at the other options on that wiki and they do not appear to be good replacements for what this CI does.

@130s
Copy link
Member Author

130s commented Jul 20, 2017

I think this is moved to #98?

@130s 130s closed this as completed Jul 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants