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

use ament_target_dependencies where possible to order include dirs #658

Closed
dirk-thomas opened this issue Feb 20, 2019 · 11 comments
Closed
Assignees
Labels
enhancement New feature or request

Comments

@dirk-thomas
Copy link
Member

dirk-thomas commented Feb 20, 2019

Follow up of ament/ament_cmake#130.

@ivanpauno
Copy link
Member

ivanpauno commented Mar 13, 2019

Am I in the right direction here ros2/rosidl_typesupport#46 and here ros2/geometry2#98?

If it is ok, I can take this issue

@ivanpauno
Copy link
Member

@dirk-thomas This three PRs need a review:
ros/class_loader#121
ros2/rosidl_typesupport#46
ros2/rcl#400

I will run a CI job with this complete update after approval of the PRs.

@wjwwood
Copy link
Member

wjwwood commented Mar 18, 2019

@ivanpauno do you want to do CI for ros2/rviz#384 as well when you do the others? There's some test failures in the CI I ran, but it might be known issues.

@dirk-thomas
Copy link
Member Author

This three PRs need a review:

I have already reviewed the second and third. @nuclearsandwich will review the first since he is the maintainer of that package/repo.

Please also set the label to match the status a ticket is supposed to have ("in review" rather than "in progress").

@ivanpauno ivanpauno added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Mar 18, 2019
@jacobperron
Copy link
Member

@ivanpauno It looks like all referenced PRs have been merged. Is there still more to do for this ticket or can it be closed?

@ivanpauno
Copy link
Member

@ivanpauno It looks like all referenced PRs have been merged. Is there still more to do for this ticket or can it be closed?

I will check again before closing it.

@clalancette
Copy link
Contributor

I'm actually going to close this out just to get it out of our review column. Feel free to reopen if you find more issues that need to be addressed as part of this.

@clalancette clalancette removed the in review Waiting for review (Kanban column) label Apr 29, 2019
@dirk-thomas
Copy link
Member Author

@clalancette You do we ensure that this will be checked with the ticket closed?

@clalancette
Copy link
Contributor

@clalancette You do we ensure that this will be checked with the ticket closed?

Nothing, but there is nothing to ensure it will be checked with it open either. Basically there is nothing actionable here, and having it just hanging out in the review column with everything merged means it is just wasting reviewer time looking at it for no reason. I could alternatively open it back up and switch to "in progress", but it definitely shouldn't be "in review".

@dirk-thomas
Copy link
Member Author

Currently there is no ticket to follow up on "I will check again before closing it.". Therefore I will reopen it.

@dirk-thomas dirk-thomas reopened this Apr 29, 2019
@ivanpauno
Copy link
Member

ivanpauno commented May 6, 2019

I opened two PRs, addressing the leftovers I've found. I will close this issue after merging them.

@ivanpauno ivanpauno removed the in progress Actively being worked on (Kanban column) label May 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants