[flutter_plugin_tool] Move branch-switching logic from tool_runner.sh to tool

[stuartmorgan-g]

Eliminates the remaining logic from tool_runner.sh, completing the goal of migrating repository tooling off of bash (both to make maintenance easier, and to better support Windows both locally and in CI). Its branch-based logic is now part of the tool itself, via a new --packages-for-branch flag (which is hidden in help since it's only useful for CI).

Part of flutter/flutter#86113

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter. (Note that unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
• I signed the CLA.
• The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
• I listed at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
• I updated CHANGELOG.md to add a description of the change.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[gaaclarke]

LGTM, just a handful of comments / nits.

[gaaclarke]

I don't think this is really an improvement because now we've introduced the risk of someone forgetting COMMON_TOOL_FLAGS. I looked at the cirrus documentation and it doesn't look like you could make a function like $RUN_PLUGIN_TOOL(podspecs).

If you made the command being run not have to be positional with the presence of a new flag you could extract RUN_PLUGIN_TOOL=dart $PLUGIN_TOOL $COMMON_TOOL_FLAGS --command

[stuartmorgan-g]

I don't think this is really an improvement because now we've introduced the risk of someone forgetting COMMON_TOOL_FLAGS.

Yes, I went back and forth on this for the same reason, and didn't see a good solution. The main benefit is that it eliminates the strange setup where tool_runner.sh is reading a magic env variable, and .cirrus.yml sets the magic variable, but it's totally non-obvious in each one in isolation what this variable is.

But I'll just revert this part for now and revisit it later.

If you made the command being run not have to be positional with the presence of a new flag you could extract RUN_PLUGIN_TOOL=dart $PLUGIN_TOOL $COMMON_TOOL_FLAGS --command

I looked into this a bit, and AFAICT allowing flags to be before the command with CommandRunner requires declaring them at different level that has totally different (and much more awkward) affordances for reading the flags. I can probably move just this one flag as a hack, but it seems ugly. Maybe I'll find a better solution when revisiting.

[gaaclarke]

nit fyi: This would be more efficient as a Stream<String> instead of Future<List<String>>.

[stuartmorgan-g]

Sure, I can change it in a follow-up. Neither the API nor the way it's being used are new in this PR, I'm just rearranging the flow a bit.

[stuartmorgan-g]

Actually, having looked at the details I'm not clear how it would be more efficient. getChangedFiles is taking the full stdout from git (the process API for the tools aren't stream-based) and splitting it by lines, giving a list. So returning a Stream instead of a List doesn't change anything there.

At this call site I need this list in two places: _changesRequireFullTest and _getChangedPackages. I can't consume the stream twice, so I would need to .toList() it here.

So the difference would be that instead of taking a List and returning it to a caller that needs a List, I would be taking a List, converting it to a Stream, and giving the stream to the caller to immediately turn back to a List. Am I missing a different way of structuring this?

[gaaclarke]

Yea, if you are going to cache the result it doesn't make a difference, it's more of an academic change.

Returning a Stream is better because it allows people to process the output while it is being parsed. This is especially helpful when you are working on input from files/pipes. This also means the operation isn't memory bound and if a consumer decides to stop consuming mid-stream resources won't be wasted processing the output that happens after it stops.

It doesn't mean a lick of difference if you cache the results though so you can process it twice.

[stuartmorgan-g]

Returning a Stream is better because it allows people to process the output while it is being parsed. This is especially helpful when you are working on input from files/pipes.

The supply-side issue here is that the git package doesn't steam git output, it uses run and returns the full output as a String.

[gaaclarke]

not introduced in this change, but is it a problem we aren't using pathSeparator here?

[stuartmorgan-g]

In practice it's not because this is only ever called with git output, which always uses POSIX paths. I changed this to p.posix.split and added a comment to the method saying that they must be POSIX paths as a quick improvement. (The better, more involved solution would be to use Files rather than Strings.)