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

Make ParameterService and Sync/AsyncParameterClient accept rclcpp::QoS #1978

Merged
merged 2 commits into from
Sep 9, 2022
Merged

Make ParameterService and Sync/AsyncParameterClient accept rclcpp::QoS #1978

merged 2 commits into from
Sep 9, 2022

Conversation

Barry-Xu-2018
Copy link
Collaborator

Address #1967

@Barry-Xu-2018
Copy link
Collaborator Author

@sloretz @clalancette Please help to review this PR.

fujitatomoya
fujitatomoya approved these changes Aug 2, 2022
View reviewed changes
Copy link
Collaborator

@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.

overall, lgtm

rclcpp/include/rclcpp/parameter_service.hpp Show resolved Hide resolved
@fujitatomoya
Copy link
Collaborator

CI:

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

@Barry-Xu-2018
Copy link
Collaborator Author

CI:

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

I will check failed test cases.

@fujitatomoya
Copy link
Collaborator

@Barry-Xu-2018 CI parameters are incorrect for test, i will start the new CI. my bad, sorry.

@fujitatomoya
Copy link
Collaborator

CI:

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

@Barry-Xu-2018
Copy link
Collaborator Author

@Barry-Xu-2018 CI parameters are incorrect for test, i will start the new CI. my bad, sorry.

It's okay. I will check new test result.

@Barry-Xu-2018
Copy link
Collaborator Author

Barry-Xu-2018 commented Aug 3, 2022 •
edited
Loading

Linux-aarch64 Build unstable

After checking failed test case, I find all are related to flak8. The next build (https://ci.ros2.org/job/ci_linux-aarch64/11632/) in CI is also the same problem.

Error message:

Traceback (most recent call last):
  File "/home/jenkins-agent/workspace/ci_linux-aarch64/ws/install/ament_flake8/lib/python3.10/site-packages/ament_flake8/main.py", line 182, in parse_config_file
    opts_manager = manager.OptionManager(prog='flake8', version='4.0.0')
TypeError: OptionManager.__init__() got an unexpected keyword argument 'prog'

The current used version of flake8 is 8-5.0.3 which not support prog argument.
https://github.com/PyCQA/flake8/blob/5.0.3/src/flake8/options/manager.py#L315-L324

projectroot.flake8
projectroot.flake8
projectroot.flake8
projectroot.flake8
projectroot.flake8
projectroot.flake8_generated_launch
projectroot.flake8
projectroot.flake8
projectroot.flake8
projectroot.flake8_generated_launch
projectroot.flake8
projectroot.flake8
projectroot.flake8
projectroot.flake8
projectroot.flake8
projectroot.flake8
test_cli.flake8.xunit.missing_result
launch_testing_examples.test.test_flake8.test_flake8
test_tracetools_launch.test_tracetools_launch.test.test_flake8.test_flake8
test_launch_ros.test.test_flake8.test_flake8
ros2bag.test.test_flake8.test_flake8
ros2lifecycle.ros2lifecycle.test.test_flake8.test_flake8
ros2component.ros2component.test.test_flake8.test_flake8
test_cli_remapping.flake8.xunit.missing_result
test_communication.flake8.xunit.missing_result
pendulum_control.flake8.xunit.missing_result
test_tracetools.flake8.xunit.missing_result
rosbag2_py.flake8.xunit.missing_result
composition.flake8_generated_launch.xunit.missing_result
composition.flake8.xunit.missing_result
lifecycle.flake8.xunit.missing_result
interactive_markers.flake8.xunit.missing_result
robot_state_publisher.flake8.xunit.missing_result
tf2_geometry_msgs.flake8.xunit.missing_result
demo_nodes_cpp.flake8.xunit.missing_result
logging_demo.flake8_generated_launch.xunit.missing_result
tf2_sensor_msgs.flake8.xunit.missing_result
dummy_robot_bringup.flake8.xunit.missing_result

@clalancette
Copy link
Contributor

Yes, we had flake8 errors for the last couple of days. It should now be fixed.

@Barry-Xu-2018
Copy link
Collaborator Author

@fujitatomoya Could you help run CI again ?

@fujitatomoya
Copy link
Collaborator

CI:

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

@Barry-Xu-2018
Copy link
Collaborator Author

For window test unstable, I find below error log.

14:05:15 [MSBuild] [-ERROR-] - 'C:/Program Files (x86)/Microsoft Visual Studio/2019/BuildTools/VC/Tools/MSVC/14.29.30133/include/xutility', IO exception has been thrown: java.nio.file.NoSuchFileException: C:\Program Files (x86)\Microsoft Visual Studio\2019\BuildTools\VC\Tools\MSVC\14.29.30133\include\xutility

It isn't related to change of this PR. It seems the test environment.
@fujitatomoya Could you help run windows test again ?

@fujitatomoya
Copy link
Collaborator

fujitatomoya commented Aug 16, 2022 •
edited
Loading

CI(Windows):

  • Windows Build Status

@Barry-Xu-2018
Avoid deprecated function using another deprecated function
5627160 
Signed-off-by: Barry Xu <barry.xu@sony.com>
@Barry-Xu-2018
Copy link
Collaborator Author

Maybe test unstable on window is related to deprecated function using another deprecated function.
So I submitted 5627160 and test again on Windows

@iuhilnehc-ynos
Copy link
Collaborator

  • Windows Build Status

@iuhilnehc-ynos
Copy link
Collaborator

rerun CI(mainly for Linux and Linux-aarch64) as the source code was updated at #1978 (comment)

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

@Barry-Xu-2018
Copy link
Collaborator Author

Friendly ping @sloretz

sloretz
sloretz approved these changes Sep 9, 2022
View reviewed changes
Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

This looks great!

@sloretz
Copy link
Contributor

sloretz commented Sep 9, 2022

Thank you for the PR! CI params and results LGTM.

@sloretz sloretz merged commit d0e1e83 into ros2:rolling Sep 9, 2022
@sloretz sloretz mentioned this pull request Sep 9, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sloretz sloretz sloretz approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

Assignees

@Barry-Xu-2018 Barry-Xu-2018

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Barry-Xu-2018 @fujitatomoya @clalancette @iuhilnehc-ynos @sloretz