Skip to content

Copy type_description_interfaces structs to runtime_c/cpp#4

Closed
emersonknapp wants to merge 1 commit intoemersonknapp/hash-typesupportfrom
emersonknapp/type-description-runtime
Closed

Copy type_description_interfaces structs to runtime_c/cpp#4
emersonknapp wants to merge 1 commit intoemersonknapp/hash-typesupportfrom
emersonknapp/type-description-runtime

Conversation

@emersonknapp
Copy link
Owner

@emersonknapp emersonknapp commented Mar 23, 2023

Depends on ros2#729
Staging this PR for visibility against PR-in-review. It will have to be reopened against ros2 when ros2#729 is merged.

Provides usable runtime types for use by dynamic types and codegen for type descriptions.

Provides a script that does the copying and modification in a single invocation - all added headers and sources in this change have not been touched by hand and never should be.

For the reviewer - most importantly see scripts/copy_type_description_generated_sources.bash

@emersonknapp emersonknapp changed the title Copy type_description_interfaces structs Copy type_description_interfaces structs to runtime_c/cpp Mar 23, 2023
@emersonknapp emersonknapp force-pushed the emersonknapp/type-description-runtime branch from 48f6b2f to 2b4d6cc Compare March 23, 2023 05:11
Copy link

@methylDragon methylDragon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a good idea to commit the generated files since version control can track if any changes to the output are made (either from changes from the rosidl code generation, or something else) (though I'm not sure who would be continually rebuilding and checking)

(Well that and users wouldn't be running the script anyway...)

LGTM

@methylDragon
Copy link

That said, I'm not sure if it would be better if the script was invoked in the CMakeLists instead. That would ensure that any future changes to rosidl would update the generated code (though there's the slight danger that errors in env var setting could lead to some pretty nasty rm calls.)

I don't have a preference for this though.

@emersonknapp
Copy link
Owner Author

emersonknapp commented Mar 23, 2023

That said, I'm not sure if it would be better if the script was invoked in the CMakeLists instead

This approach isn't possible - rosidl_runtime_c[pp] are dependencies of type_description_interfaces - there's no way to know at their build time that there even is a build directory anywhere for type_description_interfaces - they may be in a standalone workspace all by themselves. It has to be a checked-in copy updated by something (for now a human) outside the CMake/colcon build process.

@methylDragon
Copy link

Ah.. okay

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp emersonknapp force-pushed the emersonknapp/type-description-runtime branch from 2b4d6cc to e8c0236 Compare March 23, 2023 17:56
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.

2 participants