Use apksigner to sign Android APK, bump up major version #7544

yacaovsnc · 2018-06-21T17:30:46Z

jarsigner is no longer the right tool to sign APK files. Update the task to use apksigner.

This was reported in issue #4783.

Tested on Hosted, Hosted 2017 and macOS pool. It's able to locate the apksigner tool and use it to sign.

brcrista · 2018-06-21T17:52:26Z

What have you done to test?

yacaovsnc · 2018-06-21T17:54:24Z

Reformatting, changed a few files names, cleaned up some tests, removed unused fields, etc... There were quite a lot of changes went into tests. -_-

brcrista · 2018-06-21T17:58:30Z

I mean, what have you done to test the change? :)

lkillgore · 2018-06-21T18:00:27Z

Are you removing AndroidSigningV2? If so, this PR could be more legible if git was set-up as 'moves' and 'renames' instead of 'additions'.

yacaovsnc · 2018-06-21T18:01:25Z

UI didn't really change. I added a field to override the location of apksigner, this is to make it consistent with zipalign tool.

brcrista · 2018-06-21T17:53:04Z

Tasks/AndroidSigningV3/androidsigning.ts

+    // add * in search as on Windows the tool may end with ".exe" or ".bat"
+    const toolsList = tl.findMatch(tl.resolve(androidHome, 'build-tools'), tool + '*', null, { matchBase: true });
+
+    if (!toolsList || toolsList.length === 0) {


dead code -- toolsList.length === 0 implies !toolsList

Need some refreshers on dynamic languages... -_- Wouldn't you hit NRE (or equivalent of it in javascript) if you don't check for !toolsList?

No you're right. Just if (!toolsList) because an empty list is falsy

empty list isn't falsy in my testing:

a = 0 b = [] if (!a) { console.log("a is false") } if (!b) { console.log("b is false") }

prints a is false

All right, I changed it back to be explicit. Safety first. :)

Ughhhh @hashtagchris you're right: https://developer.mozilla.org/en-US/docs/Glossary/Truthy

I was confused because:

And because [] is falsy in Python:

And of course '' is still falsy, which makes no sense

brcrista · 2018-06-21T17:56:17Z

Tasks/AndroidSigningV3/task.json

+            "required": false,
+            "visibleRule": "apksign = true",
+            "groupName": "apksignerOptions",
+            "helpMarkDown": "Optionally specify the location of the apksign executable used during signing. This defaults to the apksign found in the Android SDK version folder that your application builds against."


Isn't the name of the tool apksigner? https://developer.android.com/studio/command-line/apksigner

brcrista · 2018-06-21T17:56:35Z

Tasks/AndroidSigningV3/task.json

+            "name": "apksignerLocation",
+            "aliases": ["apksignerFile"],
+            "type": "string",
+            "label": "Apksigner location",


Should the label say "APK signer location?"

The tool is called 'apksigner', maybe I shouldn't capitalize the first letter?

I think so, follow same pattern as in the android docs.

brcrista · 2018-06-21T18:00:33Z

Tasks/AndroidSigningV3/androidsigning.ts

+
+    // if the tool path is not set, let's find one (anyone) from the SDK folder
+    if (!apksigner) {
+        apksigner = findAndroidTools('apksigner');


Do we not want to fall back to JAR signing if the APK signer tool can't be found?

We don't want to fall back to JAR signing especially since it has been a year since apksigner switch was made. I like we are updating the major version of the task, so customers who want to use jarsigner can use previous version.

I agree. This is worth considering. This tool came out with version 24 (Android 7.0). https://developer.android.com/about/dashboards/ Not many use this, yet. Developers may not have updated their privately hosted agents either, and want the same definition to work on both.

I think it's ok not to fall back if JAR signing is considered deprecated now. I just wanted to know what the debate was.

I think it would be good to add in the "releaseNotes" that if you are on Android 7 or higher, you should use V3. We could update the task description on V2 as well to use V3. I am assuming apksigner would work for older android versions as well if the tool is available and used.

brcrista · 2018-06-21T18:01:55Z

Tasks/AndroidSigningV3/androidsigning.ts

+import * as Q from 'q';
+import * as tl from 'vsts-task-lib/task';
+
+const findAndroidTools = (tool: string): string => {


I think findInAndroidTools or findAndroidTool better describes what this function is doing. Also, would it make more sense to return a tl.ToolRunner instead of a string?

Changed to findAndroidTool.

yacaovsnc · 2018-06-21T18:03:00Z

@lkillgore not removing the v2 folder, the current recommendation is to keep all major versions side by side according to @stephenmichaelf.

lkillgore · 2018-06-21T18:05:48Z

Tasks/AndroidSigningV3/androidsigning.ts

+const findAndroidTools = (tool: string): string => {
+    const androidHome = tl.getVariable('ANDROID_HOME');
+    if (!androidHome) {
+        throw tl.loc('AndroidHomeNotSet');


I think it's better to get in the habit of using this pattern: throw new Error(tl.loc('AndroidHomeNotSet'). That way we get a better stack trace.

Several places in this file could use that improvement.

yacaovsnc · 2018-06-21T18:06:22Z

@brcrista I uploaded this task to my personal account, and used it to sign APKs. Only tested in Hosted VS2017 for now, need to test on other OSes.

madhurig

Did you test on hosted win, mac and linux vms? I would expect this to work on windows and mac.

madhurig · 2018-06-21T18:12:21Z

Tasks/AndroidSigningV3/postandroidsigning.ts

@@ -0,0 +1,21 @@
+import fs = require('fs');


switch to import * as fs syntax?

(modules imported with import-from are treated as readonly, while import-require basically gives you a JS object)

madhurig · 2018-06-21T18:13:27Z

Tasks/AndroidSigningV3/task.json

+    "demands": [
+        "JDK",
+        "AndroidSDK"
+    ],


Add "releasenotes" to explain what changed in this version. https://github.com/Microsoft/vsts-tasks/blob/master/Tasks/XcodeV5/task.json#L17

stephenmichaelf · 2018-06-21T18:24:53Z

@yacaovsnc @lkillgore Yes they both live side by side. I just realized... in the future it may be easier to do a branch with the new task and then a branch off of that with the changes. Have had this problem with a few other tasks that bumped major too. :)

stephenmichaelf · 2018-06-21T18:29:05Z

Also whenever you get a chance, we are generally moving away from Typings since it's deprecated and moving to @ types.

madhurig · 2018-06-21T18:31:10Z

Only issue I have with @ types is VSCode seems to not recognize them or I am missing a VSCode/plugin update

brcrista · 2018-06-21T18:37:57Z

@madhurig it works fine for me, I used @ types for the Python tasks. Shouldn't need a plugin

lkillgore · 2018-06-21T19:48:49Z

Tasks/AndroidSigningV3/preandroidsigning.ts

+
+async function run() {
+    let keystoreFileId: string;
+    let secureFileHelpers: secureFilesCommon.SecureFileHelpers;


This is just a 'nit', but I don't see why these are scoped outside the if-block, and not 'const'.

ckrempp91 · 2018-10-15T17:28:00Z

When will this be available for use?

brcrista · 2018-10-15T17:38:38Z

This was released several months ago. Make sure you have updated to V3 of the task.

ckrempp91 · 2018-10-15T17:56:10Z

Ok, the documentation is still referring to jarsigner here: https://docs.microsoft.com/en-us/azure/devops/pipelines/tasks/build/android-signing?view=vsts, was this made available for TFS 2017, or just TFS 2018 & Devops?

brcrista · 2018-10-16T11:26:30Z

This would only be in on-prem versions that shipped after this PR was completed.

brcrista · 2018-10-16T19:17:21Z

@ckrempp91 I checked on this and the only on-prem release we've shipped was TFS 2018 Update 3, which was just a bug fix release. So this will go with Azure DevOps Server 2019 (TFS 2019).

ckrempp91 · 2018-10-16T21:50:38Z

Thank you for the update @brcrista!

Use apksigner to sign Android APK, bump up major version

83b4348

yacaovsnc requested a review from stephenmichaelf June 21, 2018 17:49

brcrista reviewed Jun 21, 2018

View reviewed changes

lkillgore reviewed Jun 21, 2018

View reviewed changes

madhurig reviewed Jun 21, 2018

View reviewed changes

stephenmichaelf approved these changes Jun 21, 2018

View reviewed changes

review feedback

f5c1cad

lkillgore reviewed Jun 21, 2018

View reviewed changes

lkillgore approved these changes Jun 21, 2018

View reviewed changes

brcrista approved these changes Jun 21, 2018

View reviewed changes

review feedback v2

7a017ab

yacaovsnc merged commit d28dc61 into master Jun 22, 2018

yacaovsnc deleted the users/yacao/apksigner branch June 22, 2018 13:24

Use apksigner to sign Android APK, bump up major version #7544

Use apksigner to sign Android APK, bump up major version #7544

Conversation

yacaovsnc commented Jun 21, 2018 • edited Loading

brcrista commented Jun 21, 2018

yacaovsnc commented Jun 21, 2018

brcrista commented Jun 21, 2018

lkillgore commented Jun 21, 2018

yacaovsnc commented Jun 21, 2018

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

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

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

yacaovsnc commented Jun 21, 2018

lkillgore Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yacaovsnc commented Jun 21, 2018

madhurig 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

stephenmichaelf commented Jun 21, 2018 • edited Loading

stephenmichaelf commented Jun 21, 2018

madhurig commented Jun 21, 2018

brcrista commented Jun 21, 2018

Choose a reason for hiding this comment

ckrempp91 commented Oct 15, 2018

brcrista commented Oct 15, 2018

ckrempp91 commented Oct 15, 2018

brcrista commented Oct 16, 2018

brcrista commented Oct 16, 2018

ckrempp91 commented Oct 16, 2018

yacaovsnc commented Jun 21, 2018 •

edited

Loading

lkillgore Jun 21, 2018 •

edited

Loading

stephenmichaelf commented Jun 21, 2018 •

edited

Loading