Skip to content
This repository has been archived by the owner on Feb 11, 2022. It is now read-only.

Fix nougat install #110

Merged
merged 2 commits into from
Jan 11, 2017
Merged

Fix nougat install #110

merged 2 commits into from
Jan 11, 2017

Conversation

tasomaniac
Copy link
Contributor

@tasomaniac tasomaniac commented Jan 11, 2017

The install task is broken on Nougat devices.

Solution

Separate the arguments so that it works on Nougat devices.

Combining different arguments was not a documented feature. It might be working on some devices, it might be ignored on some devices. Now it throws exception on Nougat devices because Nougat has a complete new implementation for adb shell interaction.

Note: Unfortunately again, it was a silent fail. It was trying to run after install fail, when we use run tasks.

So I decided to include a code piece that I had on my stash. It checks the exit code and errors accordingly. This feature will only work on Nougat devices but still it is useful for Nougat users.

Here is the output on a failure:

screen shot 2017-01-11 at 10 12 44 am

This PR fixes #108

Said Tahsin Dane added 2 commits January 11, 2017 10:14
Combining different arguments was not a documented feature. It might be working on some devices, it might be ignored on some devices. Now it throws exception on Nougat devices because Nougat has a complete new implementation for adb shell interaction.
Copy link
Contributor

@florianmski florianmski left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@@ -12,7 +12,7 @@ class Install extends AdbTask {
if (getCustomFlags())
arguments += getCustomFlags()

arguments += ['-rd', apkPath]
arguments += ['-r', '-d', apkPath]
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@mr-archano mr-archano left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@mr-archano mr-archano merged commit bdaf009 into develop Jan 11, 2017
@mr-archano mr-archano deleted the 108/fix-nougat-install branch January 11, 2017 12:50
handleCommandOutput(command.execute().text)

Process process = command.execute()
if (process.waitFor() != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is what you mention only works on Nougat? Would be good to add a comment about that then, imho.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly. But it doesn't change any backward compatibility because it was always 0 before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me open a quick pr adding a comment 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The install task is broken on Nougat
4 participants