Skip to content
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.

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

feat(cli): add apksigner as a build option #6442

Merged
merged 22 commits into from
Jun 26, 2023
Merged

feat(cli): add apksigner as a build option #6442

merged 22 commits into from
Jun 26, 2023

Conversation

yooouuri
Copy link
Contributor

closes: #6284

@markemer
Copy link
Member

markemer commented Apr 5, 2023

Can you run npm run fmt - that should fix the lint failures.

@markemer markemer self-assigned this Apr 5, 2023
@yooouuri
Copy link
Contributor Author

yooouuri commented Apr 5, 2023

Can you run npm run fmt - that should fix the lint failures.

Sure my bad

@yooouuri
Copy link
Contributor Author

@markemer lint failures should be fixed now

@yooouuri
Copy link
Contributor Author

yooouuri commented Apr 12, 2023

@markemer node scripts/pack-cli-assets.mjs was failing, my latest commit fixed this

Edit: Also removed the keystorealias and keystorealiaspass build options, because they are not needed anymore

@IT-MikeS
Copy link
Contributor

Looks like some lint errors still showing up. You may just need to run another fmt but you may need more work as well.

@yooouuri
Copy link
Contributor Author

@IT-MikeS ran another npm run fmt

@yooouuri
Copy link
Contributor Author

@markemer any success on checking my PR?

@markemer
Copy link
Member

I got buy in, I think we just needed to wait for Cap5 to ship. Now I just need to get some reviews.

@markemer
Copy link
Member

I think I want to add a fallback for people that still want to use jarsigner (as inadvisable as that may be) do you mind if I make changes to this branch? I'd like you to get credit for your contributions, so if you want me to work elsewhere, we can setup a branch on main to merge this into.

@yooouuri
Copy link
Contributor Author

yooouuri commented May 30, 2023

@markemer no problem, be my guest! Use this branch!

@markemer
Copy link
Member

@markemer no problem, be my guest! Use this branch!

Cool - will do.

@liuwin7
Copy link
Contributor

liuwin7 commented Jun 9, 2023

When will this feature be released.
Looking forward ...

@dfa1234
Copy link

dfa1234 commented Jun 11, 2023

Why this is still in review?
Android 11 was like 3 years ago...

@markemer markemer changed the title feat(cli): use apksigner instead of jarsigner feat(cli): add apksigner as a build option Jun 14, 2023
@markemer markemer requested a review from giralte-ionic June 14, 2023 20:48
@markemer
Copy link
Member

Hey @yooouuri - I think I have this working right - you have to affirmatively pick apksigner - but next major release I'm going to flip that so apksigner is the default

@yooouuri
Copy link
Contributor Author

yooouuri commented Jun 15, 2023

@markemer LGTM

added signingtype: 'apksigner', to buildCommand function and it works fine!

Using the vite plugin I created

https://github.com/bistroo/vite-plugin-capacitor

import { defineConfig } from 'vite'

export default defineConfig({
  plugins: [
    viteCapacitorPlugin({
      buildOptions: {
        keystorepath: 'xxxxxxx',
        keystorepass: 'xxxxxxx',
        androidreleasetype: 'APK',
        signingtype: 'apksigner',
      },
    }),
  ],
})

@ItsChaceD ItsChaceD requested a review from markemer June 26, 2023 18:19
@markemer markemer merged commit 9818a76 into ionic-team:main Jun 26, 2023
@15aabruzzese
Copy link

Hey @markemer, @IT-MikeS, and @yooouuri can you guys give me an example of how this is used, I've been trying to get this to work without any luck. I've done the following in the pipeline and tried it locally, but only get the following:

image

image

image

I've tried both AAB and APK file types with no luck and always seems to trigger the jarsigner error.

Please help, thank you!

@markemer
Copy link
Member

@15aabruzzese --signing-type apksigner is what you need to feed it, looks like you're feeding it --signing-type AAB

@15aabruzzese
Copy link

15aabruzzese commented Jun 29, 2023

@markemer I tried both APK and AAB. Here is what I am trying to get working locally, I'm getting a incorrect password error but checked the following: console logged it in build.js file, checked that it worked to access the keystore, used both "",'', pass:"", and pass:''. Also I can sign through Android Studio
image

image

@liuwin7
Copy link
Contributor

liuwin7 commented Jul 7, 2023

@markemer
I think you should try this feature again.

  1. If run the build command without options in console by using the capacitor.config.ts file. the error will occur.
[error] Missing options. Please supply all options for android signing. (Keystore Path, Keystore Password, Keystore Key
        Alias, Keystore Key Password)
  1. The command option --signing-type not work, because the third part lib, commander, transform the option name to signingType instead of signingtype.
    .addOption(
      new Option(
        '--signing-type <signingtype>',
        'Program used to sign apps (default: jarsigner)',
      ).choices(['apksigner', 'jarsigner']),
    )
    .action(
      wrapAction(
        telemetryAction(
@@ -168,6 +174,7 @@ export function runProgram(config: Config): void {
              keystorealias,
              keystorealiaspass,
              androidreleasetype,
              signingtype,

For the error, I created a new pull request. #6716

@@ -239,9 +239,6 @@ async function loadAndroidConfig(
const buildOptions = {
keystorePath: extConfig.android?.buildOptions?.keystorePath,
keystorePassword: extConfig.android?.buildOptions?.keystorePassword,
keystoreAlias: extConfig.android?.buildOptions?.keystoreAlias,
keystoreAliasPassword:
extConfig.android?.buildOptions?.keystoreAliasPassword,

Choose a reason for hiding this comment

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

It's mistake, return pls))

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, so I created a pull request to correct it, but still not merged yet. 😞

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

Successfully merging this pull request may close these issues.

feat: CLI build android with [apksigner] to support v2 signing
8 participants