Support Fast CDR v2 #746
Support Fast CDR v2 #746
Conversation
clalancette
left a comment
clalancette
left a comment
There was a problem hiding this comment.
Other than my one comment, this looks good to me.
rmw_fastrtps_cpp/package.xml
Outdated
| <build_depend version_gte="2">fastcdr</build_depend> | ||
| <build_depend version_gte="2.13">fastrtps</build_depend> |
There was a problem hiding this comment.
Let's not add these constraints in here. I know it is technically more correct, but we don't do this anywhere else.
(same goes for all of the uses below)
There was a problem hiding this comment.
This is something we mentioned within the Production WG while discussing this proposal
Not doing it anywhere else doesn't mean it should not be done.
Although it seems that rosdep will not enforce version checks, bloom will put the version requirements in the rules to build debian packages.
That said, I will add a commit removing version restrictions from all package.xml files, and add them in a follow-up PR so we don't block merging this one
There was a problem hiding this comment.
Although it seems that rosdep will not enforce version checks, bloom will put the version requirements in the rules to build debian packages.
It's true that bloom will put those version checks in, but there isn't really a scenario where it would ever be needed, especially in the core and in Rolling. Releasing this package will cause all of the downstream packages to rebuild (which is most of the ROS ecosystem). If any of that fails to build, then we won't have a viable distribution to "sync". If it all succeeds in building, then we will sync the whole thing out together. In that sense, it is "atomic" with the rest of the packages (at least up to desktop). You can definitely construct scenarios where someone cherry-picks certain packages and tries to install them together, but anyone doing that on Rolling gets to keep all of the pieces, in my opinion.
Anyway, I agree with that this is a wider discussion we should have, so removing it from this PR is the correct thing for now.
Signed-off-by: Miguel Company <[email protected]>
Signed-off-by: Miguel Company <[email protected]>
Signed-off-by: Miguel Company <[email protected]>
Signed-off-by: Miguel Company <[email protected]>
Signed-off-by: Miguel Company <[email protected]>
Signed-off-by: Miguel Company <[email protected]>
MiguelCompany
force-pushed
the
support-fast-cdr-2
branch
from
178d2d3 to
3d2f5f5
Compare
Signed-off-by: Miguel Company <[email protected]>
clalancette
left a comment
clalancette
left a comment
There was a problem hiding this comment.
One linter change needed here.
| reinterpret_cast<char *>(payload->data), payload->max_size); | ||
| eprosima::fastcdr::Cdr ser( // Object that serializes the data | ||
| fastbuffer, eprosima::fastcdr::Cdr::DEFAULT_ENDIAN, eprosima::fastcdr::Cdr::DDS_CDR); | ||
| fastbuffer, eprosima::fastcdr::Cdr::DEFAULT_ENDIAN, eprosima::fastcdr::CdrVersion::XCDRv1); |
There was a problem hiding this comment.
CI is complaining that this line is too long (needs to be <= 100 characters): https://ci.ros2.org/job/ci_linux-aarch64/14925/testReport/junit/(root)/projectroot/cpplint/
Signed-off-by: Miguel Company <[email protected]>
2 participants
This makes the necessary changes to allow building with Fast CDR v2
Part of ros2/ros2#1530