Skip to content

No debug break by default implementation added for iOS and Android #1801

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 6, 2016
Merged

No debug break by default implementation added for iOS and Android #1801

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/man_pages/project/testing/debug-android.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
debug android
==========

All you need to do to start debugging your app is to execute `tns debug android`. The NativeScript CLI will build, deploy, start your app, start Chrome DevTools and attach the debugger.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be under the Usage|Synopsis part of the help (here -> https://github.com/NativeScript/nativescript-cli/pull/1801/files#diff-e7b2eaa70b7b2394cb03e78ab260308aR18 ). We print the table of usage first in all of the help content.

* You cannot debug if there are multiple devices available (emulators and/or real devices). You need to have started only one device or emulator.

Usage | Synopsis
---|---
Deploy on device, run the app and stop at the first breakpoint | `$ tns debug android --debug-brk [--device <Device ID>] [--debug-port <port>] [--timeout <timeout>]`
Expand Down
2 changes: 2 additions & 0 deletions docs/man_pages/project/testing/debug-ios.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
debug ios
==========
All you need to do to start debugging your app is to execute `tns debug ios`. The NativeScript CLI will build, deploy, start your app, start Chrome DevTools and attach the debugger.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we start Chrome DevTools for iOS?
Also this should be under the Usage|Synopsis part of the help. We print the table of usage first in all of the help content.

* You cannot debug if there are multiple devices available (emulators and/or real devices). You need to have started only one device or emulator.

Usage | Synopsis
---|---
Expand Down
57 changes: 31 additions & 26 deletions lib/services/android-debug-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,12 +106,7 @@ class AndroidDebugService implements IDebugService {
return (() => {
let packageFile = "";

if (!this.$options.debugBrk && !this.$options.start && !this.$options.getPort && !this.$options.stop) {
this.$logger.warn("Neither --debug-brk nor --start option was specified. Defaulting to --debug-brk.");
this.$options.debugBrk = true;
}

if (this.$options.debugBrk && !this.$options.emulator) {
if (!this.$options.start && !this.$options.emulator) {
let cachedDeviceOption = this.$options.forDevice;
this.$options.forDevice = true;
this.$platformService.buildPlatform(this.platform).wait();
Expand Down Expand Up @@ -141,6 +136,11 @@ class AndroidDebugService implements IDebugService {
this.detachDebugger(packageName).wait();
} else if (this.$options.debugBrk) {
this.startAppWithDebugger(packageFile, packageName).wait();
} else {
this.startAppWithDebugger(packageFile, packageName).wait();
//TODO: Find different way to make sure that the app is started.
sleep(500);
this.attachDebugger(device.deviceInfo.identifier, packageName).wait();
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe I'm totally missing something here, but shouldn't startAppWithDebugger do this... Based on it's name it looks like that's what it should do and we do not have to attach debugger again?
What's the case here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dunno, you tell me?

}
}).future<void>()();
}
Expand Down Expand Up @@ -197,33 +197,38 @@ class AndroidDebugService implements IDebugService {
// Arguments passed to executeShellCommand must be in array ([]), but it turned out adb shell "arg with intervals" still works correctly.
// As we need to redirect output of a command on the device, keep using only one argument.
// We could rewrite this with two calls - touch and rm -f , but -f flag is not available on old Android, so rm call will fail when file does not exist.
this.device.adb.executeShellCommand([`cat /dev/null > /data/local/tmp/${packageName}-debugbreak`]).wait();

if(this.$options.debugBrk) {
this.device.adb.executeShellCommand([`cat /dev/null > /data/local/tmp/${packageName}-debugbreak`]).wait();
}

this.device.applicationManager.stopApplication(packageName).wait();
this.device.applicationManager.startApplication(packageName).wait();

let waitText: string = `0 /data/local/tmp/${packageName}-debugbreak`;
let maxWait = 12;
let debugerStarted: boolean = false;
while (maxWait > 0 && !debugerStarted) {
let forwardsResult = this.device.adb.executeShellCommand(["ls", "-s", `/data/local/tmp/${packageName}-debugbreak`]).wait();
maxWait--;
debugerStarted = forwardsResult.indexOf(waitText) === -1;
if (!debugerStarted) {
sleep(500);
if(this.$options.debugBrk) {
let waitText: string = `0 /data/local/tmp/${packageName}-debugbreak`;
let maxWait = 12;
let debugerStarted: boolean = false;
while (maxWait > 0 && !debugerStarted) {
let forwardsResult = this.device.adb.executeShellCommand(["ls", "-s", `/data/local/tmp/${packageName}-debugbreak`]).wait();
maxWait--;
debugerStarted = forwardsResult.indexOf(waitText) === -1;
if (!debugerStarted) {
sleep(500);
}
}
}

if (debugerStarted) {
this.$logger.info("# NativeScript Debugger started #");
} else {
this.$logger.warn("# NativeScript Debugger did not start in time #");
}
if (debugerStarted) {
this.$logger.info("# NativeScript Debugger started #");
} else {
this.$logger.warn("# NativeScript Debugger did not start in time #");
}

if (this.$options.client) {
let localDebugPort = this.getForwardedLocalDebugPortForPackageName(this.device.deviceInfo.identifier, packageName).wait();
this.startDebuggerClient(localDebugPort).wait();
this.openDebuggerClient(AndroidDebugService.DEFAULT_NODE_INSPECTOR_URL + "?port=" + localDebugPort);
if (this.$options.client) {
let localDebugPort = this.getForwardedLocalDebugPortForPackageName(this.device.deviceInfo.identifier, packageName).wait();
this.startDebuggerClient(localDebugPort).wait();
this.openDebuggerClient(AndroidDebugService.DEFAULT_NODE_INSPECTOR_URL + "?port=" + localDebugPort);
}
}
}).future<void>()();
}
Expand Down
25 changes: 14 additions & 11 deletions lib/services/ios-debug-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,21 +43,23 @@ class IOSDebugService implements IDebugService {
this.$errors.failWithoutHelp("Expected exactly one of the --debug-brk or --start options.");
}

if(!this.$options.debugBrk && !this.$options.start) {
this.$logger.warn("Neither --debug-brk nor --start option was specified. Defaulting to --debug-brk.");
this.$options.debugBrk = true;
}

if (this.$options.emulator) {
if (this.$options.debugBrk) {
return this.emulatorDebugBrk();
return this.emulatorDebugBrk(true);
} else if (this.$options.start) {
return this.emulatorStart();
} else {
return this.emulatorDebugBrk();
}
} else {
if (this.$options.debugBrk) {
return this.deviceDebugBrk();
return this.deviceDebugBrk(true);
} else if (this.$options.start) {
return this.deviceStart();
} else {
let deploy = this.$platformService.deployOnDevice(this.platform);
deploy.wait();

return this.deviceStart();
}
}
Expand All @@ -72,14 +74,15 @@ class IOSDebugService implements IDebugService {
}).future<void>()();
}

private emulatorDebugBrk(): IFuture<void> {
private emulatorDebugBrk(shouldBreak? : boolean): IFuture<void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

when we have to pass boolean options to method, we prefer using objects and pass a property of them:

 private emulatorDebugBrk(debugOptions?: {shouldBreak : boolean}): IFuture<void> {

So the usage will be:

this.emulatorDebugBrk().wait()
this.emulatorDebugBrk({shouldBreak: true}).wait();

This way it's pretty easy to understand the purpose of the passed value. (passing only true does not give you this information)

And in the method you'll make the check:

let args = debugOptions && debugOptions.shouldBreak ? "--nativescript-debug-brk" : "--nativescript-debug-start";

Copy link
Contributor

Choose a reason for hiding this comment

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

Forgot to mention that this is not a merge stopper, it's up to you to decide which is the approach you prefer :) :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see many other methods with such signature. Can you point some? In fact most methods are using signature similar to what I've made here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course,
https://github.com/telerik/mobile-cli-lib/blob/master/declarations.d.ts#L133
https://github.com/telerik/mobile-cli-lib/blob/master/declarations.d.ts#L413
https://github.com/telerik/mobile-cli-lib/blob/master/declarations.d.ts#L431
https://github.com/NativeScript/nativescript-cli/blob/master/lib/declarations.ts#L170
https://github.com/NativeScript/nativescript-cli/blob/master/lib/declarations.ts#L320

But anyway, it depends on you to decide which approach to take. As I've said, this is not a merge stopper :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, these in my opinion are more like exceptions not common rule. Look at other methods signature.

Copy link

Choose a reason for hiding this comment

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

we prefer using objects and pass a property of them

It is not a matter of what is preferred but what works. Different teams have different cultures. Thanks for suggestion but please try to focus on arguments that both teams consider as constructive.

return (() => {
let platformData = this.$platformsData.getPlatformData(this.platform);
this.$platformService.buildPlatform(this.platform).wait();
let emulatorPackage = this.$platformService.getLatestApplicationPackageForEmulator(platformData).wait();

let args = shouldBreak ? "--nativescript-debug-brk" : "--nativescript-debug-start";
let child_process = this.$iOSEmulatorServices.runApplicationOnEmulator(emulatorPackage.packageName, { waitForDebugger: true, captureStdin: true,
args: "--nativescript-debug-brk", appId: this.$projectData.projectId }).wait();
args: args, appId: this.$projectData.projectId }).wait();
let lineStream = byline(child_process.stdout);

lineStream.on('data', (line: NodeBuffer) => {
Expand Down Expand Up @@ -108,12 +111,12 @@ class IOSDebugService implements IDebugService {
}).future<void>()();
}

private deviceDebugBrk(): IFuture<void> {
private deviceDebugBrk(shouldBreak? : boolean): IFuture<void> {
return (() => {
this.$devicesService.initialize({ platform: this.platform, deviceId: this.$options.device }).wait();
this.$devicesService.execute((device: iOSDevice.IOSDevice) => (() => {
if(device.isEmulator) {
return this.emulatorDebugBrk().wait();
return this.emulatorDebugBrk(shouldBreak).wait();
}
// we intentionally do not wait on this here, because if we did, we'd miss the AppLaunching notification
let deploy = this.$platformService.deployOnDevice(this.platform);
Expand Down