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

Improve get_apksigner_path() robustness #67668

Merged
merged 1 commit into from
Dec 23, 2022

Conversation

nikitalita
Copy link
Contributor

There are a few existing problems with signing with apksigner, all having to do with the fact that we currently just use the first one we find:

  • We don't check if apksigner is for the Target SDK
  • We don't check to see if apksigner is for a version below DEFAULT_TARGET_SDK
  • We don't check to see if the version of apksigner we chose is actually executable.

The latter is a problem if the user has many versions of build-tools installed (which is very likely if you're an android developer), and if the user has a version of build-tools that requires Java <10 (-Djava.ext.dirs was removed in Java 10). Since we currently just use the first one we find, it will be the lowest version number, which makes this more likely, and apksigner will likely fail to execute. Build tools version 27 was the last version that required Java <10.

So, this PR will trawl the versions in the directory, and it will choose one that matches the target sdk. If not, it chooses a version between 28 (or if the target sdk was manually set lower) and DEFAULT_TARGET_SDK. If it can't find any of those, it will use the first available.

It then checks all the versions it found, in order of ideal to worst, to see if they execute. Once one executes, it returns that path.

We don't check to see if it executes when we aren't actually exporting, because this can significantly slow down the UI if we do this.

Copy link
Contributor

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

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

Looks great!

@akien-mga akien-mga merged commit f7cf9fb into godotengine:master Dec 23, 2022
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:3.x Considered for cherry-picking into a future 3.x release platform:android topic:export
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants