[windows] Searches for pre-release and 'all' Visual Studio installations by franciscojma86 · Pull Request #40011 · flutter/flutter

franciscojma86 · 2019-09-07T00:09:21Z

Pre-release VS installations with the required components where not being found. There is now a new search for such installations, as well as any other type of installations:

not launchable
incomplete
reboot required

along with a new user message for each of these cases.

#35350
#37831

I added tests for visual_studio_test.dart and visual_studio_validator_test.dart

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I signed the [CLA].
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read [Handling breaking changes]). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

googlebot · 2019-09-07T00:09:27Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-09-07T00:12:06Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

codecov · 2019-09-07T00:58:42Z

Codecov Report

Merging #40011 into master will increase coverage by 0.14%.
The diff coverage is 86.66%.

@@            Coverage Diff             @@
##           master   #40011      +/-   ##
==========================================
+ Coverage   57.88%   58.02%   +0.14%     
==========================================
  Files         194      194              
  Lines       18726    18768      +42     
==========================================
+ Hits        10839    10891      +52     
+ Misses       7887     7877      -10

Flag	Coverage Δ
#flutter_tool	`58.02% <86.66%> (+0.14%)`	⬆️

Impacted Files	Coverage Δ
...tools/lib/src/windows/visual_studio_validator.dart	`92.59% <100%> (+3.11%)`	⬆️
...ages/flutter_tools/lib/src/base/user_messages.dart	`46.36% <100%> (-2.7%)`	⬇️
...s/flutter_tools/lib/src/windows/visual_studio.dart	`80.55% <81.25%> (-2.06%)`	⬇️
...lutter_tools/lib/src/android/android_workflow.dart	`30.18% <0%> (-33.34%)`	⬇️
...kages/flutter_tools/lib/src/web/web_validator.dart	`53.84% <0%> (-23.08%)`	⬇️
packages/flutter_tools/lib/src/commands/logs.dart	`6.25% <0%> (-12.5%)`	⬇️
packages/flutter_tools/lib/src/version.dart	`90.9% <0%> (-1.92%)`	⬇️
packages/flutter_tools/lib/src/cache.dart	`43.5% <0%> (-0.73%)`	⬇️
.../flutter_tools/lib/src/runner/flutter_command.dart	`80.26% <0%> (-0.44%)`	⬇️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af07bb5...ea4d3fc. Read the comment docs.

stuartmorgan-g

Overall looks great, just a few small notes.

stuartmorgan-g · 2019-09-08T19:19:07Z

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

-        _vswherePath,
-        '-format', 'json',
+      final List<String> defaultArguments = <String>[
+        '-format',


Nit: since we're formatting manually anyway, I would prefer keeping 'json' on the same line with this to make it obvious that it's conceptually one argument.

Done. I was wondering why it was indented like that, but makes sense.

stuartmorgan-g · 2019-09-08T19:20:14Z

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

+    // If a stable version is not found, try searching for a pre-release version.
+    _cachedUsableVisualStudioDetails ??= _visualStudioDetails(
+        requiredComponents: _requiredComponents().keys,
+        additionalArguments: <String>['-prerelease']);


Nit: maybe make -prerelease an internal string constant since it's used twice.

stuartmorgan-g · 2019-09-08T19:21:16Z

packages/flutter_tools/lib/src/base/user_messages.dart

+  String get visualStudioNotLaunchable =>
+      'The current Visual Studio installation is not launchable. Please reinstall Visual Studio.';
+  String get visualStudioIsIncomplete => 'The current Visual Studio installation is incomplete. Please reinstall Visual Studio';
+  String get visualStudioRebootRequired => 'Visual Studio requires a reboot of your system to complete installation';


Please give these strings trailing .s for consistency with the other messages.

stuartmorgan-g · 2019-09-08T19:24:24Z

packages/flutter_tools/lib/src/base/user_messages.dart

  String get visualStudioMissing =>
      'Visual Studio not installed; this is necessary for Windows development.\n'
      'Download at https://visualstudio.microsoft.com/downloads/.';
+  String get visualStudioIsPrerelease => 'The current Visual Studio installation is a pre-release version';


We should probably provide a bit more context about what effect this may have. Let's add another sentence: "It may not be supported by Flutter yet."

stuartmorgan-g · 2019-09-08T19:27:08Z

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

+      }
+
+      // Messages for faulty installations.
+      if (!visualStudio.isComplete) {


We probably shouldn't add all of these, as I'm not sure how they combine in practice (e.g., it may be that when a reboot required it also reports as incomplete) and we don't want to overwhelm the user with potentially conflicting information. Let's do these three as:

if (...isRebootRequired) { ... } else if (!...isComplete) { ... } else if (!...isLaunchable) { ... }

And actually, some of the issue reports we've had (see #39970) suggest that if the install isn't fully complete, the components checks don't necessarily work as we would expect which can cause confusion, so let's add the !...hasNecessaryComponents) check to the end of that chain as an else-if as well.

stuartmorgan-g · 2019-09-08T19:36:39Z

packages/flutter_tools/test/general.shard/windows/visual_studio_test.dart

+      ProcessManager: () => mockProcessManager,
+    });
+
+    testUsingContext('vcvarsPath returns null when VS is present but missing components', () {


I don't know if it's possible for the components to show as present but the install to be unusable due to one of the flags we are checking. To err on the safe side, let's also test some cases where components are present but one of the bad flags (e.g., needs reboot) are true. That will require some code changes to pass I believe—maybe change the logic of how _usableVisualStudioDetails is computed internally so that if one of the checks there finds a version that has one of the problem flags set, it's assigned to _cachedAnyVisualStudioDetails instead of _cachedUsableVisualStudioDetails.

stuartmorgan-g

LGTM!

stuartmorgan-g · 2019-09-10T18:13:01Z

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

+    if (_cachedUsableVisualStudioDetails != null) {
+      if (installationHasIssues(_cachedUsableVisualStudioDetails)) {
+        _cachedAnyVisualStudioDetails = _cachedUsableVisualStudioDetails;
+        return null;


It occurs to me reviewing this with more distance from having written it that the use of null to indicate both not having found anything usable and not having checked means we're going to run vswhere a bunch of extra times. It's a pre-existing problem which this change will make worse.

Could you do a follow-up PR to change the structure of these functions to use a sentinel value (maybe {}) for _cached* in cases where we have checked and not found anything?

Here! #40197

Fixes a crash introduced on #40011 due to an incorrect type in the vswhere search Fixes #40238

…ons (flutter#40011)

Fixes a crash introduced on flutter#40011 due to an incorrect type in the vswhere search Fixes flutter#40238

fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Sep 7, 2019

franciscojma86 requested a review from stuartmorgan-g September 7, 2019 00:09

googlebot added the cla: no label Sep 7, 2019

franciscojma86 requested a review from jonahwilliams September 7, 2019 00:09

franciscojma86 force-pushed the vswhere branch from 4a6d6b9 to 3e28019 Compare September 7, 2019 00:12

googlebot added cla: yes and removed cla: no labels Sep 7, 2019

franciscojma86 added the platform-windows Building on or for Windows specifically label Sep 7, 2019

[windows] Searches for pre-release and 'all' Visual Studio installations

8c87041

franciscojma86 force-pushed the vswhere branch from 28d830a to 8c87041 Compare September 7, 2019 03:21

stuartmorgan-g requested changes Sep 8, 2019

View reviewed changes

Address comments, adds more tests

db8c578

stuartmorgan-g approved these changes Sep 10, 2019

View reviewed changes

Whitespace

ea4d3fc

franciscojma86 merged commit 362cde4 into flutter:master Sep 11, 2019

franciscojma86 deleted the vswhere branch September 11, 2019 00:01

franciscojma86 mentioned this pull request Sep 11, 2019

Fix crash on vswhere search from flutter doctor #40263

Merged

9 tasks

stuartmorgan-g pushed a commit that referenced this pull request Sep 12, 2019

Fix crash on vswhere search from flutter doctor (#40263)

f5733f7

Fixes a crash introduced on #40011 due to an incorrect type in the vswhere search Fixes #40238

Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Sep 30, 2019

[windows] Searches for pre-release and 'all' Visual Studio installati…

976d9b7

…ons (flutter#40011)

Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Sep 30, 2019

Fix crash on vswhere search from flutter doctor (flutter#40263)

830cb95

Fixes a crash introduced on flutter#40011 due to an incorrect type in the vswhere search Fixes flutter#40238

github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021

Conversation

franciscojma86 commented Sep 7, 2019 • edited by stuartmorgan-g Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Breaking Change

Uh oh!

googlebot commented Sep 7, 2019

Uh oh!

googlebot commented Sep 7, 2019

Uh oh!

codecov bot commented Sep 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

franciscojma86 commented Sep 7, 2019 •

edited by stuartmorgan-g

Loading

codecov bot commented Sep 7, 2019 •

edited

Loading