Refactor vswhere.exe integration by loic-sharma · Pull Request #104133 · flutter/flutter

loic-sharma · 2022-05-19T00:38:59Z

Hello! This is my first PR to Flutter, so please let me know if I goofed anything! 😅

Previously, VisualStudio stored the JSON output from vswhere.exe as Maps. As a result, logic to convert values from vswhere.exe's output was repeated (first to validate values, second to expose values as getters on VisualStudio). This change introduces a new VswhereDetails type.

In the future, this VswhereDetails type will be used to make Flutter doctor resilient to bad UTF-8 output from vswhere.exe.

Part of #102451.

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 Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this 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.

Work

loic-sharma · 2022-05-19T00:41:34Z

packages/flutter_tools/lib/src/windows/visual_studio.dart

+  /// such installation.
  ///
-  /// If no installation is found, the cached VS details are set to an empty map
-  /// to avoid repeating vswhere queries that have already not found an installation.


This comment was incorrect: the "cached VS details" was only set if an installation was found but had issues. This PR keeps the existing behavior.

loic-sharma · 2022-05-19T00:44:05Z

packages/flutter_tools/lib/src/windows/visual_studio.dart

  ///
-  /// If no installation is found, the cached VS details are set to an empty map
-  /// to avoid repeating vswhere queries that have already not found an
-  /// installation.


This comment was also incorrect. This PR follows the existing behavior.

cbracken

Nice! Moving to a class is a nice improvement.

Overall looks good; most of my comments are actually about ways we could clarify existing issues with old code rather than specific to the new code.

packages/flutter_tools/lib/src/windows/visual_studio.dart

cbracken

cbracken

re-lgtm! Nice!

`VisualStudio` calls `vswhere.exe` to find Visual Studio installations and determine if they satisfy Flutter's requirements. Previously, `VisualStudio` stored the JSON output from `vswhere.exe` as `Map`s, resulting in duplicated logic to read the JSON output (once to validate values, second to expose values). Also, `VisualStudio` stored two copies of the JSON output (the latest valid installation as well as the latest VS installation). This change simplifies `VisualStudio` by introducing a new `VswhereDetails`. This type contains the logic to read `vswhere.exe`'s JSON output, and, understand whether an installation is usable by Flutter. In the future, this `VswhereDetails` type will be used to make Flutter doctor resilient to bad UTF-8 output from `vswhere.exe`. Part of flutter#102451.

…ere.exe (#105153) * [tool] Consistent FakeProcessManager.run/runSync (#103947) `FakeProcessManager` is a test-oriented implementation of `ProcessManager` that simulates launching processes and returning `ProcessResult` objects whose `exitCode`, `stdout`, `stderr` can be used to write platform-portable, hermetic tests that don't rely on actually launching processes from executables on disk. Its `run` and `runSync` methods provide asynchronous and synchronous variants of this functionality. Previously, the behaviour of `run` and `runSync` were inconsistent with regards to the treatment of the `stdoutEncoding` (similarly, `stderrEncoding`) parameters: `run`: * if the encoding was null, `ProcessResult.stdout` was returned as a String in UTF-8 encoding. This was incorrect. The behaviour as specified in `ProcessResult.stdout` is that in this case, a raw `List<int>` should be returned. * If the encoding was unspecified, `ProcessResult.stdout` was returned as a `String` in the `io.systemEncoding` encoding. This was correct. * If the encoding was non-null, `ProcessResult.stdout` was returned as a `String` in the specified encoding. This was correct. `runSync`: * if the encoding was null, `ProcessResult.stdout` was returned as a `List<int>` in UTF-8 encoding. This was incorrect. The behaviour as specified in `ProcessResult.stdout` is that in this case, a raw `List<int>` should be returned. * If the encoding was unspecified, `ProcessResult.stdout` was returned as `List<int>` in UTF-8 encoding. This was incorrect. The behaviour as specified in `ProcessResult.stdout` is that in this case, a String a `String` in the `io.systemEncoding` encoding should be returned. * if the encoding was non-null, `ProcessResult.stdout` was returned as a `String` in unknown (but probably UTF-8) encoding. This was incorrect. The behaviour as specified in `ProcessResult.stdout` is that in this case, a `String` in the specified encoding should be returned. `_FakeProcess`, from which we obtain the fake stdout and stderr values now holds these fields as raw `List<int>` of bytes rather than as `String`s. It is up to the user to supply values that can be decoded with the encoding passed to `run`/`runAsync`. `run` and `runAsync` have been updated to set stdout (likewise, stderr) as specified in the `ProcessResult` documentation. This is pre-factoring for #102451, in which the tool throws an exception when processing the JSON output from stdout of the `vswhere.exe` tool, whose output was found to include the `U+FFFD` Unicode replacement character during UTF-8 decoding, which triggers a `toolExit` exception when decoded using our [Utf8Decoder][decoder] configured with `reportErrors` = true. Because `FakeProcessManager.runAsync` did not previously invoke `utf8.decode` on its output (behaviour which differs from the non-fake implementation), it was impossible to write tests to verify the fix. Ref: https://api.flutter.dev/flutter/dart-io/ProcessResult/stdout.html Issue: #102451 [decoder]: https://github.com/flutter/flutter/blob/fd312f1ccff909fde28d2247a489bf210bbc6c48/packages/flutter_tools/lib/src/convert.dart#L51-L60 * Refactor vswhere.exe integration (#104133) `VisualStudio` calls `vswhere.exe` to find Visual Studio installations and determine if they satisfy Flutter's requirements. Previously, `VisualStudio` stored the JSON output from `vswhere.exe` as `Map`s, resulting in duplicated logic to read the JSON output (once to validate values, second to expose values). Also, `VisualStudio` stored two copies of the JSON output (the latest valid installation as well as the latest VS installation). This change simplifies `VisualStudio` by introducing a new `VswhereDetails`. This type contains the logic to read `vswhere.exe`'s JSON output, and, understand whether an installation is usable by Flutter. In the future, this `VswhereDetails` type will be used to make Flutter doctor resilient to bad UTF-8 output from `vswhere.exe`. Part of #102451. * Ignore replacement characters from vswhere.exe output (#104284) Flutter uses `vswhere.exe` to find Visual Studio installations and determine if they satisfy Flutter's requirements. However, `vswhere.exe`'s JSON output is known to contain bad UTF-8. This change ignores bad UTF-8 as long as they affect JSON properties that are either unused, or, used only for display purposes by Flutter. Fixes: #102451 Co-authored-by: Chris Bracken <chris@bracken.jp> Co-authored-by: Kevin Chisholm <kevinjchisholm@google.com>

`VisualStudio` calls `vswhere.exe` to find Visual Studio installations and determine if they satisfy Flutter's requirements. Previously, `VisualStudio` stored the JSON output from `vswhere.exe` as `Map`s, resulting in duplicated logic to read the JSON output (once to validate values, second to expose values). Also, `VisualStudio` stored two copies of the JSON output (the latest valid installation as well as the latest VS installation). This change simplifies `VisualStudio` by introducing a new `VswhereDetails`. This type contains the logic to read `vswhere.exe`'s JSON output, and, understand whether an installation is usable by Flutter. In the future, this `VswhereDetails` type will be used to make Flutter doctor resilient to bad UTF-8 output from `vswhere.exe`. Part of flutter#102451.

loic-sharma added 3 commits May 18, 2022 17:14

WIP

a496de1

Work

Fix

0c2d84f

Fixes

5a8c012

flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label May 19, 2022

loic-sharma requested review from cbracken, gspencergoog and jonahwilliams May 19, 2022 00:39

loic-sharma commented May 19, 2022

View reviewed changes

loic-sharma added 2 commits May 18, 2022 17:46

Fix

f7e334a

Address lints

ddb8e77

cbracken reviewed May 19, 2022

View reviewed changes

loic-sharma added 5 commits May 19, 2022 10:10

Remove constants

992244f

Style fixes

8a6b545

Address feedback

1c8401f

Fix

23f75a9

Tweak

648d3ea

cbracken approved these changes May 19, 2022

View reviewed changes

loic-sharma added 3 commits May 19, 2022 15:04

Improve tests

bf9e0e8

Address final feedback

c8d4b8b

Fix spelling

f047bb8

cbracken approved these changes May 19, 2022

View reviewed changes

loic-sharma merged commit 52e19ef into flutter:master May 20, 2022

engine-flutter-autoroll mentioned this pull request May 20, 2022

Roll Flutter from 17f3618ca34c to 52e19ef7ec31 (2 revisions) flutter/packages#2019

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2022

52e19ef Refactor vswhere.exe integration (flutter/flutter#104133)

802af9a

engine-flutter-autoroll mentioned this pull request May 21, 2022

Roll Flutter from a4a8e73bce15 to 1e1f4bcfb561 (65 revisions) flutter/plugins#5795

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request May 21, 2022

52e19ef Refactor vswhere.exe integration (flutter/flutter#104133)

c5e4a6b

loic-sharma mentioned this pull request Jun 1, 2022

[CP] Fix Flutter doctor crash on Windows caused by bad UTF8 from vswhere.exe #104654

Closed

loic-sharma mentioned this pull request Jun 1, 2022

[CP] Fix Flutter doctor crash on Windows caused by bad UTF8 from vswhere.exe #105153

Merged

8 tasks

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022

52e19ef Refactor vswhere.exe integration (flutter/flutter#104133)

7a29b44

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 2022

52e19ef Refactor vswhere.exe integration (flutter/flutter#104133)

116740f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Refactor vswhere.exe integration#104133

Refactor vswhere.exe integration#104133
loic-sharma merged 13 commits intoflutter:masterfrom
loic-sharma:vswhere_refactor

loic-sharma commented May 19, 2022 •

edited

Loading

Uh oh!

loic-sharma May 19, 2022 •

edited

Loading

Uh oh!

loic-sharma May 19, 2022

Uh oh!

cbracken left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cbracken left a comment

Uh oh!

cbracken left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

Conversation

loic-sharma commented May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

loic-sharma May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loic-sharma May 19, 2022

Choose a reason for hiding this comment

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

loic-sharma commented May 19, 2022 •

edited

Loading

loic-sharma May 19, 2022 •

edited

Loading