Repository navigation
Refactor devicelab bin/run.dart and other cleanup - #96320
Conversation
flutter-dashboard
Bot
added
the
c: contributor-productivity
e5988ef to
462b347
Compare
There was a problem hiding this comment.
nit: I prefer seeing these defined somewhere outside of main, or even in its own file - although I'm not sure there's any really great solution here, since that ends up meaning a reader might not have a clear idea of what arguments are defined and what their types/expected values are.
Reason for preferring outside of main is that this is 100+ lines of configuration/documentation that I have to scroll through to get to the meat of what main is doing.
There was a problem hiding this comment.
totally reasonable; I'll move it back into its own function but with more of a functional style rather than the global variables style from before.
There was a problem hiding this comment.
This doesn't work on Windows.
There was a problem hiding this comment.
(And Windows historically has been our biggest culprit for this kind of problem, especially since leaked processes can lock files something else wants/needs to delete).
Last time I brought this up with anyone from the VM team, it seems like a fundamental limitation on the API offered by windows. Hence the delegation to taskkill.exe which generally does a better job (but is not really suitable for use by the Dart VM as API surface IMO).
There was a problem hiding this comment.
roger. Is there an issue tracking this?
There was a problem hiding this comment.
neither of those seem that problematic here?
There was a problem hiding this comment.
@dnfield I looked at those - they seem to be about wrong return value or lack of waiting, they don't mention that killPid doesn't actually terminate given process.
There was a problem hiding this comment.
Ahhh that part is a bit harder - but my recollection is that on Windows I would sometimes see a process that would only terminate if the /f flag was passed to taskkill.exe, and Dart had no equivalent way of doing the force part.
There was a problem hiding this comment.
Dart also has no way of waiting for the process to terminate, so we don't know if we're continuing execution after the process has finished or not.
There was a problem hiding this comment.
Yeah, we could make killPid to wait for the process to exit. Some processes might take longer to exit, which will block this call too. Alternatively if caller insists on waiting it supposedly could call killPid until it returns false.
Would be nice if there was a repro for when killPid didn't actually work and you had to resort to shelling taskkill.exe
dnfield
left a comment
dnfield
left a comment
There was a problem hiding this comment.
Process.killPid isn't appropriate for use on Windows.
* Refactor of bin/run.dart to remove the global variables. This caught some unused arguments, which are now removed, and made some ambiguous-looking code with global variables that were shadowing each other simpler to reason about. It also allowed a bunch of `!`s to be removed. * Introduced `--terminate-stray-dart-processes` argument so that tests can set `--no-terminate-stray-dart-processes` so that they can avoid terminating each other when tests are run with `-j` > 1. * Cleanup up some console output. * Removed the `runProcessCleanup` magic from tests. * Changed the `killProcess` implementation to use `Process.killPid` rather than deferring to another process. * Renamed `localEngine` to `localEngineFromEnv` to make it clearer what it did. (Ditto another nearby getter.)
462b347 to
e694de6
Compare
|
Updated PR with suggested fixes (and more refactoring that it pushed me into). |
e694de6 to
bbfc831
Compare
Refactor of bin/run.dart to remove the global variables. This caught some unused arguments, which are now removed, and made some ambiguous-looking code with global variables that were shadowing each other simpler to reason about. It also allowed a bunch of
!s to be removed.Introduced
--terminate-stray-dart-processesargument so that tests can set--no-terminate-stray-dart-processesso that they can avoid terminating each other when tests are run with-j> 1.Cleanup up some console output.
Removed the
runProcessCleanupmagic from tests.Changed the
killProcessimplementation to useProcess.killPidrather than deferring to another process.Renamed
localEnginetolocalEngineFromEnvto make it clearer what it did. (Ditto another nearby getter.)