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

Crystal Patch Release 2 #647

Closed
nuclearsandwich opened this Issue Jan 16, 2019 · 24 comments

Comments

Projects
None yet
7 participants
@nuclearsandwich
Copy link
Member

nuclearsandwich commented Jan 16, 2019

This issue will track the list of changes that should be included in the next Crystal patch release (after the first one: #638). This patch release is planned for 2019-02-14. Follow the discussion for timeline updates.

PRs merged for this patch release:

Support rqt_gui_cpp on Xenial
Add API to get multiple parameters in a group
New `sros2_cmake` package
Patch releases to fix ros2/ros2#648
Fix CMake files for Action interface dependencies
Consistent node naming in ros2cli
Install example scripts into package-specific locations to avoid collisions.
Add COLCON_PREFIX_PATH hook to ros_workspace ros2/ros2#653
Publish logs to rosout
Fix Node Graph implementation on rmw_opensplice and remove test hack
Remove action mapping for ros1_bridge.
Add the ability to control teleop_twist_joy via parameters

Changes still in progress for this patch release

Changes in consideration for this patch release

Enable rcl_logging_log4cxx build

All merged PRs that will be released
@nuclearsandwich

This comment has been minimized.

Copy link
Member Author

nuclearsandwich commented Jan 23, 2019

Added the plan to fix #648 in the next patch release.

@jacobperron

This comment has been minimized.

Copy link
Member

jacobperron commented Jan 26, 2019

I think we can consider the new package sros2_cmake for the patch release (ros2/sros2#75).

@mjcarroll

This comment has been minimized.

Copy link
Contributor

mjcarroll commented Jan 28, 2019

On sros2, I would also like to squeeze this in: ros2/sros2#72 but it still requires some CI changes.

@dirk-thomas

This comment has been minimized.

Copy link
Member

dirk-thomas commented Jan 29, 2019

A bug fix for the problem in the Debian packages described in #653 should be consider.

@chapulina

This comment has been minimized.

Copy link
Contributor

chapulina commented Jan 29, 2019

A few more fixes to consider:

@sloretz

This comment has been minimized.

Copy link

sloretz commented Feb 2, 2019

ros2/examples#226 would be a good one. It changes where the examples install their entry points so they're not accidentally included.

@dirk-thomas

This comment has been minimized.

Copy link
Member

dirk-thomas commented Feb 5, 2019

The following PR needs to be in to fix action which have fields using other packages: ros2/rosidl#343

@nuclearsandwich

This comment has been minimized.

Copy link
Member Author

nuclearsandwich commented Feb 5, 2019

I've updated the issue body.

@jacobperron: I think we can consider the new package sros2_cmake for the patch release (ros2/sros2#75).

I've added this to the to-be-included list.

@mjcarroll: On sros2, I would also like to squeeze this in: ros2/sros2#72 but it still requires some CI changes.

I haven't pulled this in above because I think the CI configuration still has open questions. It will have to wait for a future patch release.

@dirk-thomas: A bug fix for the problem in the Debian packages described in #653 should be consider.

This has been added to the release's in progress changes with the anticipation that it will merge before the end of the week (ball is in my court to create an off-farm test package)

@chapulina: A few more fixes to consider:

* [ros2/rmw_opensplice#255](https://github.com/ros2/rmw_opensplice/pull/255)

* [ros2/rcl#368](https://github.com/ros2/rcl/pull/368)

These two changes will require backport PRs to the respective crystal branches as mainline on those packages have broken compatibility in preparation for the Dashing.

* [ros2/ros2cli#158](https://github.com/ros2/ros2cli/pull/158)

This change has been added to the release queue.

@sloretz: ros2/examples#226 would be a good one. It changes where the examples install their entry points so they're not accidentally included.

This change has been added to the release queue.

@dirk-thomas: The following PR needs to be in to fix action which have fields using other packages: ros2/rosidl#343

This change was somehow missed in the previous patch release process along with some of the uncrustify updates and is in the queue for this one.

@jacobperron if you think there's a possibility of the Actions in Python work being ready by the end of the day Friday could you please link those PRs here. If not then we'll target them for patch release 3.

@dirk-thomas

This comment has been minimized.

Copy link
Member

dirk-thomas commented Feb 5, 2019

ros2/rmw_opensplice#255

@chapulina Why should this be backported? What exact problems does it fix?

@ross-desmond

This comment has been minimized.

Copy link

ross-desmond commented Feb 6, 2019

ros2/rmw_opensplice#255

@chapulina Why should this be backported? What exact problems does it fix?

This fixes tests for opensplice when searching for a topic via a ros node in the dynamic node graph. Prior to this fix in rmw-opensplice, a node was unable to lookup it's own topics and services discovered through the Simple Discovery Protocol. This enables rmw-opensplice to behave the way rmw-connext and rmw-fastrtps in regards to the node graph API.

@dirk-thomas

This comment has been minimized.

Copy link
Member

dirk-thomas commented Feb 6, 2019

This fixes tests for opensplice when searching for a topic via a ros node in the dynamic node graph.

This on its own is not a reason for backporting it.

Prior to this fix in rmw-opensplice, a node was unable to lookup it's own topics and services discovered through the Simple Discovery Protocol. This enables rmw-opensplice to behave the way rmw-connext and rmw-fastrtps in regards to the node graph API.

This certainly is a good reason to backport it. (It might be good to mention this on the PR itself in case future reader wonder.) Thanks for clarifying 👍

@jacobperron

This comment has been minimized.

Copy link
Member

jacobperron commented Feb 6, 2019

@jacobperron if you think there's a possibility of the Actions in Python work being ready by the end of the day Friday could you please link those PRs here. If not then we'll target them for patch release 3.

I'd say there is a chance for Actions in Python to be ready.
Here are the relevant PRs:

Time permitting, there may also be another PR related to client adding some nice-to-have feature that was missed.

EDIT: added the examples to the list.

@nuclearsandwich

This comment has been minimized.

Copy link
Member Author

nuclearsandwich commented Feb 6, 2019

As an update, the rosout logging may either be dropped from the patch release or need further changes as it makes new assumptions about node name uniqueness. The conversation starts with ros2/rcl#350 (comment) and is so far continuing there.

At this time my intention is to continue with the patch release as scheduled and drop the rcl changes from it pending the outcome of that discussion. I don't believe the backport of ros2/rcl#368 is critical and I also believe we can backport ros2/rmw_opensplice#255 without it but please correct me if I am mistaken.

Update: As referenced below the new current plan is to update the change so that it issues a warning instead. Context is in ros2/rcl#350 (comment)

@dirk-thomas

This comment has been minimized.

Copy link
Member

dirk-thomas commented Feb 6, 2019

I am wondering if ros2/rmw_fastrtps#238 and ros2/rmw_fastrtps#254 are fixing an important enough issue (avoid a race when waiting for a service) which is worth being backported to Crystal?

@ross-desmond

This comment has been minimized.

Copy link

ross-desmond commented Feb 6, 2019

I don't believe the backport of ros2/rcl#368 is critical and I also believe we can backport ros2/rmw_opensplice#255 without it but please correct me if I am mistaken.

You COULD backport ros2/rmw_opensplice#255 without ros2/rcl#368, but this would result in code in rmw_opensplice that would not be tested. The rcl changes are simply 2 deletions that enable testing of rmw_opensplice node graph

@nuclearsandwich

This comment has been minimized.

Copy link
Member Author

nuclearsandwich commented Feb 6, 2019

I am wondering if ros2/rmw_fastrtps#238 and ros2/rmw_fastrtps#254 are fixing an important enough issue (avoid a race when waiting for a service) which is worth being backported to Crystal?

Given that Fast-RTPS is our default rmw provider I'd be inclined to backport improvements. My biggest concern would be if those changes are compatible with Fast-RTPS 1.7.0 rather than master.

@nuclearsandwich

This comment has been minimized.

Copy link
Member Author

nuclearsandwich commented Feb 6, 2019

You COULD backport ros2/rmw_opensplice#255 without ros2/rcl#368, but this would result in code in rmw_opensplice that would not be tested.

Thanks for the feedback here @ross-desmond now that we've got a path to resolving the rosout node name kerfuffle (see ros2/rcl#350 (comment)) my updated intention (see #647 (comment) for previous) is to release rcl once the amended rosout behavior has been merged and backported, so we can backport this rcl change along with it. Although it is potentially moot I am not too concerned about code not being tested directly in crystal since we know it's getting tested on master. But that concern will increase as master and crystal diverge so I'd prefer to backport the test change as well and would only have avoided it to avoid releasing on an unstable branch.

@nuclearsandwich

This comment has been minimized.

Copy link
Member Author

nuclearsandwich commented Feb 7, 2019

While reviewing devel jobs yesterday I saw that ros2cli relies on unreleased changes in rclpy. Which reminds me to run my script checking for other unreleased changes.

@nuclearsandwich

This comment has been minimized.

Copy link
Member Author

nuclearsandwich commented Feb 8, 2019

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

This is a test of the current state of Crystal's development branches with the backport PRs I just opened. At least it will be if I set it up correctly. There are some repositories that have changes on their development branch which won't be released in this patch release but this is the easiest way to test things before actually starting to make releases.

@nuclearsandwich

This comment has been minimized.

Copy link
Member Author

nuclearsandwich commented Feb 8, 2019

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

The failure in the previous build is related to ament/ament_cmake@06f842e which isn't scheduled for release in this patch release. I've started a new round of builds pinning that repo to it's Crystal release.

Update: The fix for this issue has been proposed for backport ros2/rmw#166

Update: These jobs are failing because my repos file generator doesn't handle system_tests which isn't released in Crystal. Maybe we should add it to the distribution.yaml without a release.

@nuclearsandwich

This comment has been minimized.

Copy link
Member Author

nuclearsandwich commented Feb 14, 2019

  • Linux Bionic Build Status
  • Linux-aarch64 Bionic Build Status
  • Linux Xenial Build Status
  • Linux-aarch64 Xenial Build Status
  • macOS Build Status
  • Windows Build Status
@nuclearsandwich

This comment has been minimized.

Copy link
Member Author

nuclearsandwich commented Feb 14, 2019

Flake8 tests are failing in these builds because the Crystal track doesn't include ament/ament_lint#124 which is required to use flake8 >= 3.7.0. When installing from debs Xenial uses 2.5.4 and Bionic uses 3.5.0. I don't feel like this is a release blocker but we ought to consider the ament_lint PR for backport in a future patch release to make it easier for our CI to test Crystal.

@nuclearsandwich

This comment has been minimized.

Copy link
Member Author

nuclearsandwich commented Feb 15, 2019

Packaging jobs:

  • Linux AMD64 Bionic Build Status
  • Linux ARM64 Bionic Build Status
  • Linux AMD64 Xenial Build Status
  • Linux ARM64 Xenial Build Status
  • macOS Build Status
  • Windows Build Status
  • Windows Debug Build Status

Update, the macos job failed with an infrastructure issue. Build Status

@nuclearsandwich

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment