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

[humble] Resolve recording option problem #1644

Closed
wants to merge 2 commits into from
Closed

[humble] Resolve recording option problem #1644

wants to merge 2 commits into from

Conversation

Barry-Xu-2018
Copy link
Contributor

Address #1620

@Barry-Xu-2018
Resolve recording option problem
ed8d984 
Signed-off-by: Barry Xu <barry.xu@sony.com>
@Barry-Xu-2018 Barry-Xu-2018 requested a review from a team as a code owner May 8, 2024 06:10
@Barry-Xu-2018 Barry-Xu-2018 requested review from MichaelOrlov and hidmic and removed request for a team May 8, 2024 06:10
Barry-Xu-2018
Barry-Xu-2018 commented May 8, 2024
View reviewed changes
ros2bag/ros2bag/verb/record.py
if (args.all and (args.topics or args.regex)) or (args.topics and args.regex):
return print_error('Must specify only one option out of topics, --regex or --all')
if (args.all and args.topics):
return print_error('Specify either --all or topics, but not both simultaneously.')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is an existing test case to check this

rosbag2/rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp

Lines 665 to 673 in ed8d984

TEST_F(RecordFixture, record_fails_if_both_all_and_topic_list_is_specified) {
internal::CaptureStderr();
auto exit_code =
execute_and_wait_until_completion("ros2 bag record -a /some_topic", temporary_dir_path_);
auto error_output = internal::GetCapturedStderr();
EXPECT_THAT(exit_code, Eq(EXIT_FAILURE));
EXPECT_FALSE(error_output.empty());
}

@Barry-Xu-2018
Copy link
Contributor Author

Test rosbag2 / build_and_test (pull_request) failed.
After checking, I found the below error log. It seems that the cause is from CI process.

Error: The process '/usr/bin/bash' failed with exit code 2

@Barry-Xu-2018
Copy link
Contributor Author

BTW, Iron has the same problem.
After this PR is merged, I will apply this PR to Iron branch.

@MichaelOrlov MichaelOrlov changed the title Resolve recording option problem [humble] Resolve recording option problem May 8, 2024
@fujitatomoya fujitatomoya mentioned this pull request May 8, 2024
Closed
fujitatomoya
fujitatomoya reviewed May 8, 2024
View reviewed changes
Copy link
Contributor

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

@Barry-Xu-2018 please take a look at this comment, #1620 (comment)

@Barry-Xu-2018
Copy link
Contributor Author

@fujitatomoya

@Barry-Xu-2018 please take a look at this comment, #1620 (comment)

I added comments #1620 (comment).

fujitatomoya
fujitatomoya approved these changes May 9, 2024
View reviewed changes
Copy link
Contributor

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

@Barry-Xu-2018 thanks for the explanation.

fix looks good, btw is this supposed to target to iron 1st. then we can backport it to humble. (i think we can do vice-versa, so just a question)

CC: @MichaelOrlov

@MichaelOrlov MichaelOrlov linked an issue May 10, 2024 that may be closed by this pull request
--help mentions that --regex is applied on top of topic list, but doing so gives "Must specify only one option out of topics, --regex or --all" #1620
Closed
@Barry-Xu-2018
Copy link
Contributor Author

@fujitatomoya

fix looks good, btw is this supposed to target to iron 1st. then we can backport it to humble. (i think we can do vice-versa, so just a question)

Yes. I will create a new PR for Iron and then backport it to Humble.

@Barry-Xu-2018
Print warning instead of parameter description while using '--all' an…
c84824f 
…d '--regex' at the same time

Signed-off-by: Barry Xu <barry.xu@sony.com>
@Barry-Xu-2018
Copy link
Contributor Author

#1649 for Iron has been created. After it is merged, it will backport to Humble.
So I will close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaelOrlov MichaelOrlov MichaelOrlov left review comments

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@hidmic hidmic Awaiting requested review from hidmic hidmic is a code owner automatically assigned from ros2/tooling-reviewers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@Barry-Xu-2018 @MichaelOrlov @fujitatomoya