Skip to content

Enable leak detector on nplb in CI#10769

Open
sacuff wants to merge 1 commit into
youtube:mainfrom
sacuff:nplb
Open

Enable leak detector on nplb in CI#10769
sacuff wants to merge 1 commit into
youtube:mainfrom
sacuff:nplb

Conversation

@sacuff
Copy link
Copy Markdown
Contributor

@sacuff sacuff commented Jun 4, 2026

Bug: 477257357

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 4, 2026

🤖 Gemini Suggested Commit Message


ci: Enable leak detector for nplb

Include the nplb target in the API leak detector GitHub action. This
ensures that Starboard API symbol leaks are identified during the
continuous integration process for the nplb test suite.

Bug: 477257357

💡 Pro Tips for a Better Commit Message:

  1. Influence the Result: Want to change the output? You can write custom prompts or instructions directly in the Pull Request description. The model uses that text to generate the message.
  2. Re-run the Generator: Post a comment with: /generate-commit-message

NPLB is no longer leaking symbols, so we can now have CI check for new
leaks being introduced in it.

This also requires a change to the leak detector to not allow NPLB to
"remove" leaks that still need to be fixed in Cobalt.

Bug: 477257357
@sacuff sacuff requested a review from andrewsavage1 June 4, 2026 21:28
@sacuff sacuff marked this pull request as ready for review June 4, 2026 21:28
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request updates the API leak detector action to also run against the 'nplb' target and modifies the leak detector script to reset the 'removed' leak count when checking non-default targets. A review comment suggests resetting 'removed' using 'type(removed)()' to maintain type consistency and prevent potential type errors.

Comment on lines +728 to +729
if args.target != _DEFAULT_TARGET:
removed = 0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

To maintain type consistency and avoid potential runtime TypeErrors or static analysis issues, it is safer to reset removed to an empty instance of its original type (e.g., if removed is a list or set of symbols rather than an int). Using type(removed)() dynamically creates the correct empty/falsy value regardless of whether removed is an integer, list, or set.

Suggested change
if args.target != _DEFAULT_TARGET:
removed = 0
if args.target != _DEFAULT_TARGET:
removed = type(removed)()

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.

1 participant