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

[21129] Migrate fastrtps namespace to fastdds #4898

Merged
merged 18 commits into from
Jun 18, 2024

Conversation

elianalf
Copy link
Contributor

@elianalf elianalf commented Jun 5, 2024

Description

This PR migrates all the fastrtps namespace to fastdds. It also removes all the unnecessary nested namespace and using namespace.

Related PR:
Fast DDS Python: eProsima/Fast-DDS-python#135
Shapes Demo: eProsima/ShapesDemo#141
Fast DDS GEN: eProsima/Fast-DDS-Gen#350
Discovery Server: eProsima/Discovery-Server#87

Contributor Checklist

  • Commit messages follow the project guidelines.
  • The code follows the style guidelines of this project.
  • N/A Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added; the added tests pass locally
  • N/A Any new/modified methods have been properly documented using Doxygen.
  • N/A Any new configuration API has an equivalent XML API (with the corresponding XSD extension)
  • ❌ Changes are backport compatible: they do NOT break ABI nor change library core behavior.
  • ❌ Changes are API compatible.
  • New feature has been added to the versions.md file (if applicable).
  • New feature has been documented/Current behavior is correctly described in the documentation.
    Related documentation PR: [21129] Migrate fastrtps namespace Fast-DDS-docs#813
  • N/A Applicable backports have been included in the description.

Reviewer Checklist

  • The PR has a milestone assigned.
  • The title and description correctly express the PR's purpose.
  • Check contributor checklist is correct.
  • Check CI results: changes do not issue any warning.
  • Check CI results: failing tests are unrelated with the changes.

@elianalf elianalf added this to the v3.0.0 milestone Jun 5, 2024
@elianalf elianalf force-pushed the feature/migrate_fastrtps_namespace branch from 7f89bd2 to 8333472 Compare June 10, 2024 08:35
@elianalf elianalf marked this pull request as ready for review June 10, 2024 09:53
@elianalf elianalf force-pushed the feature/migrate_fastrtps_namespace branch from 5590928 to 19234a4 Compare June 10, 2024 10:31
@elianalf elianalf force-pushed the feature/migrate_fastrtps_namespace branch from 19234a4 to 3a0ce6d Compare June 12, 2024 06:32
@elianalf elianalf requested a review from EduPonz June 12, 2024 06:36
@github-actions github-actions bot added the ci-pending PR which CI is running label Jun 12, 2024
@elianalf elianalf requested review from EduPonz and removed request for EduPonz June 12, 2024 06:45
@elianalf elianalf force-pushed the feature/migrate_fastrtps_namespace branch from 3e559b4 to 0053c97 Compare June 12, 2024 09:26
@elianalf elianalf force-pushed the feature/migrate_fastrtps_namespace branch 3 times, most recently from 86de509 to a14d4b6 Compare June 14, 2024 09:43
Copy link

@EduPonz EduPonz left a comment

Choose a reason for hiding this comment

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

  1. StatisticsCommon.hpp has a fastrtps reference
  2. StatisticsBase.hpp has a fastrtps reference
  3. utils includes have a guard with FASTRTPS
  4. We can remove the XFAIL_FASTRTPS_TESTS in xfail_tests.cmake
  5. Remove fastdds:: from all headers in include. It is not needed
  6. Remove fastdds:: from all src files. It is not needed almost anywhere
  7. Almost all using fastdds:: that are not using fastdds::rtps are not needed anymore. Same happens with using eprosima::fastdds:: outside of the examples

include/fastdds/dds/domain/DomainParticipant.hpp Outdated Show resolved Hide resolved
include/fastdds/dds/core/condition/WaitSet.hpp Outdated Show resolved Hide resolved
include/fastdds/dds/core/policy/ParameterTypes.hpp Outdated Show resolved Hide resolved
src/cpp/fastdds/publisher/PublisherImpl.cpp Outdated Show resolved Hide resolved
Copy link

@EduPonz EduPonz left a comment

Choose a reason for hiding this comment

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

I'm afraid the following are not yet addressed in full:

  1. Remove fastdds:: from all headers in include. It is not needed
  2. Remove fastdds:: from all src files. It is not needed almost anywhere
  3. Almost all using fastdds:: that are not using fastdds::rtps are not needed anymore.
  4. Same happens with using eprosima::fastdds:: outside of the examples

@elianalf elianalf force-pushed the feature/migrate_fastrtps_namespace branch from 8aa916e to a151857 Compare June 17, 2024 06:56
@elianalf elianalf force-pushed the feature/migrate_fastrtps_namespace branch from a151857 to 4b8c06d Compare June 17, 2024 07:01
@elianalf elianalf force-pushed the feature/migrate_fastrtps_namespace branch from 4b8c06d to 28ba6ca Compare June 17, 2024 07:23
@EduPonz EduPonz self-requested a review June 17, 2024 09:36
EduPonz
EduPonz previously approved these changes Jun 17, 2024
Copy link

@EduPonz EduPonz left a comment

Choose a reason for hiding this comment

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

This LGTM with green CI

Signed-off-by: elianalf <[email protected]>
@elianalf elianalf requested review from EduPonz and removed request for EduPonz June 17, 2024 12:50
@EduPonz
Copy link

EduPonz commented Jun 17, 2024

@richiprosima please test_3 discovery-server

@EduPonz EduPonz merged commit 332db84 into master Jun 18, 2024
16 of 18 checks passed
@EduPonz EduPonz deleted the feature/migrate_fastrtps_namespace branch June 18, 2024 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-pending PR which CI is running first-to-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants