Skip to content

Conversation

@jacobperron
Copy link
Member

Backport #743 to Foxy.

@jacobperron
Copy link
Member Author

Looks like there may be a prerequisite I've missed in order to backport this change.

@ivanpauno
Copy link
Member

ivanpauno commented Oct 2, 2020

rcl/test/rcl/test_arguments.cpp is missing an include to rcl/test/rcl/allocator_testing_utils.h, that was added in a PR before.

IMO, backporting the PRs adding those tests isn't needed.
I would only backport the one line fix and delete all the new test cases.

@brawner
Copy link
Contributor

brawner commented Oct 3, 2020

I would recommend backporting test coverage PRs to foxy. @chapulina and @ahcorde can also weigh in on that

@ivanpauno
Copy link
Member

I would recommend backporting test coverage PRs to foxy. @chapulina and @ahcorde can also weigh in on that

Backporting tests is ok, but that shouldn't block the backport of a (one line) fix.
If the test utilities haven't been backported yet, the test cases depending on them can be backported in the future.

@brawner
Copy link
Contributor

brawner commented Oct 5, 2020

@ivanpauno that's reasonable. Tests can be backported in a separate PR

@jacobperron
Copy link
Member Author

@ros-pull-request-builder retest this please

@jacobperron
Copy link
Member Author

There's a missing header include that was added in #703, I'm not sure if there are plans to backport it. If not, then I can cherry-pick the missing include line.

@ivanpauno
Copy link
Member

There's a missing header include that was added in #703, I'm not sure if there are plans to backport it.

I think that most of the test coverage that was added to Rolling is being backported to Foxy, but I'm not sure about that.
@brawner probably knows.

If not, then I can cherry-pick the missing include line.

👍
You can also only backport the changes in rcl/src/rcl/arguments.c without backporting the tests, I'm ok with what you prefer doing.

@jacobperron
Copy link
Member Author

jacobperron commented Oct 23, 2020

I asked around and it sounds like there are plans to backport #703, so I'll just wait.

@jacobperron jacobperron force-pushed the jacob/backport_foxy_743 branch from 1f6d797 to 4376552 Compare November 10, 2020 19:16
@jacobperron jacobperron deleted the jacob/backport_foxy_743 branch November 10, 2020 19:17
@jacobperron
Copy link
Member Author

I don't have a reference, but this was backported to Foxy elsewhere.

@brawner
Copy link
Contributor

brawner commented Nov 10, 2020

I think in #848

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.

4 participants