Skip to content

Conversation

@loic-sharma
Copy link
Member

@loic-sharma loic-sharma commented Jun 1, 2022

Flutter doctor crashes on Windows when using the latest version of Visual Studio and certain locales. This cherrypick the fixes to the stable release:

Cherrypick issue: #104654

Fixes: #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.

cbracken and others added 3 commits June 1, 2022 10:05
`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 flutter#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: flutter#102451

[decoder]: https://github.com/flutter/flutter/blob/fd312f1ccff909fde28d2247a489bf210bbc6c48/packages/flutter_tools/lib/src/convert.dart#L51-L60
`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.
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: flutter#102451
@flutter-dashboard
Copy link

This pull request was opened from and to a release candidate branch. This should only be done as part of the official Flutter release process. If you are attempting to make a regular contribution to the Flutter project, please close this PR and follow the instructions at Tree Hygiene for detailed instructions on contributing to Flutter.

Reviewers: Use caution before merging pull requests to release branches. Ensure the proper procedure has been followed.

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jun 1, 2022
Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

LGTM stamp from a Japanese personal seal

@itsjustkevin
Copy link
Contributor

The following tests have been removed:

  • Linux build_aar_module_test
  • Mac build_aar_module_test
  • Mac gradle_plugin_fat_apk_test
  • Windows build_aar_module_test

@itsjustkevin itsjustkevin merged commit 8b4f0d4 into flutter:flutter-2.13-candidate.0 Jun 2, 2022
bernard-lee added a commit to bernard-lee/flutter that referenced this pull request Jun 29, 2022
* 'stable' of https://github.com/flutter/flutter: (1203 commits)
  'Update Engine revision to ffe7b86a1e5b5cb63c8385ae1adc759e372ee8f4 for stable release 3.0.3' (flutter#106431)
  [flutter_releases] Removing minor iOS version filter value from ci.yaml (flutter#105059) (flutter#105629)
  [flutter_releases] Flutter stable 3.0.2 Framework Cherrypicks (flutter#105528)
  [framework] ensure ink sparkle is disposed (flutter#104569) (flutter#105144)
  [CP] Fix Flutter doctor crash on Windows caused by bad UTF8 from vswhere.exe (flutter#105153)
  [tool][web] Use 'constructor' instead of 'public field declarations' in flutter.js (Safari 13) (flutter#105162)
  [flutter_releases] Cherrypicks for SliverReorderableList and Slider regressions (flutter#105141)
  'Update Engine revision to caaafc5604ee9172293eb84a381be6aadd660317 for stable release 3.0.1' (flutter#104213)
  [flutter_releases] Flutter stable 2.13.0 Framework Cherrypicks (flutter#103375)
  [flutter_releases] Flutter beta 2.13.0-0.4.pre Framework Cherrypicks (flutter#103101)
  Enforce cpu explicitly for Mac devicelab test beds (flutter#101871) (flutter#102685)
  [flutter_releases] Flutter beta 2.13.0-0.3.pre Framework Cherrypicks (flutter#102620)
  [flutter_releases] Upgrade dwds to 12.1.1 (flutter#101546)
  [flutter_releases] Flutter beta 2.13.0-0.2.pre Framework Cherrypicks (flutter#102193)
  [flutter_releases] Flutter beta 2.12.0-4.1.pre Framework Cherrypicks (flutter#101771)
  [flutter_releases] Cherrypick engine to c341645 (flutter#101608)
  Revert "Refactor `ToggleButtons` (remove `RawMaterialButton`) (flutter#99493)" (flutter#101538)
  [Cherrypick] Partial revert of super params in tools (flutter#101436) (flutter#101527)
  Roll Engine from e7e7ca1 to b48d65e (10 revisions) (flutter#101370)
  [flutter_tools] port bash script to use sysctl not uname on macOS (flutter#101308)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants