Skip to content

Maintainer Guide

Jump to bottom Edit New page
Robert Haschke edited this page Mar 8, 2019 · 4 revisions

The purpose of this guide is to help people who are learning to maintaining this repository or as a reference to existing maintainers.

It's based on notes taken by @dhood, but was updated for posting publicly by @wjwwood.

Triage of Issues

Whenever spending time triaging issues and pull requests, follow these guidelines so other maintainers (or your future self) can know what has been triaged and what has not been triaged yet.

  • Issues or pull requests without a label are considered un-triaged
  • Apply at least one label when triaging an issue or pull request
    • Either a descriptive label like "bug" or "enhancement" or
    • a state label like "backlog", "in progress", or "review", or
    • a status label like "needs feedback or more info" or "ABI break" or "changes requested"
  • Use https://waffle.io/ros-visualization/rviz to help with visualizing what has and has not been triaged
    • Items in the "Inbox" have not been triaged and need to be moved into either "Backlog" or "In Progress" or "Review", which will apply some label

Briefly some labels that might be of interest (non-exhaustive):

  • "needs feedback or more info"
    • we've asked for more information from the issue reporter or pull request author, so "the ball is in their court"
  • "changes requested"
    • we've asked the author of a pull request to change something before merging, again "ball is in their court"
  • "ABI break"
    • we've determined the change may cause ABI breakage, which is used when determining when/where to merge it
  • "help wanted"
    • we've triaged the issue, but don't have time to work on it, so we're asking for help from the community on it

ABI Compatibility

By default, ABI breakage should be target at the next ROS release and should not be included in existing releases. The "ABI break" label should be used to mark issues and pull requests where this occurs or is required.

Notes:

  • We have automatic ABI checking on Travis using moveit_ci. However, in case of errors, the commandline output is not very helpful. The script can be run locally by following the guidelines here and subsequently inspecting the generated html report.
  • If an ABI break gets released, ROS buildfarm (currently) will rebuild every package depending on RViz anyway, so it's just off-buildfarm packages that break (or users that only update rviz package).
  • There was one occasion where we decided against abi-checker's result because it might be an issue on other compilers: https://github.com/ros-visualization/rviz/pull/1221

Branch Management

There may be a branch of development for each ROS distribution rviz is released into, but there need not be.

For example, the version of rviz released into Jade and Indigo both might come from the indigo-devel branch. If, at some point, Jade and Indigo needed to diverge, a jade-devel branch could be made. However, the version for Jade should not be released from a newer ROS distro's branch, e.g. from kinetic-devel.

When making a first release into a new ROS distribution is ok to use an existing devel branch and create one specifically for the new release at some point in the future.

Try to reserve a minor version number for each ROS distribution, e.g. if you jump from A-turtle to C-turtle (releasing B-turtle from the A-turtle devel branch) then use 1.0.x for A-turtle releases and 1.2.x for C-turtle releases, in case you ever need to make a separate B-turtle release which can be 1.1.x. We have not always done this, but it's a good idea for the future.

Release Process

There's nothing special about the general process for rviz beyond what's detailed in these tutorials:

But here are some notes on things that are optional from the tutorial:

  • Generate the changelog and then edit it to:
    • remove extra commit messages and only leave a message which is concise
    • use the past tense ('Add' versus 'Added')
    • link to github issues and pull requests properly
    • ok to leave messages about back-ported or forward-ported features

Make sure to do an acceptance test before releasing, just compile the "tip" of the branch and make sure rviz runs and can do basic stuff:

  • run the visualization tutorials, e.g. rosrun interactive_marker_tutorials basic_controls
  • run the navigation tutorials, e.g. roslaunch navigation_stage move_base_amcl_2.5cm.launch

Releasing to a new distro for the first time

Again, start with the existing tutorials:

When doing the first release, as mentioned above, you can defer the decision to create a separate devel branch till later, but it's also ok to do it on (or before) the first release, especially if you already have ABI breaking changes queued up for the next release.

Reviewing Pull Requests

In order to merge a pull request:

  • it should pass automatic CI tests
  • at least one maintainer should do a code review
    • don't be afraid to ask for consult from another maintainer if you're unsure
  • you should reproduce the issue reported and then rebuild with the patch to ensure it actually fixes the problem
    • I've seen really nicely put together pr's (with screenshots and a good write up) not even run (compile but crash), "trust but verify"
  • Notes about the tests:
  • Notes about Licensing:

When merging a pull request, default to using the "Squash and Merge" option in the GitHub WebUI, in order to make it easier to cherry-pick commits between branches which we do when forward and back porting changes.

Add a custom footer
Add a custom sidebar
Clone this wiki locally