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.

Sign up for GitHub

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

Jump to bottom

A fallback to support content filters in rcl #1

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

A fallback to support content filters in rcl #1

wants to merge 7 commits into from

Conversation

iuhilnehc-ynos
Copy link
Collaborator

@iuhilnehc-ynos iuhilnehc-ynos commented Aug 1, 2022 •
edited
Loading

To address partial requirement in ros2/rcl#982.
Some extra enhancements will be the follow-up PRs.

Signed-off-by: Chen Lihui [email protected]

This was referenced Aug 1, 2022
Open
Open
@iuhilnehc-ynos
Copy link
Collaborator Author

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

@iuhilnehc-ynos
Copy link
Collaborator Author

rerun CI on the aarch64

  • Linux-aarch64 Build Status

@iuhilnehc-ynos
Copy link
Collaborator Author

Unrelated failures about flake8 on https://ci.ros2.org/job/ci_linux-aarch64/11605/ .

  • CI uses flake8 with version 5.0.1
// https://ci.ros2.org/job/ci_linux-aarch64/11605/consoleFull

14:19:00 Collecting flake8
14:19:00   Downloading flake8-5.0.1-py2.py3-none-any.whl (61 kB)
14:19:00      ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 61.7/61.7 KB 12.0 MB/s eta 0:00:00

The prog was removed. Please refer to https://github.com/PyCQA/flake8/blob/5.0.1/src/flake8/options/manager.py#L315-L324

@iuhilnehc-ynos
Copy link
Collaborator Author

iuhilnehc-ynos commented Aug 1, 2022 •
edited
Loading

Give it another shot.

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

@iuhilnehc-ynos iuhilnehc-ynos force-pushed the topic-fallback_to_content_filter branch from 29f4661 to 528c2dc Compare August 4, 2022 01:44
@iuhilnehc-ynos
Copy link
Collaborator Author

retry CI:

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

@iuhilnehc-ynos
Copy link
Collaborator Author

a flaky test on windows is a known issue reported in ros2/examples#340.

Test Result (1 failure / -1)
launch_testing_examples.launch_testing_examples.check_multiple_nodes_launch_test.launch_testing_examples.check_multiple_nodes_launch_test

@fujitatomoya fujitatomoya mentioned this pull request Aug 23, 2022
Open
4 tasks
@fujitatomoya
Copy link

@iuhilnehc-ynos i will be working on this as well.

fujitatomoya
fujitatomoya reviewed Mar 23, 2023
View reviewed changes
Copy link

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

1st review for tao_pegtl_vendor and test_content_filter_msgs, just minor comments.

i will try to review core until next week.

test_content_filter_msgs/package.xml Outdated
<name>test_content_filter_msgs</name>
<version>0.0.1</version>
<description>Complex messages for test rcl content filter fallback api.</description>
<maintainer email="[email protected]">Chen Lihui</maintainer>
Copy link

Choose a reason for hiding this comment

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

So are other packages, could you add [email protected] just in case? it would be probably better to put multiple members here for it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you. Updated in 850727f.

I added @fujitatomoya and @Barry-Xu-2018 to the maintainer list, but I don't know if I should add somebody at openrobotics.org. I'll update it if there is another request.

tao_pegtl_vendor/tao_pegtl_vendor-extras.cmake Outdated
@@ -0,0 +1,18 @@
# Copyright 2022 Sony Corporation
Copy link

Choose a reason for hiding this comment

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

skipping the year, but precise company name should be applied. (and else where)

Suggested change
# Copyright 2022 Sony Corporation
# Copyright 2022 Sony Group Corporation

tao_pegtl_vendor/package.xml Outdated
<version>0.0.1</version>
<description>
Wrapper around pegtl, providing nothing but a dependency on pegtl, on some systems.
On others, it provides an ExternalProject build of spdlog.
Copy link

Choose a reason for hiding this comment

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

Why this package is related to spdlog?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, I forgot to update it. Updated in 850727f

rcl_content_filter_fallback/CMakeLists.txt Outdated
Comment on lines 2 to 3
project(rcl_content_filter_fallback)

Copy link

Choose a reason for hiding this comment

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

?

Suggested change
project(rcl_content_filter_fallback)
project(rcl_content_filter_fallback)

rcl_content_filter_fallback/include/rcl_content_filter_fallback/rcl_content_filter_fallback.h Outdated
{
#endif

/// Create a rcl content filter fallback instance to filter ros2 message data.
Copy link

Choose a reason for hiding this comment

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

Suggested change
/// Create a rcl content filter fallback instance to filter ros2 message data.
/// Create a rcl content filter fallback instance to filter ros 2 message data.

@fujitatomoya
add multiple maintainer and use correct descriptions
850727f 
Co-authored-by: Tomoya.Fujita <[email protected]>
Signed-off-by: Chen Lihui <[email protected]>
@iuhilnehc-ynos
Copy link
Collaborator Author

repos

CI for #1, ros2/rcl#996, ros2/demos#579:

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

@iuhilnehc-ynos iuhilnehc-ynos mentioned this pull request Sep 5, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fujitatomoya fujitatomoya fujitatomoya left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@iuhilnehc-ynos @fujitatomoya