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

Force extension points to be registered in order #485

Merged
merged 3 commits into from May 14, 2020

Conversation

ivanpauno
Copy link
Member

@ivanpauno ivanpauno commented May 11, 2020

This is needed after ament/ament_cmake#256.

Reverse inclusion of generated export targets files was a lucky choice in this case, as the extension points are executed in alphabetical order, and reverse alphabetical order was coincidental with the dependency order (a extreme coincidence 😄).

This PR ensures that extension points are registered in the correct order (and thus, executed in the correct order too).

@ivanpauno ivanpauno force-pushed the ivanpauno/fix-target-export-config-include-order branch from a2e42d3 to 45d2326 Compare May 12, 2020 18:25
@dirk-thomas
Copy link
Member

While this is the right way to fix this I am certain that a runtime dependency on a generator package is undesired. @chapulina FYI - I couldn't find the meta ticket for that goal?

@ivanpauno
Copy link
Member Author

While this is the right way to fix this I am certain that a runtime dependency on a generator package is undesired. @chapulina FYI - I couldn't find the meta ticket for that goal?

This introduces a "build tool export" dependency, which will require rosidl_generator_c/cpp to be available at build time of downstream packages (but not at runtime). That reflects the reality, as it's actually required (before the PR it was required too, though it wasn't explicitly expressed here).

I can modify the PR, but I don't think that deleting the REQUIRED is correct.

@dirk-thomas
Copy link
Member

This introduces a "build tool export" dependency, which will require rosidl_generator_c/cpp to be available at build time of downstream packages (but not at runtime).

This is not how these dependency types work. This will result in a run dependency of the Debian package: https://github.com/ros-infrastructure/bloom/blob/9e81373b0e9be8d0e154dcc57ec8530b4ddc8ea5/bloom/generators/debian/generator.py#L317-L319

@chapulina
Copy link
Contributor

FYI - I couldn't find the meta ticket for that goal?

I don't think there's a ticket for this. @ahcorde and @wjwwood worked out the plan to reduce runtime dependencies for rclcpp.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
…pp in rosidl_typesupport_introspection_c/cpp

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno changed the title Force extension points to be executed in order Force extension points to be registered in order May 13, 2020
@ivanpauno ivanpauno force-pushed the ivanpauno/fix-target-export-config-include-order branch from 45d2326 to ee57927 Compare May 13, 2020 13:46
@ivanpauno
Copy link
Member Author

I don't think there's a ticket for this. @ahcorde and @wjwwood worked out the plan to reduce runtime dependencies for rclcpp.

This doesn't introduce a runtime dependency on rosidl_generator_c/cpp in rclcpp, neither transitively.
What it's true is that it introduces a runtime dependency (transitively) on rosidl_generator_c/cpp in rmw_cyclonedds_cpp/rmw_fastrtps_dynamic_cpp, so systems interested in use those rmw implementations and not interested in generating messages will get an unnecessary dependency.

Applied suggestion here in ee57927.

ros2/rosidl_typesupport#73 was introducing a runtime dependency on rosidl_generator_* in rclcpp, that was fixed too in ros2/rosidl_typesupport@5d2790a.


I think it would be better in the future to split these packages (rosidl_typesupport_introspection_c/cpp, rosidl_typesupport_c/cpp) in two pieces: one with what rmw_*/rclcpp need, and one with the generation part.

@dirk-thomas
Copy link
Member

Same comments as ros2/rosidl_typesupport#73 (comment) and ros2/rosidl_typesupport#73 (comment).

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

Same comments as ros2/rosidl_typesupport#73 (comment) and ros2/rosidl_typesupport#73 (comment).

Addressed in 7c1af45.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants