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

[foxy] Fix bug error handling get_param_files (#743) #819

Closed
wants to merge 0 commits into from

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 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.

3 participants