Fix getting info for Android tools #2610

rosen-vladimirov · 2017-03-14T22:10:07Z

Due to changes in Android SDK, we have to update the checks in CLI. While gathering system information, we check the android executable, which is no longer returning correct results.

We use the android executable to find information about installed Android SDKs and to construct correct paths based on ANDROID_HOME. Fix this by listing directories inside ANDROID_HOME and find the information about installed SDKs from there.
Fix messages pointing to android executable to point to sdkmanager (in case it exists).

In order to fix this, we rely on the emulator executable, which is the real thing we need as it is the one that allows us to work with Android Emulators (this is changed in mobile-cli-lib).
Fix sys-info checks and get correct path to emulator according to latest changes.

PR in mobile-cli-lib telerik/mobile-cli-lib#906

Due to changes in Android SDK, we have to update the checks in CLI. While gathering system information, we check the android executable, which is no longer returning correct results. We use the android executable to find information about installed Android SDKs and to construct correct paths based on ANDROID_HOME. Fix this by listing directories inside ANDROID_HOME and find the information about installed SDKs from there. Fix messages pointing to `android` executable to point to `sdkmanager` (in case it exists). In order to fix this, we rely on the emulator executable, which is the real thing we need as it is the one that allows us to work with Android Emulators (this is changed in mobile-cli-lib). Fix sys-info checks and get correct path to emulator according to latest changes.

rosen-vladimirov · 2017-03-14T22:52:41Z

lib/android-tools-info.ts

@@ -186,6 +149,28 @@ export class AndroidToolsInfo implements IAndroidToolsInfo {
 		return null;
 	}

+	private _cachedAndroidHomeValidationResult: boolean = null;


use @cache decorator here instead of variable

rosen-vladimirov · 2017-03-14T22:53:26Z

lib/android-tools-info.ts

@@ -318,44 +303,23 @@ export class AndroidToolsInfo implements IAndroidToolsInfo {
 	private async getInstalledTargets(): Promise<string[]> {


this method no longer needs to return Promise - its operations are sync now

As one of the methods in androidSysInfo no longer needs to be async, after removing its async signature, this lead to avalanche of methods that no longer needs to be async. So fix all of them.

petekanev · 2017-03-15T05:09:42Z

lib/services/android-project-service.ts

@@ -204,10 +203,11 @@ export class AndroidProjectService extends projectServiceBaseLib.PlatformProject
 		}
 	}

-	public async interpolateConfigurationFile(projectData: IProjectData, platformSpecificData: IPlatformSpecificData): Promise<void> {
+// TODO: Check if we can make this method sync.


The method is already sync. Delete the comment?

petekanev

This appears to be exactly what we need to not depend on any particular tools sdk version. Well done!

yyosifov · 2017-03-15T08:15:42Z

lib/android-tools-info.ts

@@ -186,6 +139,51 @@ export class AndroidToolsInfo implements IAndroidToolsInfo {
 		return null;
 	}

+	@cache()
+	public validateAndroidHomeEnvVariable(options?: { showWarningsAsErrors: boolean }): boolean {


Do we need tests for this functionality? Will the cache work as expected here, don't we need to somehow include the "androidHome" to the cacheKey?

The cache decorator has tests here
The androidHome will not be changed between executions of the method (I've also change androidHome to be only getter).

Regarding tests for the whole androidToolsInfo - we'll move it to nativescript-doctor package very soon and we'll add tests there.

yyosifov · 2017-03-15T08:17:58Z

lib/android-tools-info.ts

+
+	@cache()
+	private getPathToSdkManagementTool(): string {
+		const sdkmanagerName = "sdkmanager";


Although I understand the "sdkmanager" is lowercase, the variables containing "Sdkmanager" look little less readable to me. (sdkmanagerName, pahtToSdkmanager)

yyosifov · 2017-03-15T08:19:31Z

lib/android-tools-info.ts

+
+		const isAndroidHomeValid = this.validateAndroidHomeEnvVariable();
+
+		if (isAndroidHomeValid) {


if it's not valid, we'll return "sdkmanager" as result. Shouldn't we throw an error in case the AndroidHome is invalid?

This method is used just to get the path that we'll show in the additionalMessage. The idea is that if something is not configured properly, tns doctor command to show some suggestions how to fix the issue.
In case you execute any other command, CLI will fail immediately in case the ANDROID_HOME is not set. The default is sdkmanager as when the ANDROID_HOME is not set, most probably the user does not have any Android SDK. After installing it, he'll have the new SDK, which requires usage of sdkmanager not android.

yyosifov · 2017-03-15T08:22:40Z

lib/android-tools-info.ts

-	private async getAppCompatRange(): Promise<string> {
-		let compileSdkVersion = await this.getCompileSdk();
+	private getAppCompatRange(): string {
+		let compileSdkVersion = this.getCompileSdk();


why not rename the "getCompileSdk()" method as "getCompileSdkVersion"? It returns a number and it's assigned to a variable sdkVersion, therefore I think it would be more correctly named this way

TsvetanMilanov · 2017-03-15T09:37:45Z

lib/android-tools-info.ts

 		return _.findLast(AndroidToolsInfo.SUPPORTED_TARGETS.sort(), supportedTarget => _.includes(installedTargets, supportedTarget));
 	}

 	private parseAndroidSdkString(androidSdkString: string): number {
 		return parseInt(androidSdkString.replace(`${AndroidToolsInfo.ANDROID_TARGET_PREFIX}-`, ""));
 	}

-	private async getInstalledTargets(): Promise<string[]> {
+	private getInstalledTargets(): string[] {
 		if (!this.installedTargetsCache) {
 			try {


I think we don't need this try/catch. You can move the logger.trace in the if instead of throw new Errorand do the rest in the else.

The command `tns emulate android --available-devices` should list the available Android Virtual Devices on the current machine. It uses `android list avd` command which is no longer available. The alternative is to use the new `$ANDROID_HOME/tools/bin/avdmanager` executable or use the way we already have in the mobile-cli-lib - parse the `.ini` files of each device. Use the methods from `androidEmulatorServices` so the code will work with both old and new SDKs.

rosen-vladimirov added os: android bug labels Mar 14, 2017

rosen-vladimirov self-assigned this Mar 14, 2017

rosen-vladimirov force-pushed the vladimirov/fix-android-builds branch from d8e2c83 to 54cff3a Compare March 14, 2017 22:13

rosen-vladimirov commented Mar 14, 2017

View reviewed changes

Make androidSysInfo calls sync

95f3997

As one of the methods in androidSysInfo no longer needs to be async, after removing its async signature, this lead to avalanche of methods that no longer needs to be async. So fix all of them.

rosen-vladimirov force-pushed the vladimirov/fix-android-builds branch from 344ceb7 to 95f3997 Compare March 15, 2017 00:18

rosen-vladimirov requested review from Plamen5kov, dtopuzov, TsvetanMilanov and petekanev March 15, 2017 00:32

petekanev reviewed Mar 15, 2017

View reviewed changes

petekanev approved these changes Mar 15, 2017

View reviewed changes

yyosifov reviewed Mar 15, 2017

View reviewed changes

Plamen5kov approved these changes Mar 15, 2017

View reviewed changes

TsvetanMilanov reviewed Mar 15, 2017

View reviewed changes

rosen-vladimirov added 2 commits March 16, 2017 00:23

Code styling fixes

ed3cd28

rosen-vladimirov force-pushed the vladimirov/fix-android-builds branch from a286def to 18b91a8 Compare March 16, 2017 12:11

Fix getting installed Android SDKs

3dbd332

rosen-vladimirov force-pushed the vladimirov/fix-android-builds branch from 18b91a8 to 3dbd332 Compare March 16, 2017 12:22

dtopuzov approved these changes Mar 16, 2017

View reviewed changes

rosen-vladimirov merged commit 068d17c into master Mar 16, 2017

rosen-vladimirov deleted the vladimirov/fix-android-builds branch March 16, 2017 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix getting info for Android tools #2610

Fix getting info for Android tools #2610

rosen-vladimirov commented Mar 14, 2017 •

edited

Loading

rosen-vladimirov Mar 14, 2017

rosen-vladimirov Mar 14, 2017

petekanev Mar 15, 2017

petekanev left a comment

yyosifov Mar 15, 2017

rosen-vladimirov Mar 15, 2017

yyosifov Mar 15, 2017

yyosifov Mar 15, 2017

rosen-vladimirov Mar 15, 2017

yyosifov Mar 15, 2017 •

edited

Loading

TsvetanMilanov Mar 15, 2017

		@@ -318,44 +303,23 @@ export class AndroidToolsInfo implements IAndroidToolsInfo {
		private async getInstalledTargets(): Promise<string[]> {


		const isAndroidHomeValid = this.validateAndroidHomeEnvVariable();

		if (isAndroidHomeValid) {

Fix getting info for Android tools #2610

Fix getting info for Android tools #2610

Conversation

rosen-vladimirov commented Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petekanev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyosifov Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosen-vladimirov commented Mar 14, 2017 •

edited

Loading

yyosifov Mar 15, 2017 •

edited

Loading