Calculate type hash from TypeDescription (rep2011) #1027

emersonknapp · 2023-01-17T20:16:17Z

Related to ros-infrastructure/rep#358
Part of ros2/ros2#1159
Depends on ros2/rosidl#729

This PR introduces a utility function to provide a stable reproducible hash for type_description_interfaces/msg/TypeDescription for use in type comparison/advertising in REP-2011

clalancette

I've left a few suggestions for improvement inline, along with some questions.

Besides that, this PR should be retargeted to rolling, which is our default branch. Also, this can't be merged until we merge in the type_description_interfaces package.

rcl/include/rcl/type_version_hash.h

rcl/src/rcl/type_version_hash.c

rcl/include/rcl/type_version_hash.h

emersonknapp · 2023-02-01T21:18:51Z

Changed the base branch, Yes, I've marked the dependency in the description - though github doesn't understand that language, it has only the "closes" behavior semantic for cross-linking issues.

Addressing other comments shortly.

clalancette · 2023-02-01T21:24:03Z

Changed the base branch, Yes, I've marked the dependency in the description - though github doesn't understand that language, it has only the "closes" behavior semantic for cross-linking issues.

Thanks. Yeah, it would be nice if we could express these dependencies in GitHub somehow. The good news is that this will totally fail to compile in CI until type_description_interfaces is available, so we should be OK.

emersonknapp · 2023-02-01T22:49:55Z

OK - I think I've addressed everything. I appreciate the feedback, round 2 welcome :)

clalancette

Overall, I love this new structure; so much better than the macros. Thanks for taking the time to investigate this!

I've left a couple of minor comments inline; otherwise, I think this is ready to go modulo getting the type_description_interface package in.

rcl/src/rcl/type_version_hash.c

clalancette

This looks good to me! We should discuss how to get ros2/rcl_interfaces#146 landed.

emersonknapp · 2023-02-22T00:49:11Z

Moving this back to draft while working out the type hash struct - which this calculator should also provide

clalancette

I left a few more nits to fix here, otherwise looks pretty good to me.

rcl/include/rcl/type_version_hash.h

rcl/src/rcl/type_version_hash.c

rcl/test/rcl/test_type_version_hash.cpp

wjwwood

other than the implicit dependency on libyaml, this lgtm

wjwwood · 2023-03-23T18:07:43Z

rcl/include/rcl/type_hash.h

+ *   the JSON representation of type_description. Note that output_repr will have a
+ *   terminating null character, which should be omitted from hashing. To do so, use
+ *   (output_repr.buffer_length - 1) or strlen(output_repr.buffer) for the size of data to hash.
+ * \return RCL_RET_OK on success, RCL_RET_ERROR if any problems occur in translation


nitpick: you might want to have a look at how the other rcl functions document return types, I think they use one per \return and wrap the return types in `RCL_RET_...` for a reason, but I can't remember them off-hand. Thought I do think it's best to just emulate them.

Done (they use

\return #RCL_RET_... for reason, or \return #RCL_RET_... for a final reason, period.

so I did that

wjwwood · 2023-03-23T18:11:07Z

rcl/src/rcl/type_hash.c

+
+#include <stdio.h>
+
+#include <yaml.h>


This concerns me, I think it's coming in implicitly through rcl_yaml_param_parser. I actually don't love depending on libyaml from rcl proper like this, though the effect is the same as the current situation, since we hard depend on the yaml stuff in rcl_yaml_param_parser.

I don't think we should do something different, I think we should use libyaml, but I do think we need to explicitly depend on libyaml, via package.xml and CMake.

Signed-off-by: Emerson Knapp <[email protected]>

clalancette

Two small comment updates, then this looks good to me.

rcl/include/rcl/type_hash.h

Signed-off-by: Emerson Knapp <[email protected]>

emersonknapp · 2023-03-27T21:36:58Z

Gist: https://gist.githubusercontent.com/emersonknapp/ff276a843492f27fd1afac2806fdb849/raw/e273147f65788680698e21401ce10184f120cdfd/ros2.repos
BUILD args: --packages-above-and-dependencies rcl
TEST args: --packages-above rcl
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11684

Linux
Linux-aarch64
Windows

Signed-off-by: Emerson Knapp <[email protected]>

emersonknapp · 2023-03-28T04:16:19Z

Gist: https://gist.githubusercontent.com/emersonknapp/e163a4a54a5fa58279b131748d54a31b/raw/e273147f65788680698e21401ce10184f120cdfd/ros2.repos
BUILD args: --packages-above-and-dependencies rcl
TEST args: --packages-above rcl
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11688

Linux
Linux-aarch64
Windows

clalancette · 2023-03-28T13:57:29Z

FYI, windows build is failing with:

LINK : fatal error LNK1181: cannot open input file 'yaml.lib' [C:\ci\ws\build\rcl_lifecycle\rcl_lifecycle.vcxproj]

We might need an extra export or something? Not sure.

Signed-off-by: Emerson Knapp <[email protected]>

emersonknapp · 2023-03-28T19:19:45Z

Linux
Linux-aarch64
Windows

I noticed that the other project using this dependency, rcl_yaml_param_parser, switched over to using target_link_libraries away from ament_target_dependencies. That allowed it to hide yaml behind a PRIVATE dependency. I was hesitant to try that conversion and confuse the rest of this review, though

clalancette · 2023-03-28T19:58:10Z

I noticed that the other project using this dependency, rcl_yaml_param_parser, switched over to using target_link_libraries away from ament_target_dependencies. That allowed it to hide yaml behind a PRIVATE dependency. I was hesitant to try that conversion and confuse the rest of this review, though

I almost think it is worth it, but there is enough going on here that we can punt it. Assuming what you did already fixes the problem for now, I'll do a follow-up where we switch to target_link_libraries, since it makes it a lot nicer.

Signed-off-by: Emerson Knapp <[email protected]>

emersonknapp · 2023-03-29T04:34:06Z

Linux
Linux-aarch64
Windows

* Convert TypeDescription to YAML and hash Signed-off-by: Emerson Knapp <[email protected]> Signed-off-by: Hans-Joachim Krauch <[email protected]>

* Convert TypeDescription to YAML and hash Signed-off-by: Emerson Knapp <[email protected]>

This was referenced Jan 17, 2023

Type Description Distribution (REP-2011 subset / REP-2016) - formerly "message definitions on-the-wire" ros2/ros2#1159

Closed

Type hash in interface codegen (rep2011) ros2/rosidl#722

Merged

emersonknapp changed the title ~~[WIP] REP-2011 Type Version Hash Calculator~~ [WIP] REP-2011 Type Version Hash at runtime Jan 24, 2023

emersonknapp mentioned this pull request Jan 24, 2023

Add SHA256 utility implementation ros2/rcutils#408

Merged

emersonknapp changed the title ~~[WIP] REP-2011 Type Version Hash at runtime~~ Type Version Hash at runtime (rep-2011) Feb 1, 2023

emersonknapp marked this pull request as ready for review February 1, 2023 00:50

emersonknapp requested review from audrow, ivanpauno and wjwwood as code owners February 1, 2023 00:50

clalancette requested changes Feb 1, 2023

View reviewed changes

emersonknapp changed the base branch from master to rolling February 1, 2023 20:43

emersonknapp requested review from clalancette and removed request for wjwwood, audrow and ivanpauno February 1, 2023 22:49

clalancette requested changes Feb 1, 2023

View reviewed changes

rcl/src/rcl/type_version_hash.c Outdated Show resolved Hide resolved

rcl/src/rcl/type_version_hash.c Outdated Show resolved Hide resolved

emersonknapp requested a review from clalancette February 1, 2023 23:09

clalancette approved these changes Feb 2, 2023

View reviewed changes

emersonknapp force-pushed the emersonknapp/type-version-hash branch from ca813b1 to 024c4c5 Compare February 7, 2023 03:02

emersonknapp force-pushed the emersonknapp/type-version-hash branch from 024c4c5 to d5cae44 Compare February 15, 2023 23:13

emersonknapp changed the title ~~Type Version Hash at runtime (rep-2011)~~ Calculate type hash from TypeDescription (rep2011) Feb 21, 2023

emersonknapp marked this pull request as draft February 22, 2023 00:49

emersonknapp force-pushed the emersonknapp/type-version-hash branch 3 times, most recently from 0ffcc0f to 96a8486 Compare March 13, 2023 06:24

clalancette requested changes Mar 13, 2023

View reviewed changes

emersonknapp force-pushed the emersonknapp/type-version-hash branch from 30144b6 to ade7055 Compare March 22, 2023 08:45

emersonknapp marked this pull request as ready for review March 22, 2023 08:46

wjwwood requested changes Mar 23, 2023

View reviewed changes

emersonknapp requested a review from clalancette March 24, 2023 16:49

emersonknapp added 4 commits March 24, 2023 10:26

Convert TypeDescription to YAML and hash

c44ac92

Signed-off-by: Emerson Knapp <[email protected]>

Update to latest

9d745da

Signed-off-by: Emerson Knapp <[email protected]>

Address review nits

b1c1bbc

Signed-off-by: Emerson Knapp <[email protected]>

Explicit yaml dependency and doctext update

febd2ff

Signed-off-by: Emerson Knapp <[email protected]>

emersonknapp force-pushed the emersonknapp/type-version-hash branch from 9245a40 to febd2ff Compare March 24, 2023 17:27

clalancette requested changes Mar 27, 2023

View reviewed changes

rcl/include/rcl/type_hash.h Outdated Show resolved Hide resolved

rcl/include/rcl/type_hash.h Outdated Show resolved Hide resolved

Update docstring language

454adac

Signed-off-by: Emerson Knapp <[email protected]>

emersonknapp requested review from clalancette and wjwwood and removed request for clalancette and wjwwood March 27, 2023 21:35

clalancette approved these changes Mar 27, 2023

View reviewed changes

Fix ament exported dependency

fb40e7c

Signed-off-by: Emerson Knapp <[email protected]>

Ament export dependency

c761578

Signed-off-by: Emerson Knapp <[email protected]>

Fix msbuild warnings

00dbb57

Signed-off-by: Emerson Knapp <[email protected]>

clalancette merged commit 973951d into ros2:rolling Mar 29, 2023

clalancette mentioned this pull request Mar 29, 2023

Switch to target_link_libraries in rcl. #1051

Merged

danthony06 pushed a commit to danthony06/rcl that referenced this pull request Jun 14, 2023

Calculate type hash from TypeDescription (rep2011) (ros2#1027)

18fc931

* Convert TypeDescription to YAML and hash Signed-off-by: Emerson Knapp <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Calculate type hash from TypeDescription (rep2011) #1027

Calculate type hash from TypeDescription (rep2011) #1027

emersonknapp commented Jan 17, 2023 •

edited

Loading

clalancette left a comment

emersonknapp commented Feb 1, 2023

clalancette commented Feb 1, 2023

emersonknapp commented Feb 1, 2023

clalancette left a comment

clalancette left a comment

emersonknapp commented Feb 22, 2023

clalancette left a comment

wjwwood left a comment

wjwwood Mar 23, 2023

emersonknapp Mar 23, 2023

wjwwood Mar 23, 2023

wjwwood Mar 23, 2023

emersonknapp Mar 23, 2023

clalancette left a comment

emersonknapp commented Mar 27, 2023

emersonknapp commented Mar 28, 2023 •

edited by clalancette

Loading

clalancette commented Mar 28, 2023

emersonknapp commented Mar 28, 2023 •

edited by clalancette

Loading

clalancette commented Mar 28, 2023

emersonknapp commented Mar 29, 2023 •

edited

Loading


		#include <stdio.h>

		#include <yaml.h>

Calculate type hash from TypeDescription (rep2011) #1027

Calculate type hash from TypeDescription (rep2011) #1027

Conversation

emersonknapp commented Jan 17, 2023 • edited Loading

clalancette left a comment

Choose a reason for hiding this comment

emersonknapp commented Feb 1, 2023

clalancette commented Feb 1, 2023

emersonknapp commented Feb 1, 2023

clalancette left a comment

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

emersonknapp commented Feb 22, 2023

clalancette left a comment

Choose a reason for hiding this comment

wjwwood left a comment

Choose a reason for hiding this comment

wjwwood Mar 23, 2023

Choose a reason for hiding this comment

emersonknapp Mar 23, 2023

Choose a reason for hiding this comment

wjwwood Mar 23, 2023

Choose a reason for hiding this comment

wjwwood Mar 23, 2023

Choose a reason for hiding this comment

emersonknapp Mar 23, 2023

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

emersonknapp commented Mar 27, 2023

emersonknapp commented Mar 28, 2023 • edited by clalancette Loading

clalancette commented Mar 28, 2023

emersonknapp commented Mar 28, 2023 • edited by clalancette Loading

clalancette commented Mar 28, 2023

emersonknapp commented Mar 29, 2023 • edited Loading

emersonknapp commented Jan 17, 2023 •

edited

Loading

emersonknapp commented Mar 28, 2023 •

edited by clalancette

Loading

emersonknapp commented Mar 28, 2023 •

edited by clalancette

Loading

emersonknapp commented Mar 29, 2023 •

edited

Loading