This repository was archived by the owner on Aug 7, 2021. It is now read-only.

Buhov/local snapshot improvements #194

Merged

ivanbuhov merged 6 commits into master from buhov/local-snapshot-improvements

Jun 20, 2017

Merged

Buhov/local snapshot improvements #194

ivanbuhov merged 6 commits into master from buhov/local-snapshot-improvements

Jun 20, 2017

Conversation

ivanbuhov

Copy link

Contributor

@ivanbuhov ivanbuhov commented Jun 19, 2017 •

edited

Loading

mksnapshot tools are now downloaded on demand before running them
Introduce __snapshot and __snapshotEnabled global variables:
__snapshotEnabled -> has a value of true if snapshot is loaded in the app
__snapshot -> has a value of true when the executed code is in snapshot context and false when the code is executed on device/emulator
Clean platforms/android/build folder on snapshot generation in order to trigger full rebuild of the .apk files
Install include.gradle file even when blobs are used in order to exclude the snapshotted file from the apk.
Respect ANDROID_NDK_HOME env variable in case androidNdkPath is not specified

ivanbuhov added 4 commits

June 19, 2017 13:46

@ivanbuhov


 Remove mksnapshot tools from the distributed package and downlaod the...

404275c

...m on demand before snapshot generation

@ivanbuhov


 Rename __snapshot to __snapshotEnabled. Make __snapshot flag to be eq...

f14adf5

...ual to true only when in snapshot context

@ivanbuhov


 Make sure the apk is fully rebuild when snapshots are installed

39f08e4

@ivanbuhov


 Exclude the snapshotted bundle from the apk even when blobs are used

07483c0

@ivanbuhov ivanbuhov requested a review from sis0k0

June 19, 2017 10:56

sis0k0

sis0k0 reviewed

Jun 19, 2017

View reviewed changes

snapshot/android/project-snapshot-generator.js Outdated

this.generateTnsJavaClassesFile({ output: tnsJavaClassesDestination, options: generationOptions.tnsJavaClassesOptions });

}

var snapshotToolsPath = generationOptions.snapshotToolsPath ?

Copy link

Contributor

Choose a reason for hiding this comment

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

style: let's make it an arrow func instead of using ternary operator 2 times

snapshot/android/project-snapshot-generator.js Outdated

const generator = new SnapshotGenerator({ buildPath: this.getBuildPath() });

const generatorBuildPath = generator.generate({

return generator.generate({

snapshotToolsPath: snapshotToolsPath,

Copy link

Contributor

Choose a reason for hiding this comment

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

style: just snapshotToolsPath is more readable

plugins/NativeScriptSnapshotPlugin.js Outdated

options: options.tnsJavaClassesOptions

});

// Run the snapshot tool when the packing is done

Copy link

Contributor

Choose a reason for hiding this comment

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

style: update the comment, too, or remove it

snapshot/android/snapshot-generator.js Outdated

}

module.exports = SnapshotGenerator;

SnapshotGenerator.MKSNAPSHOT_TOOLS_PATH = path.join(__dirname, "snapshot-generator-tools/mksnapshot");

Copy link

Contributor

Choose a reason for hiding this comment

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

style suggestion:

const { join, dirname } = require("path");
...
const getAbsolutePath = path => join(__dirname, path);

Copy link

Contributor Author

@ivanbuhov ivanbuhov Jun 19, 2017

Choose a reason for hiding this comment

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

Why do you think we need getAbsolutePath function? It is as verbose as using join(__dirname, path) but I find join(__dirname, path) to be more clear.

snapshot/android/snapshot-generator.js Outdated

}

SnapshotGenerator.prototype.getMksnapshotToolsDirOrThrow = function(v8Version) {

var snapshotToolsDownloads = {};

Copy link

Contributor

Choose a reason for hiding this comment

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

const

snapshot/android/snapshot-generator.js Outdated

});

}

var downloadUrl = "https://raw.githubusercontent.com/NativeScript/mksnapshot-tools/production/" + mksnapshotToolRelativePath;

Copy link

Contributor

Choose a reason for hiding this comment

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

const and maybe we should extract the url to some global var

snapshot/android/snapshot-generator.js Outdated

throw new Error("Can't find mksnapshot tool for " + arch + " at path " + currentArchMksnapshotToolPath);

}

var androidArch = this.convertToAndroidArchName(arch);

Copy link

Contributor

Choose a reason for hiding this comment

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

const

snapshot/android/snapshot-generator.js Outdated

var currentArchBlobOutputPath = path.join(this.buildPath, "snapshots/blobs", androidArch);

shelljs.mkdir("-p", currentArchBlobOutputPath);

var stdErrorStream = fs.openSync(mksnapshotStdErrPath, 'w');

child_process.execSync(currentArchMksnapshotToolPath + " " + inputFile + " --startup_blob " + path.join(currentArchBlobOutputPath, "TNSSnapshot.blob") + " --profile_deserialization", {encoding: "utf8", stdio: [process.stdin, process.stdout, stdErrorStream]});

Copy link

Contributor

@sis0k0 sis0k0 Jun 19, 2017

Choose a reason for hiding this comment

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

extract the command

snapshot/android/snapshot-generator.js Outdated

var currentArchBlobOutputPath = path.join(this.buildPath, "snapshots/blobs", androidArch);

shelljs.mkdir("-p", currentArchBlobOutputPath);

var stdErrorStream = fs.openSync(mksnapshotStdErrPath, 'w');

Copy link

Contributor

@sis0k0 sis0k0 Jun 19, 2017

Choose a reason for hiding this comment

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

erm, let's use exec and not pipe the error stream to a file

snapshot/android/snapshot-generator.js Outdated

var currentArchSrcOutputPath = path.join(this.buildPath, "snapshots/src", androidArch);

shelljs.mkdir("-p", currentArchSrcOutputPath);

shellJsExecuteInDir(currentArchBlobOutputPath, function(){

shelljs.exec("xxd -i TNSSnapshot.blob > " + path.join(currentArchSrcOutputPath, "TNSSnapshot.c"));

Copy link

Contributor

@sis0k0 sis0k0 Jun 19, 2017

Choose a reason for hiding this comment

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

let's extract TNSSnapshot (the filename) to a const somewhere

ivanbuhov added 2 commits

June 19, 2017 19:56

@ivanbuhov


 Respect ANDROID_NDK_HOME env variable in case androidNdkPath is not s...

20d496a

...pecified

@ivanbuhov


 Refactoring

d18fd66

@ivanbuhov ivanbuhov force-pushed the buhov/local-snapshot-improvements branch from 24778ab to d18fd66 Compare

June 20, 2017 13:08

sis0k0

sis0k0 approved these changes

Jun 20, 2017

View reviewed changes

@sis0k0

Copy link

Contributor

sis0k0 commented Jun 20, 2017

groceries

@ivanbuhov ivanbuhov merged commit dc897ea into master

Jun 20, 2017

@ivanbuhov ivanbuhov deleted the buhov/local-snapshot-improvements branch

June 20, 2017 16:14

Labels

None yet

2 participants

@ivanbuhov @sis0k0

Uh oh!

Buhov/local snapshot improvements #194

Buhov/local snapshot improvements #194

Uh oh!

Conversation

@ivanbuhov ivanbuhov commented Jun 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

@sis0k0 sis0k0 Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

@sis0k0 sis0k0 Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

@sis0k0 sis0k0 Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

@sis0k0 sis0k0 Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

@ivanbuhov ivanbuhov Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

@sis0k0 sis0k0 Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

@sis0k0 sis0k0 Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

@sis0k0 sis0k0 Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

@sis0k0 sis0k0 Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

@sis0k0 sis0k0 Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

@sis0k0 sis0k0 Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

sis0k0 commented Jun 20, 2017

Uh oh!

Uh oh!

@ivanbuhov ivanbuhov commented Jun 19, 2017 •

edited

Loading