simonschmeisser commented 4 years ago

I think we should start releasing this and once the assistant pr has landed promote it some more.

Is there anything missing yet besides

[x] Version number greater than 0.0.0
[x] Finish fk test pull request for kinetic. #33
[x] Improve parameter names. #41
[x] Remove hard-coded prefix. #42
[x] Create a release repository. (here)
[x] Port submodule change to kinetic
[x] Merge new C++ standard specification
[x] Run prerelease test again
[x] Finally decide on a proper branching strategy #59
[x] Write documentation. moveit tutorials
[x] release tag and release here (edit: automatically created during the release process, I think.)
[x] release of opw_kinematics https://github.com/Jmeyer1292/opw_kinematics/issues/39 (not necessary anymore!)
[x] adding it to rosdistro (no, I have not read the docs yet ...)

Do we still want to change anything on the parameter names? @gavanderhoorn

Do you want to learn how to do releases @JeroenDM or should I learn it, I have never done it either

Edit: I added the TODO list point from other comments, almost all of them are done now.

gavanderhoorn commented 4 years ago

Do we still want to change anything on the parameter names?

I would like to, as there's nothing MoveIt specific about them, and the kinematics_solver prefix is somehing MoveIt started.

It's not a big deal though, so if ppl want to keep using those prefixes, that would also be OK.

JeroenDM commented 4 years ago

I will try to learn how to release it this weekend, should be interesting :)

Do we start at version 1.0.0 for kinetic and 2.0.0 for melodic? They have an incompatible API, and we need a unique version for each release Another option is 0.0.1 for kinetic and 1.0.0 for melodic. I never really used Semantic Versioning in the real world...

JeroenDM commented 4 years ago

Some other TODO's:

[x] Finish fk test pull request for kinetic. #33
[x] Improve parameter names. #41
[x] Remove hard-coded prefix. #42
[x] Create release repository.

gavanderhoorn commented 4 years ago

Do we start at version 1.0.0 for kinetic and 2.0.0 for melodic? They have an incompatible API, and we need a unique version for each release Another option is 0.0.1 for kinetic and 1.0.0 for melodic. I never really used Semantic Versioning in the real world...

I would suggest to not start releasing with a major number immediately, unless you are satisfied with the current state and want to guarantee your users you won't introduce any breaking changes in upcoming minor releases.

I'd suggest using 0.1.x for Kinetic for now, and 0.2.x for Melodic.

If there are no real differences between the Kinetic and the Melodic version, you could even opt for using the same version nrs for both.

JeroenDM commented 4 years ago

I would suggest to not start releasing with a major number immediately, unless you are satisfied with the current state and want to guarantee your users you won't introduce any breaking changes in upcoming minor releases.

I'd suggest using 0.1.x for Kinetic for now, and 0.2.x for Melodic.

Ok! Good points. I'm not that confident that everything will be smooth if it goes out into the wild :)

If there are no real differences between the Kinetic and the Melodic version, you could even opt for using the same version nrs for both.

25 introduced melodic specific changes. So I think separate version numbers would be appropriate.

edit: #30 is also melodic specific.

JeroenDM commented 4 years ago

Another issue is that the tests have an external unreleased dependency with kuka_test_resources.

Ideally, I want to add some other robots there to improve test coverage, and then actually use the kinematic parameters from this repository.

However, apart from the release problems, it was also recommended to depend on the robot packages directly, instead of using a special resources repository.

gavanderhoorn commented 4 years ago

test_depends are not typically released, unless you really want to.

So I'm not sure how having an unreleased test_depend poses problems.

simonschmeisser commented 4 years ago

Interesting, what about build_depends? Does that mean we do not necessarily need a release of opw_kinematics as it is header-only?

gavanderhoorn commented 4 years ago

build_depends do need to be released, as the buildfarm will only get a source tarball of moveit_opw_kinematics_plugin. All dependencies must be resolvable either as other ROS packages or as system dependencies.

Edit: well, there would be a way around that for opw_kinematics, but it's rather ugly: use a git submodule in moveit_opw_kinematics_plugin which points to opw_kinematics. The ROS buildfarm used to ignore submodules, but it got added as some ppl wanted to use them.

gavanderhoorn commented 4 years ago

Perhaps as a first version of the release we could go with the git submodule.

It would require the least amount of cooperation upstream and keep us (you) in full control.

You would want to make sure to not export the include path from the IK plugin's package. Easiest way to do that would be to only include opw_kinematics headers in .cpp files. Never in public headers.

As with the submodule you can add the path to the headers directly to the include path, no add_subdirectory would be necessary, and if opw_kinematics ever does get released, you'd only need to remove the submodule and add the appropriate find_package(..) and target_link_libraries(..).

JeroenDM commented 4 years ago

Do you know of any project that does something similar that I can use as an example for this setup?

gavanderhoorn commented 4 years ago

Not that I can point you to right now.

It's not that hard to setup though: add a git submodule (plenty of documentation available) and change your CMakeLists.txt to add the location which contains the opw_kinematics headers to the include path of the targets which need it. As opw_kinematics changes very infrequently, tracking upstream with the submodule will not incur too much overhead I believe.

Then remove the #include from the plugins header (I don't believe it's needed there).

Now build as normal.

If you make the changes in a PR, I'll review.

JeroenDM commented 4 years ago

New TODO list:

[x] Port submodule change to kinetic
[x] Merge new C++ standard specification
[x] Run prerelease test again
[ ] Write documentation.

gavanderhoorn commented 4 years ago

:100: for "writing documentation", but I would personally appreciate it if we could make that particular task not a blocker for the release.

Don't assume everything needs to be perfect for a release. Releasing an update is almost zero effort after you have done the initial one (update changelogs, tag, run Bloom). And no one will "see" a release until after things have been synced with the main repositories, so releasing multiple times would not be a problem.

JeroenDM commented 4 years ago

Ok, agreed.

I build the melodic version in a clean docker container and everything seemed to work fine (rosdep install, building and running tests).

Then I also ran prerelease tests on melodic-devel using http://prerelease.ros.org/ but I'm not sure about the output.

Invocation failed without any known error condition, printing all lines to debug known error detection:

The workspace created by the prerelease script /tmp/prerelease_job/ws builds and runs fine.

I'll need to look into it in more detail but I'm hungry now.

gavanderhoorn commented 4 years ago

I'm almost 100% certain that is not caused by your package, but more likely either a transient apt problem or caused by some other external factor.

If building everything in a clean container worked, there's a good chance a release will also build cleanly on the buildfarm.

gavanderhoorn commented 4 years ago

So what's still outstanding?

@JeroenDM: could you perhaps merge https://github.com/JeroenDM/moveit_opw_kinematics_plugin/issues/40#issuecomment-626185579 into the first post here by @simonschmeisser and update the list (ie: mark the boxes which have been completed)?

JeroenDM commented 4 years ago

I just have to merge #61 (can someone maybe quickly glance over it and approve it before I merge?).

I think that's it. :) Edit: I forgot, the branching strategy could change #59, I have to make a decision on that front.

Then I have to make time to go through the release process that creates the pull request on the rosdistro repo. Maybe this evening.

JeroenDM commented 4 years ago

I also ran the pre-release tests on the kinetic branch and it's a success!

One thing that's not clear to me is:

So I'm not sure how having an unreleased test_depend poses problems.

As far as I understand from these slides the ROS build farm does run the tests. For the pre-release tests I always had to add the kuka_test_resources package to the underlay. What am I missing?

(Minor point, it is recommended to add a CHANGELOG.rst file. But since there are no tags yet here, I will just add empty file for future use.)

gavanderhoorn commented 4 years ago

So I'm not sure how having an unreleased test_depend poses problems.

As far as I understand from these slides the ROS build farm does run the tests.

Tests are run for a release job, but the results are not used for anything. They are ignored.

For the pre-release tests I always had to add the kuka_test_resources package to the underlay. What am I missing?

The prerelease job most likely wants to install all dependencies, including test_depends.

If How do I conditionally include a catkin component only for testing? is still the current situation, then test_depend is only used for runtime test dependencies.

I haven't checked whether the dependencies you have as test_depend fall into that category or not.

gavanderhoorn commented 4 years ago

Minor point, it is recommended to add a CHANGELOG.rst file. But since there are no tags yet here, I will just add empty file for future use.)

catkin_generate_changelog creates that file for you. I don't believe you have to create it manually.

gavanderhoorn commented 4 years ago

@JeroenDM: just remembered that Bloom will not send the PR to rosdistro unless you allow it to.

The check for whether all dependencies of packages can be resolved is multiple steps before that. If something isn't right, Bloom will tell you. Almost all of the steps perform a dry-run first, so if it runs into problems, it will be able to detect that.

Worst case you'll have to recreate the release repository.

I would just test running through Bloom to see whether it succeeds. Then answer n to the question at the end whether you want to create a PR to ros/rosdistro.

JeroenDM commented 4 years ago

catkin_generate_changelog creates that file for you. I don't believe you have to create it manually.

The command does not create the file if there are no release tags yet, that's why I mentioned adding it manually. Now that that the tags are there I could update it, but for a first version a change log seems not necessary, as there is no previous version to change against.

I would just test running through Bloom to see whether it succeeds. Then answer n to the question at the end whether you want to create a PR to ros/rosdistro.

I did this for both versions and everything seemed to work as it should. Although the test dependencies end up as build dependencies, as for melodic I got:

Package 'moveit-opw-kinematics-plugin' has dependencies:
Run Dependencies:
  rosdep key           => bionic key
  moveit_core          => ['ros-melodic-moveit-core']
  roscpp               => ['ros-melodic-roscpp']
  pluginlib            => ['ros-melodic-pluginlib']
Build and Build Tool Dependencies:
  rosdep key           => bionic key
  eigen_conversions    => ['ros-melodic-eigen-conversions']
  moveit_core          => ['ros-melodic-moveit-core']
  roscpp               => ['ros-melodic-roscpp']
  pluginlib            => ['ros-melodic-pluginlib']
  catkin               => ['ros-melodic-catkin']
  rostest              => ['ros-melodic-rostest'] 
  moveit_resources     => ['ros-melodic-moveit-resources']
  moveit_ros_planning  => ['ros-melodic-moveit-ros-planning']

The kuka_test_resources package is not added in the package.xml so it does not cause problems.

But if the package does get released even if the tests fail, I can do the release for real. (Or at least create the pull request already.)

JeroenDM commented 4 years ago

The kuka_test_resources package is not added in the package.xml so it does not cause problems.

Note that this is to make the CI happy. I checked again just to be sure that it does not work.

gavanderhoorn commented 4 years ago

Did you get a chance to release @JeroenDM ?

JeroenDM commented 4 years ago

I will run bloom again and create the pull request today.

But I still would like to find a solution for the test dependencies. But we probably better discuss this in a separate issue. #63

JeroenDM commented 4 years ago

I went through the full release process with bloom for both versions:

If I'm fast enough I can write some documentation before they make it to the public :) I still feel a bit uneasy about the failing tests, but I guess this falls under "don't let perfect be the enemy of good" or "move fast and break things".

JeroenDM commented 4 years ago

Both versions where successfully build on the ROS build farm :) (Apart from the 2 Kuka related tests that failed of course.)

Now we wait for the next sync and then we can close this issue I supose.

gavanderhoorn commented 4 years ago

:+1: on the release. Thanks for that.

You could post a question on ROS Answers asking about the support for running tests with unreleased test_depends on the buildfarm. I expect the answer will be: "that's not supported and also not a problem for release jobs (as the test results are ignored)". But let's see.

Devel jobs should however have access to test_depends and I believe that's actually possible by specifying additional repositories as dependencies/prerequisites somehow in the distribution.yaml.

gavanderhoorn commented 4 years ago

Just tested the plugin (using the ros-testing repository).

On Melodic it seems to work. Both in a configuration with a single robot as well as with multiple motion groups.

I'll start adding OPW parameters to the Fanuc support packages.

:hamburger: :beer:

gavanderhoorn commented 4 years ago

Synced to public repositories for both Kinetic (New Packages for Kinetic Kame 2020-05-28) and Melodic (New packages for Melodic 2020-05-28).

JeroenDM commented 4 years ago

@gavanderhoorn fwi, I finally created the ROS answers question.

JeroenDM commented 4 years ago

It looks like opw_kinematics is being released.

gavanderhoorn commented 3 years ago

@JeroenDM: https://github.com/Jmeyer1292/opw_kinematics/issues/39 was just closed, as opw_kinematics is now available on Noetic.

A Noetic release of the MoveIt plugin would also be nice.

It would now no longer need to come with opw_kinematics included (as is the case for the Melodic version).

JeroenDM commented 3 years ago

That's good news! I don't have time now, but I hope I can get to this next month or so. Feel free to ping me again if I forget about it ;) It's been a while since I did something useful here.

gavanderhoorn commented 3 years ago

Would it be too early for a friendly ping?

Seeing as you already have a Melodic release, releasing to Noetic should not be too difficult.

gavanderhoorn commented 3 years ago

@JeroenDM: ping?

JeroenDM / moveit_opw_kinematics_plugin

Release? #40

25 introduced melodic specific changes. So I think separate version numbers would be appropriate.