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

Fixes for ros2/rcl PR #882 #66

Merged
merged 5 commits into from
Mar 26, 2021
Merged

Conversation

norro
Copy link
Collaborator

@norro norro commented Mar 25, 2021

Fixes following API changes in ros2/rcl#882.

Signed-off-by: Nordmann Arne (CR/ADT3) arne.nordmann@de.bosch.com

Signed-off-by: Nordmann Arne (CR/ADT3) <arne.nordmann@de.bosch.com>
@norro norro self-assigned this Mar 25, 2021
norro added 2 commits March 25, 2021 15:34
Signed-off-by: Nordmann Arne (CR/ADT3) <arne.nordmann@de.bosch.com>
Signed-off-by: Nordmann Arne (CR/ADT3) <arne.nordmann@de.bosch.com>
@norro norro marked this pull request as draft March 25, 2021 14:38
Signed-off-by: Nordmann Arne (CR/ADT3) <arne.nordmann@de.bosch.com>
@norro
Copy link
Collaborator Author

norro commented Mar 25, 2021

This PR is fixing rclc for rolling. Also, it is building against rcl master now, no longer against the latest rcl package. This should make us react to RCL changes quicker in the future.

The PR is ready to merge, IMHO, but:

  • as it is now only building for rolling, we may want to introduce a dashing branch. A foxy branch is already existing, is this one also used to release for dashing?
  • it is currently only fixing for the new API, not yet exposing the new feature (optional comm interface in the lifecycle). I would like to expose this feature in rclc_lifecycle as well. However, if you want to merge the fix now, feel free to do so and I will create a separate PR for this feature. @pablogs9, @JanStaschulat

@norro norro requested review from JanStaschulat and pablogs9 March 25, 2021 14:57
@norro norro marked this pull request as ready for review March 25, 2021 14:57
@pablogs9
Copy link
Member

Since this breaks the API I have changed all the foxy build system to target RCLC foxy branch: micro-ROS/micro_ros_setup#296

@pablogs9 pablogs9 mentioned this pull request Mar 26, 2021
@JanStaschulat
Copy link
Contributor

JanStaschulat commented Mar 26, 2021

regarding @norro 's comment:

  1. as it is now only building for rolling, we may want to introduce a dashing branch. A foxy branch is already existing, is this one also used to release for dashing?
  2. it is currently only fixing for the new API, not yet exposing the new feature (optional comm interface in the lifecycle). I would like to expose this feature in rclc_lifecycle as well. However, if you want to merge the fix now, feel free to do so and I will create a separate PR for this feature. @pablogs9, @JanStaschulat

Hmm,

  1. can we live with the foxy branch for the dashing release as well? As EOL of Dashing is May 2021 I would use the Foxy branch for Dashing Release as well.
    This would need a change in the CI-job.
  2. did not understand which options we have.

@norro
Copy link
Collaborator Author

norro commented Mar 26, 2021

@JanStaschulat

  1. yes, we can go with the foxy branch for dashing as well. I have no problem with that.
  2. I just learned that all other changes only make sense after Provide lifecycle services in the rclc lifecycle nodes #51 has been merged. So this PR is ready to merge now.

@pablogs9
Copy link
Member

Ok from my part, merge if you both are ok

@JanStaschulat
Copy link
Contributor

Why does this PR not build for Rolling on the ros2 build farm? @norro could you check this?

@norro
Copy link
Collaborator Author

norro commented Mar 26, 2021

Why does this PR not build for Rolling on the ros2 build farm? @norro could you check this?

I don't know much about the configuration of the ROS 2 build farm and how this connects to github.
Maybe they configured to only build master and don't automatically build all PRs? No idea.

@JanStaschulat
Copy link
Contributor

Why does this PR not build for Rolling on the ros2 build farm? @norro could you check this?

I don't know much about the configuration of the ROS 2 build farm and how this connects to github.
Maybe they configured to only build master and don't automatically build all PRs? No idea.

there is only one compiler warning, which needs to be fixed:

https://build.ros2.org/job/Rpr__rclc__ubuntu_focal_amd64/90/gcc/

/tmp/ws/src/rclc/rclc_lifecycle/src/rclc_lifecycle/rclc_lifecycle.c: In function ‘rcl_lifecycle_node_fini’:
/tmp/ws/src/rclc/rclc_lifecycle/src/rclc_lifecycle/rclc_lifecycle.c:232:21: warning: unused parameter ‘allocator’ [-Wunused-parameter]
  232 |   rcl_allocator_t * allocator

https://build.ros2.org/job/Rpr__rclc__ubuntu_focal_amd64/90/consoleText

Signed-off-by: Jan Staschulat <jan.staschulat@de.bosch.com>
@JanStaschulat
Copy link
Contributor

JanStaschulat commented Mar 26, 2021

  • created a PR to update the source branch for Dashing bloom release: (so that future PR on master branch (for Roling release) will not be built for Dashing any more: Updating source branch of rclc-repo for Dashing ros/rosdistro#28855
  • updated branch protection rules for ros2/rclc repo master and foxy branch (specified which checks shall be listed here and which shall be shown as required)

@JanStaschulat JanStaschulat merged commit 90f47c9 into ros2:master Mar 26, 2021
@JanStaschulat JanStaschulat deleted the fix-for-rcl-PR-882 branch March 26, 2021 13:22
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.

3 participants