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

Upgrade Fast CDR to 2.2.x #1530

Merged
merged 1 commit into from
Mar 26, 2024
Merged

Conversation

MiguelCompany
Copy link
Contributor

This PR updates Fast CDR to 2.2.x

The following PRs update the dependent packages to deal with the API breaks from Fast CDR v1 to v2,
and should be merged along with this one:

For simplicity, I have prepared this repos file with which we have tested the changes

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
@EduPonz
Copy link

EduPonz commented Mar 5, 2024

@clalancette friendly ping here

@MichaelOrlov
Copy link

@MiguelCompany Out of curiosity.
What type of changes are in the Fast CDR 2.2 in comparison to the v1 among API?
I am a bit worried about the performance during serialization and deserialization especially with complicated embedded datatypes.

@MichaelOrlov
Copy link

Also, another question is whether they are backward compatible.

@MiguelCompany
Copy link
Contributor Author

@MichaelOrlov

What type of changes are in the Fast CDR 2.2 in comparison to the v1 among API?

Apart from the changes explained here, methods where changed from CamelCase to snake_case

I am a bit worried about the performance during serialization and deserialization especially with complicated embedded datatypes.
Also, another question is whether they are backward compatible.

The serialization algorithm is the same if the Cdr object is constructed with CdrVersion::XCDRv1. This makes the change both backwards compatible and equally performant.

@clalancette
Copy link
Contributor

@clalancette friendly ping here

Apologies. We are frantically trying to get Rolling working on Ubuntu 24.04. Once the dust settles on that, I'll take a look at this set of patches.

@clalancette clalancette self-assigned this Mar 21, 2024
@clalancette
Copy link
Contributor

Preliminary CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor

clalancette commented Mar 21, 2024

Another round of CI after ros2/rosidl_typesupport_fastrtps#114 (comment)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor

New CI after ros2/rmw_fastrtps#746 (comment)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@MiguelCompany
Copy link
Contributor Author

@clalancette it seems my repos file is outdated and you should not use it for CI

@clalancette
Copy link
Contributor

@clalancette it seems my repos file is outdated and you should not use it for CI

Oops, yeah. Here is a new one, with my updated repos file:

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor

clalancette commented Mar 22, 2024

Actually, I just realized another mistake in CI; we aren't using Fast-DDS 2.14.x. I've updated my gist, here is another round of CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor

Here's additional CI:

  • RHEL Build Status
  • Clang Build Status
  • Windows Debug Build Status

@clalancette
Copy link
Contributor

@MiguelCompany @EduPonz It looks like RHEL-9 builds are failing because the new version of Fast-CDR currently requires CMake 3.22, whereas RHEL-9 only has CMake 3.20. Can you take a look?

@EduPonz
Copy link

EduPonz commented Mar 25, 2024

@MiguelCompany @EduPonz It looks like RHEL-9 builds are failing because the new version of Fast-CDR currently requires CMake 3.22, whereas RHEL-9 only has CMake 3.20. Can you take a look?

I've opened eProsima/Fast-CDR#204 to avoid building Fast CDR test if the minimum required CMake is not met.

@MiguelCompany
Copy link
Contributor Author

@clalancette eProsima/Fast-CDR#204 was merged. I think we can keep going on this one

@clalancette
Copy link
Contributor

All right, here is new CI for everything:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status
  • RHEL Build Status
  • Clang Build Status
  • Windows Debug Build Status

@clalancette
Copy link
Contributor

All of the failures here are unrelated. So I'm going to go ahead and merge this one and the dependencies in. Thanks for the work here!

@clalancette clalancette merged commit 7e05759 into ros2:rolling Mar 26, 2024
2 checks passed
@MiguelCompany MiguelCompany deleted the support-fast-cdr-2 branch March 26, 2024 13:27
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.

4 participants