Skip to content

Specify encoding for vswhere output - #59607

Merged
stuartmorgan-g merged 2 commits into
flutter:masterfrom
stuartmorgan-g:windows-vswhere-encoding
Jun 18, 2020
Merged

Specify encoding for vswhere output#59607
stuartmorgan-g merged 2 commits into
flutter:masterfrom
stuartmorgan-g:windows-vswhere-encoding

Conversation

@stuartmorgan-g

@stuartmorgan-g stuartmorgan-g commented Jun 16, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

On Windows, Process.run assumes the output uses the system codepage by default. This allows specifying it in our wrapper, and sets the encoding for vswhere to UTF-8 since we're passing a flag that forces it to use UTF-8 output.

Related Issues

Fixes #53515

Tests

I added the following tests: Ensure vswhere is called with the correct output encoding.

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 read the Tree Hygiene wiki page, which explains my responsibilities.
  • 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

Did any tests fail when you ran them? Please read Handling breaking changes.

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change.

@fluttergithubbot fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jun 16, 2020
@fluttergithubbot

fluttergithubbot commented Jun 16, 2020

Copy link
Copy Markdown
Contributor

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@stuartmorgan-g stuartmorgan-g changed the title Specifi Specify encoding for vswhere output Jun 16, 2020
@jonahwilliams

jonahwilliams commented Jun 16, 2020

Copy link
Copy Markdown
Contributor

you might need to pull latest master? I'm not sure when you branched but it seems to have issues

jonahwilliams
jonahwilliams approved these changes Jun 16, 2020
View reviewed changes

@jonahwilliams jonahwilliams left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

(but not to CI)

@stuartmorgan-g

stuartmorgan-g commented Jun 17, 2020

Copy link
Copy Markdown
Contributor Author

Everything says "The current Flutter SDK version is 0.0.0-unknown". I'm not sure what's going on there.

@jonahwilliams

jonahwilliams commented Jun 17, 2020

Copy link
Copy Markdown
Contributor

Did you branch in a weird way? on master the flutter version is determined by the first parent, so in this case subsequent pulls might not fix it.

You could create a new PR off of upstream/master and merge this into that

@stuartmorgan-g
stuartmorgan-g force-pushed the windows-vswhere-encoding branch from aee28e2 to d0067ba Compare June 17, 2020 17:01
@stuartmorgan-g

stuartmorgan-g commented Jun 17, 2020

Copy link
Copy Markdown
Contributor Author

🤷 I'm not sure what I did differently here (I made the branch a while ago), but rebase + force push seems to be making things happier.

@stuartmorgan-g
stuartmorgan-g merged commit e85655c into flutter:master Jun 18, 2020
@stuartmorgan-g
stuartmorgan-g deleted the windows-vswhere-encoding branch June 18, 2020 21:00
zljj0818 pushed a commit to zljj0818/flutter that referenced this pull request Jun 22, 2020
@stuartmorgan-g @zljj0818
Specify encoding for vswhere output (flutter#59607)
768f317 
On Windows, Process.run assumes the output uses the system codepage by default. This allows specifying it in our wrapper, and sets the encoding for vswhere to UTF-8 since we're passing a flag that forces it to use UTF-8 output.

Fixes flutter#53515
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
@stuartmorgan-g @mingwandroid
Specify encoding for vswhere output (flutter#59607)
8302ce0 
On Windows, Process.run assumes the output uses the system codepage by default. This allows specifying it in our wrapper, and sets the encoding for vswhere to UTF-8 since we're passing a flag that forces it to use UTF-8 output.

Fixes flutter#53515
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Jul 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@jonahwilliams jonahwilliams jonahwilliams 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.

vswhere output parsing uses wrong encoding, causing path failures for some VS installations

4 participants

@stuartmorgan-g @fluttergithubbot @jonahwilliams @googlebot