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

Starboard: support --v for verbosity #4583

Merged
merged 9 commits into from
Dec 18, 2024

Conversation

yell0wd0g
Copy link
Contributor

@yell0wd0g yell0wd0g commented Dec 14, 2024

Starboard builds and in particular the nplb integration test support a command line flag
--min_log_level=(info|warning|error|fatal). Chromium binaries prefer in general --v
(or -v) for this purpose.

This CL adds logic to handle --v; to preserve previous behaviour, if and when both
--min_log_level and --v are present, the former takes precedence.

This CL also add unit tests for the translation between flag arguments and log level.
First instance of starboard_unittests !

Bug: 378879686
Bug: 384819454

@yell0wd0g
Copy link
Contributor Author

@niranjanyardi can you PTAL at the starboard_unittest definition plz? Thanks!

Copy link
Contributor

@madhurajayaraman madhurajayaraman left a comment

Choose a reason for hiding this comment

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

Looks good, but do we need to move anything around based on teh conversation in the Chrobalt 3P chat?

starboard/BUILD.gn Outdated Show resolved Hide resolved
Copy link
Contributor

@niranjanyardi niranjanyardi left a comment

Choose a reason for hiding this comment

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

apart from the open comment lgtm

starboard/BUILD.gn Outdated Show resolved Hide resolved
@yell0wd0g yell0wd0g force-pushed the feature/Support_v_for_verbosity branch from c282596 to 7182287 Compare December 17, 2024 02:19
Copy link
Contributor Author

@yell0wd0g yell0wd0g left a comment

Choose a reason for hiding this comment

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

PTAL again!

starboard/BUILD.gn Outdated Show resolved Hide resolved
starboard/BUILD.gn Outdated Show resolved Hide resolved
Copy link
Contributor

@andrewsavage1 andrewsavage1 left a comment

Choose a reason for hiding this comment

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

As is, common_test won't run—it hasn't been added to the gha config code. If that's on purpose because other tests in the suite fail right now, okay, but if the test does currently pass let's run it.

I also think Niranjan is correct that the common_test target should be built with the starboard toolchain instead of the Cobalt toolchain, but that's something we can follow up with

@yell0wd0g
Copy link
Contributor Author

As is, common_test won't run—it hasn't been added to the gha config code. If that's on purpose because other tests in the suite fail right now, okay, but if the test does currently pass let's run it.

I also think Niranjan is correct that the common_test target should be built with the starboard toolchain instead of the Cobalt toolchain, but that's something we can follow up with

I say let's land this CL/PR, which is about the logging --v, and work on fixing+folding all unittests into starboard_unittests in subsequent CLs, let me make a bug for it.

@yell0wd0g yell0wd0g enabled auto-merge (squash) December 18, 2024 03:28
@yell0wd0g yell0wd0g merged commit 0cd7c8f into youtube:main Dec 18, 2024
69 of 70 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants