Skip to content

check subprocess.returncode to print error message.#639

Merged
audrow merged 4 commits intoros2:masterfrom
fujitatomoya:bugfix-20210520-check-returncode
Jun 17, 2021
Merged

check subprocess.returncode to print error message.#639
audrow merged 4 commits intoros2:masterfrom
fujitatomoya:bugfix-20210520-check-returncode

Conversation

@fujitatomoya
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya commented May 20, 2021

address #601

Signed-off-by: Tomoya Fujita Tomoya.Fujita@sony.com

@fujitatomoya
check subprocess.returncode to print error message.
8dc9236 
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
fujitatomoya
fujitatomoya commented May 20, 2021
View reviewed changes
@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

fujitatomoya commented May 20, 2021

@audrow could you take a look when you got time?

audrow
audrow approved these changes May 21, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@audrow audrow left a comment

Choose a reason for hiding this comment

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

Looks like a good change to me. Is there a way to add a test for it?

@audrow audrow self-assigned this May 27, 2021
@fujitatomoya
print failure message based on os.name()
ee5b4bf 
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

fujitatomoya commented May 31, 2021

@clalancette i'd like to request a review.

@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

fujitatomoya commented May 31, 2021

CI:

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

clalancette
clalancette requested changes Jun 1, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Besides the comments inline, I'm thinking we should have some prefix to show that these messages came from ros2run, and not from the process itself. Maybe something like "[ros2run]: Message".

@fujitatomoya
check if returncode is in the valid signal range.
1c64a77 
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

fujitatomoya commented Jun 3, 2021

@clalancette all comments are addressed, i will run CI.

@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

fujitatomoya commented Jun 3, 2021

CI:

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

@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

fujitatomoya commented Jun 9, 2021

@clalancette friendly ping.

@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Jun 9, 2021

Besides the comments inline, I'm thinking we should have some prefix to show that these messages came from ros2run, and not from the process itself. Maybe something like "[ros2run]: Message".

This comment is still open. I do think we should have some kind of prefix on the output so people can tell it comes from ros2run and not from their binary.

@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

fujitatomoya commented Jun 9, 2021

@clalancette my bad, i missed that. i will address it.

@fujitatomoya
add ros2run message prefix.
612ce4e 
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

fujitatomoya commented Jun 9, 2021

unit test to confirm the message. (using simple example to generate segmentation fault)

https://github.com/fujitatomoya/ros2_test_prover/blob/master/prover_rclcpp/src/ros2cli_601.cpp

root@134f29e8f25f:~/ros2_ws/colcon_ws# ros2 run prover_rclcpp ros2cli_601
Doing something terrible!
[ros2run]: Segmentation fault

@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

fujitatomoya commented Jun 9, 2021

CI:

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

@audrow audrow requested a review from clalancette June 11, 2021 20:53
@audrow
Copy link
Copy Markdown
Member

audrow commented Jun 17, 2021

Rerunning CI (just in case):

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

@audrow audrow merged commit aaeda77 into ros2:master Jun 17, 2021
@audrow
Copy link
Copy Markdown
Member

audrow commented Jun 17, 2021

Thanks for the PR, @fujitatomoya!

@fujitatomoya fujitatomoya mentioned this pull request Sep 7, 2021
Closed
@christophfroehlich christophfroehlich mentioned this pull request Feb 2, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Barry-Xu-2018 Barry-Xu-2018 Barry-Xu-2018 left review comments

@clalancette clalancette clalancette approved these changes

@audrow audrow audrow approved these changes

Assignees

@audrow audrow

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@fujitatomoya @clalancette @audrow @Barry-Xu-2018