Skip to content

Conversation

@jmeistrich
Copy link
Contributor

Summary:

This fixes multiple issues on Windows:

  1. It was crashing when checking Android SDKs if no Android SDKs installed.
  2. It was checking for watchman, which is not part of the install guide for Windows.
  3. There is a bug in envinfo which does not get Android Studio info correctly: fix: Android SDK info not working on Windows tabrindle/envinfo#119. This has a workaround until that is fixed.

Test Plan:

Run doctor on a Windows machine with no Android SDKs installed to see #1, and then install Android SDKs and see that it was still showing an error.

: 'sdkmanager --list',
(err, stdout) => {
if (err) {
resolve('Not Found');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be reject()?

Copy link
Member

Choose a reason for hiding this comment

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

I was wondering same thing. This probably should be reject.


return {
needsToBeFixed:
(sdks === 'Not Found' && installMessage) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make 'Not Found' a constant or make sdks undefined instead?

? {
'Build Tools': matches,
}
: 'Not Found',
Copy link
Member

Choose a reason for hiding this comment

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

Not Found is resolved here as well.

@jmeistrich
Copy link
Contributor Author

I changed it to reject on failure and set sdks to 'Not Found' in the catch so it's just in one place. Is this better?

I don't think it should make sdks undefined because the goal is to fix the output of the broken call in envinfo, and once that's fixed this block can just be removed. envinfo sets it to 'Not Found' so this should just replicate that.

@thymikee thymikee merged commit 05ef7bd into react-native-community:master Sep 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants