Plamen5kov/rebuild aar #3398

Plamen5kov · 2018-02-27T12:30:08Z

Don't merge before: telerik/mobile-cli-lib#1055
this PR is based on branch: kddimitrov/nsconfig-app-folder

problem
the CLI doesn't build aar files for the native plugins

solution
build aar when:

there are AndroidManifest.xml or res folder in plugins' platforms/android/ folder
--syncAllFiles flag is passed (and a change is made in plugins' platforms/android/` folder

related issues
NativeScript/android#890
NativeScript/nativescript-plugin-seed#65

Fatme · 2018-02-27T13:16:58Z

lib/services/android-plugin-build-service.ts

+			);
+		}
+
+		return this.getDirectories(source).filter(isAndroidSourceDirectory);


Consider using the following approach

const directories = ["res", "java", "assets", "jniLibs"]; return this.$fs.enumerateFilesInDirectorySync(source, (file, stat) => stat.isDirectory() && _.includes(file, directoties));

Fatme · 2018-02-27T13:39:42Z

lib/services/android-plugin-build-service.ts

+		return this.getDirectories(source).filter(isAndroidSourceDirectory);
+	}
+
+	private getManifest(platformsDir: string) {


Can you use platformsData.getPlatformData().configurationFilePath?

this is the plugin's platforms dir, not the project

Fatme · 2018-02-27T13:40:39Z

lib/services/android-plugin-build-service.ts

+		return this.$fs.readDirectory(source).map(name => path.join(source, name));
+	}
+
+	private createDirIfDoesntExist(dirPath: string, { isRelativeToScript = false } = {}) {


Can you use this.$fs.ensureDirectoryExists(dirPath)?

rosen-vladimirov · 2018-02-27T13:45:30Z

@Plamen5kov can you please rebase the branch correctly, so it will be easier to check the new commits. Also currently it conflicts and cannot be merged.

Plamen5kov · 2018-02-27T14:36:41Z

@rosen-vladimirov I did

Fatme · 2018-02-28T18:53:49Z

lib/services/android-plugin-build-service.ts

+	private getAndroidSourceDirectories(source: string): Array<string> {
+		const directories = ["res", "java", "assets", "jniLibs"];
+		const resultArr: Array<string> = [];
+		this.$fs.enumerateFilesInDirectorySync(source, (file, stat) => {


return this.$fs.enumerateFilesInDirectorySync(source, (file, stat) => stat.isDirectory() && _.some(directories, (element) => file.endsWith(element)))

doesn't work, returns true or false i need the array

rosen-vladimirov

gradle-plugin is an external tool, so it should be placed in a separate location, not in services. A common place for such tools is a vendor directory at the root of the project. You can place the gradle-plugin there. Also note that using such tools directly in the package requires adding their LICENSE file as well, so can you please add it as well.

rosen-vladimirov · 2018-03-01T00:51:42Z

lib/services/android-plugin-build-service.ts

+import * as path from "path";
+import * as shell from "shelljs";
+
+const xml2js = require("xml2js");


You can add @types/xml2js as devDependency and use import here;

import { Builder, parseString } from "xml2js";

rosen-vladimirov · 2018-03-01T00:53:12Z

lib/services/android-plugin-build-service.ts

+		return resultArr;
+	}
+
+	private getManifest(platformsDir: string) {


this method needs return type

rosen-vladimirov · 2018-03-01T00:54:51Z

lib/services/android-plugin-build-service.ts

+		return newManifestContent;
+	}
+
+	private createManifestContent(packageName: string) {


this method needs return type

rosen-vladimirov · 2018-03-01T00:55:01Z

lib/services/android-plugin-build-service.ts

+		return newManifestContent;
+	}
+
+	private async getXml(stringContent: string) {


this method needs return type

rosen-vladimirov · 2018-03-01T00:58:08Z

lib/services/android-plugin-build-service.ts

+			if (manifestFilePath) {
+				let androidManifestContent;
+				try {
+					androidManifestContent = this.$fs.readFile(manifestFilePath).toString();


you can use this.$fs.readText here. This way you'll not have to use toString()

rosen-vladimirov · 2018-03-01T01:06:44Z

lib/services/android-plugin-build-service.ts

+		}
+	}
+
+	private validateOptions(options: IBuildOptions) {


this method needs return type

rosen-vladimirov · 2018-03-01T01:12:02Z

lib/services/android-project-service.ts

 		}

 		// Do nothing, the Android Gradle script will configure itself based on the input dependencies.json
 	}

+	public async checkIfPluginsNeedBuild(projectData: IProjectData): Promise<Array<any>> {
+		const detectedPlugins: Array<any> = [];


you can introduce an interface in this class or at least inline the properties:

public async checkIfPluginsNeedBuild(projectData: IProjectData): Promise<Array<{ platformsAndroidDirPath: string, pluginName: string }>> { const detectedPlugins: Array<{ platformsAndroidDirPath: string, pluginName: string }> = [];

rosen-vladimirov · 2018-03-01T01:14:19Z

lib/services/android-project-service.ts

+		return detectedPlugins;
+	}
+
+	private isAllowedFile(item: string) {


this method needs return type

rosen-vladimirov · 2018-03-01T01:18:50Z

lib/services/android-project-service.ts

+							const currentItemStat = this.$fs.getFsStats(item);
+							if (currentItemStat.mtime > aarStat.mtime) {
+								detectedPlugins.push({
+									platformsAndroidDirPath: platformsAndroidDirPath,


you can use the short syntax here:

detectedPlugins.push({ platformsAndroidDirPath, pluginName });

rosen-vladimirov · 2018-03-01T01:19:04Z

lib/services/android-project-service.ts

+						});
+					} else if (nativeFiles.length > 0) {
+						detectedPlugins.push({
+							platformsAndroidDirPath: platformsAndroidDirPath,


same as above:
you can use the short syntax here:

detectedPlugins.push({ platformsAndroidDirPath, pluginName });

Fatme · 2018-03-01T05:04:52Z

lib/services/android-plugin-build-service.ts

+import * as path from "path";
+import * as shell from "shelljs";
+
+const xml2js = require("xml2js");


I suggest to add a separate service that is wrapper for xml2js package and to inject the new service in constructor. (It's not a merge stopper)

IMO we can hold off on that because xml2js is used only in AndroidPluginBuildService and there's no publicly exposed API, that can be used when the CLI is required.

Fatme · 2018-03-01T05:14:57Z

lib/services/android-plugin-build-service.ts

+		return resultArr;
+	}
+
+	private getManifest(platformsDir: string) {


Why we need to traverse the platforms dir to find the manifest file. Shoudn't it be in platforms\android folder. If this is the case, should'n we just return

path.join(platfromsDir, "android", AndroidPluginBuildService.ANDROID_MANIFEST_XML);

The plugin's platforms/android folder does not necessarily have an AndroidManifest.xml file

The migration tool started initially with the thought that it could also support flavors of the android libraries, in which cases there could be more than one manifests. Since I later realized that is a somewhat unrealistic expectation (that plugins would have additional flavors), I dropped the idea, but didn't modify the code as it kept working.

Yes, you can indeed use path.join followed by a check if fs.existsSync()

Fatme · 2018-03-01T05:17:03Z

lib/services/android-plugin-build-service.ts

+
+const xml2js = require("xml2js");
+
+export class AndroidPluginBuildService implements IAndroidPluginBuildService {


Consider to add some unit tests for this service.

Plamen5kov · 2018-03-05T08:33:22Z

Added unit tests and rebased on master branch.
ping @rosen-vladimirov

rosen-vladimirov · 2018-03-05T19:17:50Z

lib/services/android-plugin-build-service.ts

+		private $hostInfo: IHostInfo,
+		private $androidToolsInfo: IAndroidToolsInfo) { }
+
+	private static ANDROID_MANIFEST_XML = "AndroidManifest.xml";


You can use the MANIFEST_FILE_NAME constant instead

rosen-vladimirov · 2018-03-05T19:19:26Z

lib/services/android-plugin-build-service.ts

+		private $androidToolsInfo: IAndroidToolsInfo) { }
+
+	private static ANDROID_MANIFEST_XML = "AndroidManifest.xml";
+	private static INCLUDE_GRADLE = "include.gradle";


as the include.gradle string is already used in android-project-service.ts, maybe it is better to extract a constant in constants file and use it here and in AndroidProjectService

rosen-vladimirov · 2018-03-05T19:21:02Z

lib/services/android-plugin-build-service.ts

+	private static ANDROID_PLUGIN_GRADLE_TEMPLATE = "../../vendor/gradle-plugin";
+
+	private getAndroidSourceDirectories(source: string): Array<string> {
+		const directories = ["res", "java", "assets", "jniLibs"];


you can use RESOURCES_DIR and ASSETS_DIR from constants

rosen-vladimirov · 2018-03-05T19:22:14Z

lib/services/android-plugin-build-service.ts

+
+	private async updateManifestContent(oldManifestContent: string, defaultPackageName: string): Promise<string> {
+		const content = oldManifestContent;
+		let newManifestContent;


There's no need to declare the variable here. Just declare it where you will use it.

rosen-vladimirov · 2018-03-05T19:23:13Z

lib/services/android-plugin-build-service.ts

+	}
+
+	private async updateManifestContent(oldManifestContent: string, defaultPackageName: string): Promise<string> {
+		const content = oldManifestContent;


why do you need this variable?

rosen-vladimirov · 2018-03-05T20:37:30Z

package.json

@@ -79,6 +79,7 @@
    "winreg": "0.0.17",
    "ws": "2.2.0",
    "xcode": "https://github.com/NativeScript/node-xcode/archive/1.4.0.tar.gz",
+    "xml2js": "^0.4.19",


Please use strict versions of both new dependencies.

rosen-vladimirov · 2018-03-05T20:38:33Z

test/ios-entitlements-service.ts

@@ -47,7 +47,7 @@ describe("IOSEntitlements Service Tests", () => {
 		injector = createTestInjector();

 		platformsData = injector.resolve("platformsData");
-		projectData = injector.resolve("projectData");
+		projectData = $injector.resolve<IProjectData>("projectData");


this way you'll not use the local injector variable, but the global $injector one. It is a bad practice to use the global variable in the tests. Can you elaborate why you needed to change this?

probably a copy-paste error, but thanks for the explanation

rosen-vladimirov · 2018-03-05T20:39:15Z

test/plugin-prepare.ts

@@ -32,20 +32,6 @@ describe("Plugin preparation", () => {
 		assert.deepEqual({}, pluginPrepare.preparedDependencies);
 	});

-	it("skips prepare if every plugin prepared", async () => {


does it mean that now we'll never skip prepare even if all plugins are okay?

no, if the JS part of the plugins is prepared we will skill prepare, but we will always check for changes in the native part of a plugin for changes that need rebuilding.

rosen-vladimirov · 2018-03-05T20:41:14Z

test/services/android-plugin-build-service.ts

+			await androidBuildPluginService.buildAar(config);
+			const hasGeneratedAar = fs.exists(path.join(tempFolder, expectedAarName));
+
+			assert.equal(hasGeneratedAar, true);


assert.isTrue(hasGeneratedAar);

rosen-vladimirov · 2018-03-05T20:44:49Z

vendor/gradle-plugin/build.gradle

+
+allprojects {
+    repositories {
+        jcenter()


shouldn't we add the google() repository as well?

no need, because we decide what dependencies to use, and we don't have any dependencies that would use google()

Plamen5kov · 2018-03-07T09:46:01Z

run ci

rosen-vladimirov · 2018-03-07T11:22:11Z

lib/commands/plugin/build-plugin.ts

+		private $fs: IFileSystem,
+		private $options: IOptions) {
+
+		if (this.$options.path) {


you can shorten the whole syntax:

this.pluginProjectPath = path.resolve(this.$options.path || ".");

rosen-vladimirov · 2018-03-07T11:25:45Z

lib/commands/plugin/build-plugin.ts

+
+		const tempAndroidProject = path.join(platformsAndroidPath, "android-project");
+
+		const options: IBuildOptions = {


you can use short syntax here:

const options: IBuildOptions = { aarOutputDir: platformsAndroidPath, platformsAndroidDirPath, pluginName, tempPluginDirPath: tempAndroidProject };

But this is just a preference, so definitely not a merge stopper

rosen-vladimirov · 2018-03-07T11:26:47Z

lib/commands/plugin/build-plugin.ts

+	}
+
+	public async canExecute(args: string[]): Promise<boolean> {
+		if (!this.$fs.exists(path.join(this.pluginProjectPath, "platforms", "android"))) {


platforms -> constants.PLATFORMS_DIR_NAME

rosen-vladimirov · 2018-03-07T11:29:47Z

lib/definitions/android-plugin-migrator.d.ts

@@ -0,0 +1,15 @@
+
+interface IBuildOptions extends IAndroidBuildOptions{


This interface does not add anything to the IAndroidBuildOptions. What is the purpose of having it?

In one of your previous comments, you mentioned the properties are android specific so I moved them to an android specific interface.

rosen-vladimirov · 2018-03-07T12:09:28Z

lib/services/android-project-service.ts

+		return buildOptions;
+	}
+
+	public getBuildOptions(configurationFilePath?: string): Array<string> {


Thanks for the explanation. As I cannot think of a better name, lets keep the current one.

rosen-vladimirov · 2018-03-07T12:12:57Z

lib/services/livesync/livesync-service.ts

-						filesToSync.push(filePath);
-					} else if (event === "unlink" || event === "unlinkDir") {
-						filesToRemove.push(filePath);
+					if (!isAllowedFinalFile(filePath)) {


Can I place .aar file in App_Resources? In case yes, what should happen here - we'll not include the files in filesToSync and... we should rebuild the application? Will this happen?

We haven't talked about placing aar files in App_Resources so I wouldn't know what would happen.

@rosen-vladimirov App_Resources/Android/libs (where aars can be placed) is watched separately and should force a rebuild

@rosen-vladimirov I checked it out and when you have an example.aar file in the App_Resources and the tns run android or tns run android --syncAllFiles is running and the example.aar is changed, the changes are being ignored.
When the command is stopped and ran again, the example.aar forces a rebuild.

rosen-vladimirov

After green build

handle --syncAllFiles in node_modules pass the same gradle options to the buildAar plugin and the native project

…lFiles flag * removed invalid test * move gradlew plugin to vendor folder and add licese file * add unit test for android build plugin service

…d aars docs(plugin build command): add docs for the new command

miroslavaivanova · 2018-03-08T07:14:21Z

run ci

Plamen5kov mentioned this pull request Feb 27, 2018

refactor: update helpers telerik/mobile-cli-lib#1055

Merged

Plamen5kov requested review from Mitko-Kerezov and petekanev February 27, 2018 12:30

KristianDD force-pushed the kddimitrov/nsconfig-app-folder branch from ae4f213 to b96a8cc Compare February 27, 2018 13:15

Fatme reviewed Feb 27, 2018

View reviewed changes

Plamen5kov force-pushed the plamen5kov/rebuild-aar branch from e177efe to 10d60af Compare February 27, 2018 14:27

Plamen5kov self-assigned this Feb 28, 2018

Plamen5kov force-pushed the plamen5kov/rebuild-aar branch from 10d60af to d59dbe7 Compare February 28, 2018 11:45

KristianDD force-pushed the kddimitrov/nsconfig-app-folder branch from b96a8cc to 2b5ef94 Compare February 28, 2018 12:01

Plamen5kov removed request for Mitko-Kerezov and petekanev February 28, 2018 15:44

Fatme reviewed Feb 28, 2018

View reviewed changes

rosen-vladimirov suggested changes Mar 1, 2018

View reviewed changes

Fatme reviewed Mar 1, 2018

View reviewed changes

Plamen5kov force-pushed the plamen5kov/rebuild-aar branch 2 times, most recently from e620e83 to 9e44a11 Compare March 2, 2018 14:35

Plamen5kov changed the base branch from kddimitrov/nsconfig-app-folder to master March 5, 2018 07:33

Plamen5kov force-pushed the plamen5kov/rebuild-aar branch 3 times, most recently from 4ecee82 to 149bdb2 Compare March 5, 2018 08:31

Plamen5kov added this to the 4.0.0 milestone Mar 5, 2018

Plamen5kov added the in progress label Mar 5, 2018

rosen-vladimirov suggested changes Mar 5, 2018

View reviewed changes

rosen-vladimirov reviewed Mar 7, 2018

View reviewed changes

Plamen5kov force-pushed the plamen5kov/rebuild-aar branch from ae61e5a to e5cf9a3 Compare March 7, 2018 13:08

Plamen5kov force-pushed the plamen5kov/rebuild-aar branch 3 times, most recently from 6d0786c to 7e192f9 Compare March 7, 2018 15:20

rosen-vladimirov approved these changes Mar 7, 2018

View reviewed changes

Plamen5kov and others added 11 commits March 7, 2018 18:08

refactor: rebuild aar files in nodemodules if necessary

6e3650a

handle --syncAllFiles in node_modules pass the same gradle options to the buildAar plugin and the native project

refactor: when building plugins take CLI generated options

c8d021d

refactor: make the build plugin service not use platformData

f13f6f9

fix: respect native folders in plugin

0b49a64

refactor: the CLI builds aar files if necessary even with no --syncAl…

04f9737

…lFiles flag * removed invalid test * move gradlew plugin to vendor folder and add licese file * add unit test for android build plugin service

refactor: update after review

180fe32

feat(command): introduce tns plugin build command which builds androi…

34323a3

…d aars docs(plugin build command): add docs for the new command

chore: update node to LTS

2a15071

chore: update after review + submodule

d9dd119

fix: update after rebase

22743ef

chore: trying to fix travis

1cda138

Plamen5kov force-pushed the plamen5kov/rebuild-aar branch from 7e192f9 to 1cda138 Compare March 7, 2018 16:08

Plamen5kov merged commit 283b31c into master Mar 8, 2018

Plamen5kov deleted the plamen5kov/rebuild-aar branch March 8, 2018 08:17


		const xml2js = require("xml2js");

		export class AndroidPluginBuildService implements IAndroidPluginBuildService {


		const tempAndroidProject = path.join(platformsAndroidPath, "android-project");

		const options: IBuildOptions = {

		@@ -0,0 +1,15 @@

		interface IBuildOptions extends IAndroidBuildOptions{

Uh oh!

Plamen5kov/rebuild aar #3398

Plamen5kov/rebuild aar #3398

Uh oh!

Conversation

Plamen5kov commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rosen-vladimirov commented Feb 27, 2018

Uh oh!

Plamen5kov commented Feb 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rosen-vladimirov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Plamen5kov Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Plamen5kov commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Plamen5kov commented Feb 27, 2018 •

edited

Loading

Plamen5kov Mar 2, 2018 •

edited

Loading

Plamen5kov commented Mar 5, 2018 •

edited

Loading