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

[humble] Backport. Added support for filtering topics via regular expressions (#1034)- #1039

Merged
merged 2 commits into from
Jul 28, 2022

Conversation

esteve
Copy link
Member

@esteve esteve commented Jul 6, 2022

This is a backport of #1034 to Humble

* Added support for filtering topics via regular expressions

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>

* Added application function for evaluating regular expressions from within sqlite3

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>

* Use nullptr for null pointers

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>

* Added tests for topics-regex API

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
@esteve esteve requested a review from a team as a code owner July 6, 2022 15:55
@esteve esteve requested review from MichaelOrlov and jhdcs and removed request for a team July 6, 2022 15:55
@esteve esteve changed the title Added support for filtering topics via regular expressions (#1034) [humble] Backport. Added support for filtering topics via regular expressions (#1034)- Jul 6, 2022
@esteve
Copy link
Member Author

esteve commented Jul 12, 2022

@jhdcs @MichaelOrlov I'm about to implement an option to exclude topics from being played back, and then I realized that it'd be best that this PR's option (--topics-regex) and the one I'm about to implement matched the options in ros2 bag record, it'd be more ergonomic. What do you think? If you agree, I'd update this PR and also submit another PR against rolling.

Edit: by updated I mean renaming --topics-regex to match ros2 bag record, the --exclude option would go into another PR

@MichaelOrlov
Copy link
Contributor

@esteve I don't mind if you will rename in this PR and in Rolling --topics-regex to just simple -regex to be compliant with parameters format from rosbag2 recorder.
We should have caught it during review. Sorry we missed it.

@esteve esteve mentioned this pull request Jul 19, 2022
Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

@esteve I didn't expect that you will rename storage_filter.topics_regex to the simple storage_filter.regex.
As for me it would be more clear if it will be full name storage_filter.topics_regex.
Anyway I approve this MR and it's up to you if you prefer to keep it as.

@MichaelOrlov
Copy link
Contributor

MichaelOrlov commented Jul 26, 2022

Running CI:
Gist: https://gist.githubusercontent.com/MichaelOrlov/a373802dcda49593a16efb760e2eb61a/raw/303af71374c03d69edfd179d61fece958a357364/ros2.repos
BUILD args: --packages-above-and-dependencies ros2bag rosbag2_py rosbag2_storage rosbag2_storage_default_plugins rosbag2_transport rosbag2_tests
TEST args: --packages-above ros2bag rosbag2_py rosbag2_storage rosbag2_storage_default_plugins rosbag2_transport rosbag2_tests
ROS Distro: humble
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10574

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

@MichaelOrlov
Copy link
Contributor

It seems Windows build failed due to some infrastructure issue.
Trying to re-run it.

  • Windows Build Status

@MichaelOrlov MichaelOrlov added this to TODO in Humble Patch Release 1 via automation Jul 26, 2022
@MichaelOrlov
Copy link
Contributor

MichaelOrlov commented Jul 27, 2022

@clalancette @audrow It seems something wrong with build infrastructure for Windows build on CI.
I've ran CI build for Windows a couple times and it failed with some errors seems unrelated to the changes and I can't figure out what exactly wrong.
Is it known issue with build infrastructure or I've missed something?

Could you please help with it?

@clalancette
Copy link
Contributor

clalancette commented Jul 27, 2022

Is it known issue with build infrastructure or I've missed something?

The first job you ran failed because of an infrastructure issue on Windows that sometimes fails for reasons we don't understand (though it happens infrequently enough that we just live with it for now).

The second job you ran for reasons I don't understand either. Other than the above sometimes flaky problem, we don't have any other known Windows infrastructure issues, and Windows CI passed overnight: https://ci.ros2.org/view/nightly/job/nightly_win_rel/2376/

Here's a Humble Windows job without these changes to see what happens: * Windows Build Status

@MichaelOrlov
Copy link
Contributor

MichaelOrlov commented Jul 27, 2022

@clalancette Thanks for helping figuring out what is going on.

Trying to run Windows build one more time:

  • Windows Build Status

@MichaelOrlov MichaelOrlov merged commit 193caee into ros2:humble Jul 28, 2022
@esteve
Copy link
Member Author

esteve commented Jul 31, 2022

@esteve I didn't expect that you will rename storage_filter.topics_regex to the simple storage_filter.regex.
As for me it would be more clear if it will be full name storage_filter.topics_regex.
Anyway I approve this MR and it's up to you if you prefer to keep it as.

I agree. I've been offline the past few days, so I haven't had time to address your feedback, I can do it on Monday.

@esteve
Copy link
Member Author

esteve commented Aug 1, 2022

@MichaelOrlov I've addressed your feedback in #1053 , I'll submit a backport PR for Humble shortly.

@esteve esteve deleted the filter-regex-humble branch August 3, 2022 10:51
esteve added a commit that referenced this pull request Aug 16, 2022
esteve added a commit that referenced this pull request Aug 16, 2022
…ular expressions (#1034)- (#1039)"

This reverts commit 193caee.

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
MichaelOrlov pushed a commit that referenced this pull request Aug 17, 2022
…ular expressions (#1034)- (#1039)" (#1069)

This reverts commit 193caee.

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
@audrow audrow moved this from TODO to Done in Humble Patch Release 1 Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants