Skip to content

Removing obsolete message generation.#476

Merged
2 commits merged intomasterfrom
unknown repository
Dec 11, 2018
Merged

Removing obsolete message generation.#476
2 commits merged intomasterfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Dec 11, 2018

VoxelGrid is part of the nav2_msgs package. Doesn't need to be in
costmap_2d.


Basic Info

Info Please fill out this column
Ticket(s) this addresses #473
Primary OS tested on Ubuntu
Robotic platform tested on NA

Description of contribution in a few bullet points

@ghost ghost requested review from SteveMacenski and mkhansenbot December 11, 2018 18:18
@SteveMacenski
Copy link
Member

SteveMacenski commented Dec 11, 2018

So costmap 2d actually did build correctly in the build farm on the last release, so I'm reluctant to change this without due cause this round in case it causes other up stream problems. I dont see anything here I would take issue with however

Edit: I'll let Matt decide what he wants to do here

@ghost
Copy link
Author

ghost commented Dec 11, 2018

This would affect packages that depend on costmap_2d rather than costmap_2d itself. I saw those same rosidl_default_generator errors because of this problem. For me, the package affected was costmap_queue.

My guess is we will see the problem come up again depending on exact build order. However, I'm OK with waiting on this if the build farm is passing at the moment.

@SteveMacenski
Copy link
Member

You have a good point, approved

@SteveMacenski
Copy link
Member

@crdelsey it looks like you deleted stuff in tasks that #474 is handling, can you revert those?

Carl Delsey added 2 commits December 11, 2018 11:05
VoxelGrid is part of the nav2_msgs package. Doesn't need to be in
costmap_2d.
They are a separate change. I'll submit them in a different PR.
@ghost
Copy link
Author

ghost commented Dec 11, 2018

Ok. I've removed the nav2_tasks part of the changes. I'll add them back in a separate PR after Crystal.

@SteveMacenski
Copy link
Member

@crdelsey go ahead and merge

@mkhansenbot
Copy link
Collaborator

@crdelsey - I approved this but then realized it doesn't remove the rosidl_default_generators from the nav2_tasks package, didn't we determine that's not used anymore?

@SteveMacenski - What's the status of the release, are you going to tag a 0.1.4 or how will these changes get pulled into the build farm?

@mkhansenbot mkhansenbot added the 1 - High High Priority label Dec 11, 2018
@SteveMacenski
Copy link
Member

@mkhansen-intel I'll wait until #477 is done since that will likely cause more issues if we don't resolve it beforehand. I'd rather get the bulk of the errors / all of them out of the way before trying again

@ghost
Copy link
Author

ghost commented Dec 11, 2018

@mkhansen-intel Steve asked that I remove the nav2_tasks part of this PR, I'm assuming to minimize risk since #474 fixed them for now. I was going to add them in a separate PR after release.

@ghost
Copy link
Author

ghost commented Dec 11, 2018

@SteveMacenski I think trying to prune back all the exported dependencies (#477) is risky for the upcoming release. For the most part, having extra dependencies doesn't hurt anything. It was just a problem for the IDL generators because we were exporting a dependency without actually generating the code/CMake files for it.

@SteveMacenski
Copy link
Member

@crdelsey is there reason to believe that no other export dependencies would have a similar outcome? I haven't looked under the hood in what makes idl "special" so if the answer is that idl is "special" I'll just take your word on it

@SteveMacenski
Copy link
Member

I just dont want to keep re-releasing if we didnt fix the underlying issue, if we have, I can do that this afternoon

@ghost
Copy link
Author

ghost commented Dec 11, 2018

IDL is special because there is a message generation step that creates source code and extra stuff in the package exports.

If that doesn't run because there are no messages, then we have the exports listed, but no actual files for the downstream package to import.

There is nothing else like that in ROS2 at the moment ... to the best of my knowledge.

@ghost ghost merged commit df922c6 into ros-navigation:master Dec 11, 2018
@ghost ghost deleted the remove_obsolete_msg_gen branch December 21, 2018 18:56
mini-1235 pushed a commit to mini-1235/navigation2 that referenced this pull request Feb 5, 2025
Forsyth-Creations pushed a commit to Forsyth-Creations/navigation2 that referenced this pull request Feb 19, 2025
…tion#476)

* [JTC] Remove deprecation from parameters validation file.

* Add new dependencies.

* Add missing dependencies

* Update package.xml
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1 - High High Priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants