Type Description Codegen and Typesupport (rep2011) #727
Conversation
|
Gonna drop this in here before I forget. Unfortunately there isn't a way for files to get excluded from the linters yet, so we might have to just live with this regression for now (or turn off the linting tests)... |
|
@methylDragon There are only two problems that the generated code is allowed to get away with already: line length and missing copyright declaration. I can fix the copyright easily by running "living with the regression" isn't an option, nor is turning off the linters for the package - I just hadn't got as far as making all tests happy in this PR yet. |
|
Whoops update - actually it looks like the capability you talked about was implemented in ament/ament_lint#386 and you even personally signed off on it :) I've added a commit excluding the copied files from the linters and it works. I've got a lingering question of whether we should run linters here. They are linted in the place they're originally created (minus copyright and with no line length limit) so as long as the script's modifications are considered fine then I'm leaning toward just excluding them. Or, we could run a few extra linter checks on just the copied files - some other packages do this for generated files, like https://github.com/ros2/rcutils/blob/rolling/CMakeLists.txt#L172 |
WHOOPS 🤦 , nice catch. I should go update the issue... Edit: Thanks for updating the issue |
Yeah, I'm in favor of linting them, even though it is a bit redundant. But I don't have a strong opinion. |
emersonknapp
force-pushed
the
emersonknapp/type-description-struct
branch
14 times, most recently
from
f4763c5
to
dc3d464
Compare
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
| const rosidl_runtime_c__type_description__TypeSource__Sequence * | ||
| @(td_c_typename)__@(GET_SOURCES_FUNC)() | ||
| { | ||
| @# TODO(emersonknapp) Implement raw source code embedding/generation. This sequence is left empty for now. |
There was a problem hiding this comment.
Is this handled by the later prs?
| @@ -0,0 +1,114 @@ | |||
| // Copyright 2015 Open Source Robotics Foundation, Inc. | |||
There was a problem hiding this comment.
Not sure if this should be updated?
There was a problem hiding this comment.
I think it should, I'll do it in emersonknapp#10
| add_executable( | ||
| test_descriptions_c | ||
| test/rosidl_generator_c/test_descriptions.c | ||
| ) | ||
| ament_add_test( | ||
| test_descriptions_c | ||
| COMMAND "$<TARGET_FILE:test_descriptions_c>" | ||
| GENERATE_RESULT_FOR_RETURN_CODE_ZERO | ||
| ) | ||
| target_link_libraries(test_descriptions_c | ||
| ${c_generator_target} | ||
| rosidl_runtime_c::rosidl_runtime_c | ||
| ) |
There was a problem hiding this comment.
Were you trying to avoid C++/gtest? I'm not sure that's a requirement. I mean, it's done now, no need to change it, but I was just curious.
There was a problem hiding this comment.
All the other tests here are plain C so I was just following pattern rather than go off-script with C++/gtest dependency
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
…n-include-nested Extend TypeDescription codegen with referenced type inclusion, disable option, raw sources
|
I did two additional pieces of analysis here:
As a general comment, I would say that the "new" set of PRs is somewhat better on binary size than the original set for the largest libraries, but worse for smaller libraries. It is clear that disabling codegen helps significantly, so I'm glad we added that option. So with that said, I'm going to approve this PR and work on reviewing the rest of the set. I'd like to get @wjwwood 's final approval as well, and of course we'll need new CI. |
Note that the "new" PRs also now contain raw sources, where the "old" ones did not have that data so that muddies the comparison a bit unfortunately. |
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
|
Gist: https://gist.githubusercontent.com/emersonknapp/3d42343216f83e16ec6b739829a33a15/raw/710780b6ce1a1318b21363581bad84a247f92bb4/ros2.repos
|
|
Just because of the size of this series, I'm going to run RHEL and Windows Debug as well:
|
|
And we're green! I'm going to go ahead and merge the lot, and cross my fingers for overnight CI. |
Part of ros2/ros2#1159
Bundled with (must be merged together):
Key features:
TYPE_HASH_TUPLESto more accurateTYPE_DESCRIPTION_TUPLESin cmakeget_type_description,get_individual_type_description_source, andget_type_description_sourcesidl__description.c.em, generating code for type descriptions and raw interface definition sources, embedded in the binariesROSIDL_GENERATOR_C_DISABLE_TYPE_DESCRIPTION_CODEGENrosidl_generator_type_descriptionto output a new structure oftype_description_msgplus a flat list of all referenced types and their hashes at the time of generation. These hashes are used at runtime (in Debug builds) to detect ABI breaks when constructing type descriptions the first time