From b625efee3d2fdce19c288c5135e12fe78e2572ca Mon Sep 17 00:00:00 2001 From: Anton Nikolaev Date: Thu, 23 Jan 2020 13:35:12 -0800 Subject: [PATCH] Doctor complains Android SDK is not installed Summary: There are complaints about Android SDK being reported as "not installed" when it is actually installed. To address them, I changed the way how we detect SDK and also added some minimal actionable feedback for each check. The problem with the previous implementation of Android SDK check via "envinfo" is that the library uses "sdkmanager" tool under the hood, and this tool doesn't work on Java 9+. To fix this I'm changing the way how we assume SDK is installed to simple check for "adb" tool existence. Actionable feedback is shown on Doctor report when you click to an item. Reviewed By: jknoxville Differential Revision: D19517769 fbshipit-source-id: 1c21f1bdcd05c7c0ae3f97b9c3454efa2c861d26 --- doctor/package.json | 2 +- doctor/src/environmentInfo.ts | 25 +--- doctor/src/index.ts | 130 ++++++++++++++----- package.json | 2 +- src/chrome/DoctorSheet.tsx | 75 +++++++---- src/reducers/__tests__/healthchecks.node.tsx | 9 +- src/reducers/healthchecks.tsx | 1 - src/utils/runHealthchecks.tsx | 6 +- yarn.lock | 8 +- 9 files changed, 161 insertions(+), 97 deletions(-) diff --git a/doctor/package.json b/doctor/package.json index d97d1119e..5486a0b02 100644 --- a/doctor/package.json +++ b/doctor/package.json @@ -1,6 +1,6 @@ { "name": "flipper-doctor", - "version": "0.6.1", + "version": "0.7.0", "description": "Utility for checking for issues with a flipper installation", "main": "lib/index.js", "types": "lib/index.d.ts", diff --git a/doctor/src/environmentInfo.ts b/doctor/src/environmentInfo.ts index 65b0b8b6f..bdbcaef5e 100644 --- a/doctor/src/environmentInfo.ts +++ b/doctor/src/environmentInfo.ts @@ -35,35 +35,14 @@ async function retrieveAndParseEnvInfo(): Promise { return JSON.parse( await run( { - SDKs: ['iOS SDK', 'Android SDK'], + SDKs: ['iOS SDK'], IDEs: ['Xcode'], - Languages: ['Java'], }, {json: true, showNotFound: true}, ), ); } -// Temporary workaround for https://github.com/facebook/flipper/issues/667 until it properly fixed in 'envinfo'. -async function workaroundForNewerJavaVersions(envInfo: any) { - try { - if (envInfo.Languages.Java && envInfo.Languages.Java.version) { - const [majorVersion] = envInfo.Languages.Java.version - .split('.') - .slice(0, 1) - .map((x: string) => parseInt(x, 10)); - if (8 < majorVersion && majorVersion < 11) { - process.env.JAVA_OPTS = - '-XX:+IgnoreUnrecognizedVMOptions --add-modules java.se.ee'; - return await retrieveAndParseEnvInfo(); - } - } - } catch (e) { - console.error(e); - } - return envInfo; -} - export async function getEnvInfo(): Promise { - return workaroundForNewerJavaVersions(await retrieveAndParseEnvInfo()); + return await retrieveAndParseEnvInfo(); } diff --git a/doctor/src/index.ts b/doctor/src/index.ts index 19ac3b415..8cbc7706e 100644 --- a/doctor/src/index.ts +++ b/doctor/src/index.ts @@ -10,8 +10,10 @@ import {exec} from 'child_process'; import {promisify} from 'util'; import {EnvironmentInfo, getEnvInfo} from './environmentInfo'; -export {getEnvInfo} from './environmentInfo'; +export {EnvironmentInfo, getEnvInfo} from './environmentInfo'; import * as watchman from 'fb-watchman'; +import * as fs from 'fs'; +import * as path from 'path'; export type HealthcheckCategory = { label: string; @@ -36,12 +38,12 @@ export type Healthcheck = { key: string; label: string; isRequired?: boolean; - run: ( - env: EnvironmentInfo, - ) => Promise<{ - hasProblem: boolean; - helpUrl?: string; - }>; + run: (env: EnvironmentInfo) => Promise; +}; + +export type HealthchecRunResult = { + hasProblem: boolean; + message: string; }; export type CategoryResult = [ @@ -68,9 +70,14 @@ export function getHealthchecks(): Healthchecks { key: 'common.openssl', label: 'OpenSSL Installed', run: async (_: EnvironmentInfo) => { - const isAvailable = await commandSucceeds('openssl version'); + const result = await tryExecuteCommand('openssl version'); + const hasProblem = result.hasProblem; + const message = hasProblem + ? `OpenSSL (https://wiki.openssl.org/index.php/Binaries) is not installed or not added to PATH. ${result.message}.` + : `OpenSSL (https://wiki.openssl.org/index.php/Binaries) is installed and added to PATH. ${result.message}.`; return { - hasProblem: !isAvailable, + hasProblem, + message, }; }, }, @@ -81,6 +88,9 @@ export function getHealthchecks(): Healthchecks { const isAvailable = await isWatchmanAvailable(); return { hasProblem: !isAvailable, + message: isAvailable + ? 'Watchman file watching service (https://facebook.github.io/watchman/) is installed and added to PATH. Live reloading after changes during Flipper plugin development is enabled.' + : 'Watchman file watching service (https://facebook.github.io/watchman/) is not installed or not added to PATH. Live reloading after changes during Flipper plugin development is disabled.', }; }, }, @@ -95,9 +105,28 @@ export function getHealthchecks(): Healthchecks { key: 'android.sdk', label: 'SDK Installed', isRequired: true, - run: async (e: EnvironmentInfo) => ({ - hasProblem: e.SDKs['Android SDK'] === 'Not Found', - }), + run: async (_: EnvironmentInfo) => { + if (process.env.ANDROID_HOME) { + const androidHome = process.env.ANDROID_HOME; + if (!fs.existsSync(androidHome)) { + return { + hasProblem: true, + message: `ANDROID_HOME points to a folder which does not exist: ${androidHome}.`, + }; + } + const platformToolsDir = path.join(androidHome, 'platform-tools'); + if (!fs.existsSync(path.join(androidHome, 'platform-tools'))) { + return { + hasProblem: true, + message: `Android SDK Platform Tools not found at the expected location "${platformToolsDir}". Probably they are not installed.`, + }; + } + return await tryExecuteCommand( + path.join(platformToolsDir, 'adb') + ' version', + ); + } + return await tryExecuteCommand('adb version'); + }, }, ], }, @@ -112,40 +141,66 @@ export function getHealthchecks(): Healthchecks { key: 'ios.sdk', label: 'SDK Installed', isRequired: true, - run: async (e: EnvironmentInfo) => ({ - hasProblem: + run: async (e: EnvironmentInfo) => { + const hasProblem = !e.SDKs['iOS SDK'] || !e.SDKs['iOS SDK'].Platforms || - !e.SDKs['iOS SDK'].Platforms.length, - }), + !e.SDKs['iOS SDK'].Platforms.length; + const message = hasProblem + ? 'iOS SDK is not installed. You can install it using Xcode (https://developer.apple.com/xcode/).' + : `iOS SDK is installed for the following platforms: ${JSON.stringify( + e.SDKs['iOS SDK'].Platforms, + )}.`; + return { + hasProblem, + message, + }; + }, }, { key: 'ios.xcode', label: 'XCode Installed', isRequired: true, - run: async (e: EnvironmentInfo) => ({ - hasProblem: e.IDEs == null || e.IDEs.Xcode == null, - }), + run: async (e: EnvironmentInfo) => { + const hasProblem = e.IDEs == null || e.IDEs.Xcode == null; + const message = hasProblem + ? 'Xcode (https://developer.apple.com/xcode/) is not installed.' + : `Xcode version ${e.IDEs.Xcode.version} is installed at "${e.IDEs.Xcode.path}".`; + return { + hasProblem, + message, + }; + }, }, { key: 'ios.xcode-select', label: 'xcode-select set', isRequired: true, - run: async (_: EnvironmentInfo) => ({ - hasProblem: !(await commandSucceeds('xcode-select -p')), - }), + run: async (_: EnvironmentInfo) => { + const result = await tryExecuteCommand('xcode-select -p'); + const hasProblem = result.hasProblem; + const message = hasProblem + ? `Xcode version is not selected. You can select it using command "sudo xcode-select -switch Xcode.app". ${result.message}.` + : `Xcode version is selected. ${result.message}.`; + return { + hasProblem, + message, + }; + }, }, { key: 'ios.instruments', label: 'Instruments exists', isRequired: true, run: async (_: EnvironmentInfo) => { - const hasInstruments = await commandSucceeds( - 'which instruments', - ); - + const result = await tryExecuteCommand('which instruments'); + const hasProblem = result.hasProblem; + const message = hasProblem + ? `Instruments not found. Please try to re-install Xcode (https://developer.apple.com/xcode/). ${result.message}.` + : `Instruments are installed. ${result.message}.`; return { - hasProblem: !hasInstruments, + hasProblem, + message, }; }, }, @@ -153,7 +208,7 @@ export function getHealthchecks(): Healthchecks { } : { isSkipped: true, - skipReason: `Healthcheck is skipped, because iOS development is not supported on the current platform "${process.platform}"`, + skipReason: `Healthcheck is skipped, because iOS development is not supported on the current platform "${process.platform}".`, }), }, }; @@ -199,10 +254,21 @@ export async function runHealthchecks(): Promise< return results; } -async function commandSucceeds(command: string): Promise { - return await promisify(exec)(command) - .then(() => true) - .catch(() => false); +async function tryExecuteCommand( + command: string, +): Promise { + try { + const output = await promisify(exec)(command); + return { + hasProblem: false, + message: `Command "${command}" successfully executed with output: ${output.stdout}`, + }; + } catch (err) { + return { + hasProblem: true, + message: `Command "${command}" failed to execute with output: ${err.message}`, + }; + } } async function isWatchmanAvailable(): Promise { diff --git a/package.json b/package.json index 51840a66d..752ff293e 100644 --- a/package.json +++ b/package.json @@ -140,7 +140,7 @@ "expand-tilde": "^2.0.2", "express": "^4.15.2", "fb-watchman": "^2.0.0", - "flipper-doctor": "^0.6.1", + "flipper-doctor": "^0.7.0", "fs-extra": "^8.0.1", "immer": "^5.2.1", "immutable": "^4.0.0-rc.12", diff --git a/src/chrome/DoctorSheet.tsx b/src/chrome/DoctorSheet.tsx index 9824801ef..f6d251804 100644 --- a/src/chrome/DoctorSheet.tsx +++ b/src/chrome/DoctorSheet.tsx @@ -88,6 +88,7 @@ const SideContainer = styled(FlexBox)({ const SideContainerText = styled(Text)({ display: 'block', wordWrap: 'break-word', + overflow: 'auto', }); const HealthcheckLabel = styled(Text)({ @@ -170,6 +171,7 @@ function HealthcheckIcon(props: {checkResult: HealthcheckResult}) { function HealthcheckDisplay(props: { label: string; result: HealthcheckResult; + selected?: boolean; onClick?: () => void; }) { return ( @@ -177,6 +179,7 @@ function HealthcheckDisplay(props: { @@ -187,27 +190,25 @@ function HealthcheckDisplay(props: { ); } -function SideMessageDisplay(props: { - isHealthcheckInProgress: boolean; - hasProblems: boolean; -}) { - if (props.isHealthcheckInProgress) { +function SideMessageDisplay(props: {children: React.ReactNode}) { + return {props.children}; +} + +function ResultMessage(props: {result: HealthcheckResult}) { + if (status === 'IN_PROGRESS') { + return

Doctor is running healthchecks...

; + } else if (hasProblems(props.result)) { return ( - - Doctor is running healthchecks... - - ); - } else if (props.hasProblems) { - return ( - - Doctor has discovered problems with your installation. - +

+ Doctor has discovered problems with your installation. Please click to + each item to get details. +

); } else { return ( - +

All good! Doctor has not discovered any issues with your installation. - +

); } } @@ -224,6 +225,7 @@ function hasNewProblems(result: HealthcheckResult) { export type State = { acknowledgeCheckboxVisible: boolean; acknowledgeOnClose?: boolean; + selectedCheckKey?: string; }; type Props = OwnProps & StateFromProps & DispatchFromProps; @@ -296,10 +298,20 @@ class DoctorSheet extends Component { helpUrl && shell.openExternal(helpUrl); } - async runHealthchecks() { + async runHealthchecks(): Promise { await runHealthchecks(this.props); } + getCheckMessage(checkKey: string): string { + for (const cat of Object.values(this.props.healthcheckReport.categories)) { + const check = Object.values(cat.checks).find(chk => chk.key === checkKey); + if (check) { + return check.result.message || ''; + } + } + return ''; + } + render() { return ( @@ -319,12 +331,17 @@ class DoctorSheet extends Component { {Object.values(category.checks).map(check => ( this.openHelpUrl(check.result.helpUrl) - : undefined + onClick={() => + this.setState({ + ...this.state, + selectedCheckKey: + this.state.selectedCheckKey === check.key + ? undefined + : check.key, + }) } /> ))} @@ -344,12 +361,16 @@ class DoctorSheet extends Component { - + + + {this.state.selectedCheckKey && ( +

{this.getCheckMessage(this.state.selectedCheckKey)}

+ )} + {!this.state.selectedCheckKey && ( + + )} + + diff --git a/src/reducers/__tests__/healthchecks.node.tsx b/src/reducers/__tests__/healthchecks.node.tsx index 2541aa3b1..ce8c0d685 100644 --- a/src/reducers/__tests__/healthchecks.node.tsx +++ b/src/reducers/__tests__/healthchecks.node.tsx @@ -14,8 +14,7 @@ import { updateHealthcheckResult, acknowledgeProblems, } from '../healthchecks'; -import {Healthchecks} from 'flipper-doctor'; -import {EnvironmentInfo} from 'flipper-doctor/lib/environmentInfo'; +import {Healthchecks, EnvironmentInfo} from 'flipper-doctor'; const HEALTHCHECKS: Healthchecks = { ios: { @@ -27,7 +26,7 @@ const HEALTHCHECKS: Healthchecks = { key: 'ios.sdk', label: 'SDK Installed', run: async (_env: EnvironmentInfo) => { - return {hasProblem: false}; + return {hasProblem: false, message: ''}; }, }, ], @@ -41,7 +40,7 @@ const HEALTHCHECKS: Healthchecks = { key: 'android.sdk', label: 'SDK Installed', run: async (_env: EnvironmentInfo) => { - return {hasProblem: true}; + return {hasProblem: true, message: 'Error'}; }, }, ], @@ -55,7 +54,7 @@ const HEALTHCHECKS: Healthchecks = { key: 'common.openssl', label: 'OpenSSL Istalled', run: async (_env: EnvironmentInfo) => { - return {hasProblem: false}; + return {hasProblem: false, message: ''}; }, }, ], diff --git a/src/reducers/healthchecks.tsx b/src/reducers/healthchecks.tsx index f305d90ab..b32fb0e4f 100644 --- a/src/reducers/healthchecks.tsx +++ b/src/reducers/healthchecks.tsx @@ -60,7 +60,6 @@ export type HealthcheckResult = { status: HealthcheckStatus; isAcknowledged?: boolean; message?: string; - helpUrl?: string; }; export type HealthcheckReportItem = { diff --git a/src/utils/runHealthchecks.tsx b/src/utils/runHealthchecks.tsx index d404b7769..9cae8f3ea 100644 --- a/src/utils/runHealthchecks.tsx +++ b/src/utils/runHealthchecks.tsx @@ -66,14 +66,14 @@ async function launchHealthchecks(options: HealthcheckOptions): Promise { checkResult.hasProblem && h.isRequired ? { status: 'FAILED', - helpUrl: checkResult.helpUrl, + message: checkResult.message, } : checkResult.hasProblem && !h.isRequired ? { status: 'WARNING', - helpUrl: checkResult.helpUrl, + message: checkResult.message, } - : {status: 'SUCCESS'}; + : {status: 'SUCCESS', message: checkResult.message}; options.updateHealthcheckResult(categoryKey, h.key, result); } } diff --git a/yarn.lock b/yarn.lock index 4f17e6ad1..839741ffe 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4192,10 +4192,10 @@ flatted@^2.0.0: resolved "https://registry.yarnpkg.com/flatted/-/flatted-2.0.1.tgz#69e57caa8f0eacbc281d2e2cb458d46fdb449e08" integrity sha512-a1hQMktqW9Nmqr5aktAux3JMNqaucxGcjtjWnZLHX7yyPCmlSV3M54nGYbqT8K+0GhF3NBgmJCc3ma+WOgX8Jg== -flipper-doctor@^0.6.1: - version "0.6.1" - resolved "https://registry.yarnpkg.com/flipper-doctor/-/flipper-doctor-0.6.1.tgz#7a10cbe655293332c509d3ca37611bc32ee5d514" - integrity sha512-XRN5LqTK9J+2K5ixPwEODHYRyfL3hs9qDsKzcPoecoHP2DxPBOJNm2d2+J03lqFMedYE63a6+WhWUXOQoBRyEQ== +flipper-doctor@^0.7.0: + version "0.7.0" + resolved "https://registry.yarnpkg.com/flipper-doctor/-/flipper-doctor-0.7.0.tgz#642aca4004add6e94e29fa69e5ea58b5ab7b724a" + integrity sha512-cdT/nXiRkJH3Y2HYr3rCZyjOgk/+hLry4QYxp7gvKhKA6Nvy4SiPgSCae2aBTdFSe1KoQeqD3mFURrkVlZcsWg== dependencies: "@types/node" "^12.12.12" envinfo "^7.4.0"