[ Tool ] Support Powershell v6+ to determine Windows version in flutter doctor
#156476
Conversation
| const List<String> psArgs = <String>['-command', argument]; | ||
| try { | ||
| return await processManager.run(<String>['powershell', ...psArgs]); | ||
| } on ProcessPackageExecutableNotFoundException { | ||
| return processManager.run(<String>['pwsh', ...psArgs]); | ||
| } |
There was a problem hiding this comment.
This LGTM as-is, but I think there is a world where we can detect that powershell is unavailable (via ProcessManager.canRun) and then make the validator fail with a ValidationResult.missing that has a more specific message rather than throwing an exception on relying on higher-level error handling. (This is just an FYI; my hope is that this knowledge is useful for future work on validators).
However, this kind of situation is unlikely to happen again very often, and the error message generated in that case is still sufficient to figure out what's going wrong here1—hence the LGTM.
Footnotes
-
This is evidenced by the reporting user being able to figure out what was going wrong here themselves. ↩
There was a problem hiding this comment.
That's a great point! I definitely just skipped past all the validation logic at the top of the file... :)
I've gone ahead and added a validation step and made more use of ProcessManager.canRun rather than attempting to run executables to determine whether or not they exist.
packages/flutter_tools/test/general.shard/windows_version_validator_test.dart
Show resolved
Hide resolved
| fail('Should have thrown, but successfully ran ${result.stdout}'); | ||
| } on ProcessPackageExecutableNotFoundException { | ||
| // Expected | ||
| // ignore: avoid_catches_without_on_clauses |
There was a problem hiding this comment.
Is this line necessary? flutter analyze passes without it.
There was a problem hiding this comment.
Then again, the Linux analyze check is failing; so me running flutter analyze locally is apparently not sufficient 🤪
There was a problem hiding this comment.
Yeah, we seem to have avoid_catches_without_on_clauses: true as an override in the tool's analysis_options.yaml, so the analysis server was complaining about it locally. Very annoying that it apparently doesn't pick up the same set of lints as the bots though...
packages/flutter_tools/lib/src/windows/windows_version_validator.dart
Outdated
Show resolved
Hide resolved
…or.dart Co-authored-by: Andrew Kolos <andrewrkolos@gmail.com>
|
LGTM |
|
auto label is removed for flutter/flutter/156476, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
…in `flutter doctor` (flutter/flutter#156476)
…in `flutter doctor` (flutter/flutter#156476)
…in `flutter doctor` (flutter/flutter#156476)
…in `flutter doctor` (flutter/flutter#156476)
…in `flutter doctor` (flutter/flutter#156476)
…in `flutter doctor` (flutter/flutter#156476)
…in `flutter doctor` (flutter/flutter#156476)
Powershell v6+ use the executable name
pwsh.exeinstead ofpowershell.exe. This change adds support for determining the Windows version on systems that solely usepwsh.exeand don't havepowershell.exeon thePATH.Fixes #156189.