Skip to content

[CP] Ignore invalid description in vswhere.exe JSON output #107328

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
itsjustkevin merged 1 commit into from
Jul 12, 2022
Merged

[CP] Ignore invalid description in vswhere.exe JSON output #107328

itsjustkevin merged 1 commit into from
Jul 12, 2022

Conversation

@loic-sharma
Copy link
Member

@loic-sharma loic-sharma commented Jul 8, 2022

The flutter doctor, flutter build, and flutter run commands fail on Windows using certain locales when using the latest version of Visual Studio. This cherrypicks the fix to the latest stable release: dcbdff0 from #106836.

Cherry pick issue: #106920

Fixes: #106601

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.

@flutter-dashboard
Copy link

flutter-dashboard bot commented Jul 8, 2022

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 Jul 8, 2022
christopherfujino
christopherfujino approved these changes Jul 8, 2022
View reviewed changes
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.

code lgtm

@loic-sharma loic-sharma force-pushed the flutter-2.13-candidate.0 branch from a80240e to f19609a Compare July 9, 2022 01:03
@loic-sharma
Ignore invalid description property in vswhere.exe JSON output (flutt…
75abe1c 
…er#106836)

The `flutter doctor` command uses `vswhere.exe` to verify the Visual Studio installation. This `vswhere.exe` is known to encode its output incorrectly. This is problematic as the `description` property is localized, and in certain languages this results in invalid JSON due to the incorrect encoding.

This change introduces a fallback to our `vswhere.exe` output parsing logic: if parsing JSON fails, remove the `description` property and retry parsing the JSON.

This fix was also tested on the outputs provided here: flutter#106601 (comment)

Addresses flutter#106601
@loic-sharma loic-sharma force-pushed the flutter-2.13-candidate.0 branch from f19609a to 75abe1c Compare July 11, 2022 17:56
@itsjustkevin
Copy link
Contributor

itsjustkevin commented Jul 11, 2022

@muditatandon @CaseyHillers Any idea what is going on with these failing tests?

@CaseyHillers
Copy link
Contributor

CaseyHillers commented Jul 11, 2022

The web tests are timing out (likely due to something crashing)

@christopherfujino
Copy link
Contributor

christopherfujino commented Jul 11, 2022 •
edited
Loading

Linux build_tests_1_2 failing because ninja not found (https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8808913699168063233/+/u/run_test.dart_for_build_tests_shard_and_subshard_1_2/test_stdout):

[   +7 ms] executing: [build/linux/x64/debug/] cmake -G Ninja -DCMAKE_BUILD_TYPE=Debug -DFLUTTER_TARGET_PLATFORM=linux-x64 /b/s/w/ir/x/w/flutter/dev/integration_tests/flutter_gallery/linux
[  +39 ms] CMake Error: CMake was unable to find a build program corresponding to "Ninja".  CMAKE_MAKE_PROGRAM is not set.  You probably need to select a different build tool.
[   +1 ms] CMake Error: CMAKE_CXX_COMPILER not set, after EnableLanguage
[        ] -- Configuring incomplete, errors occurred!

I wonder if this dependency changed upstream since the branch?

itsjustkevin
itsjustkevin approved these changes Jul 12, 2022
View reviewed changes
Copy link
Contributor

@itsjustkevin itsjustkevin left a comment

Choose a reason for hiding this comment

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

LGTM

@itsjustkevin itsjustkevin merged commit 21ee205 into flutter:flutter-2.13-candidate.0 Jul 12, 2022
@loic-sharma loic-sharma deleted the flutter-2.13-candidate.0 branch July 12, 2022 18:07
@bdero bdero mentioned this pull request Jul 21, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@itsjustkevin itsjustkevin itsjustkevin approved these changes

+1 more reviewer

@christopherfujino christopherfujino christopherfujino approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@loic-sharma @itsjustkevin @CaseyHillers @christopherfujino