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

Work around an issue with ASM rewriting types with names starting with $ #22

Merged
merged 2 commits into from
Oct 27, 2023
Merged

Conversation

bddckr
Copy link
Contributor

@bddckr bddckr commented Apr 24, 2023

@bddckr
Copy link
Contributor Author

bddckr commented Apr 24, 2023

I ran into the linked issue. As explained in there, the bug is actually with ASM, kinda. However, considering that HiddenApiRefinePlugin is one of the special plugins in the Android ecosystem as it instruments not only symbols that were explicitly annotated, I've started to apply the workaround found in this PR.

Feel free to close this PR without merging it! I'm just sharing this in case it's helpful!

@bddckr
Copy link
Contributor Author

bddckr commented Apr 24, 2023

As discussed in the linked issue, I'll raise the underlying issue with ASM. I will then most likely come back to this PR to simply use a new ASM version with the fix (assuming they are OK with it, that is). In the meantime, I'll mark this PR as a draft.

@bddckr bddckr marked this pull request as draft April 24, 2023 14:25
@bddckr
Copy link
Contributor Author

bddckr commented Apr 30, 2023

I'm marking this PR as ready for review while waiting on what the maintainers of ASM think about this problem. I've opened an issue with them here.

I will raise another PR once/if ASM fixes the problem on their side. In the meantime, this PR unblocks anyone running into this issue.


You might be wondering: Why are other Android instrumentation plugins seemingly not running into this problem? AGP's transformClassesWith will instantiate and call the given AsmClassVisitorFactory. HiddenApiRefinePlugin is exceptional in that it overrides isInstrumentable always to return true - it wants to potentially instrument all classes. Other plugins only do that for specific classes (annotated with the plugin-specific annotation(s)).


I've added HiddenApiRefinePlugin's Gradle build via includeBuild into my projects, but I'm running into issues with the Android Gradle Plugin's Lint tasks. They attempt to look up the dependencies of the build, which includes this plugin as a local copy now... but that fails with some cryptic issues. However, I can confirm that it doesn't fail if I use this plugin's released version. So I'm hoping that a new release of this plugin, with this PR's changes merged, would resolve that issue and allow me to continue to work with it in my projects (the Lint task is a mandatory check in my CI).
All good here now! Resolved by fixing the includeBuild statements - I accidentally had them in the wrong settings.gradle.kts, so it didn't apply to all projects.

@bddckr bddckr marked this pull request as ready for review April 30, 2023 18:04
@bddckr
Copy link
Contributor Author

bddckr commented May 13, 2023

The ASM maintainers have decided that they won't fix this, so this PR is needed for this plugin to work correctly when using Kotlin Serialization.

@bddckr
Copy link
Contributor Author

bddckr commented Jul 10, 2023

@RikkaW @Kr328 Hello 👋 Could you chime in on here? I'm happy to change this PR to be acceptable to you. Is there anything I should split into a separate PR or explain better?

I'd love to remove my fork of this plugin and get back to using your source (and published releases), but this issue affects me as I'm consuming hidden APIs in projects that also use Kotlin Serialization.

@Kr328
Copy link
Collaborator

Kr328 commented Oct 16, 2023

I apologize for the delayed review of your Pull Request. Could you please rebase the main branch :)

@bddckr
Copy link
Contributor Author

bddckr commented Oct 25, 2023

@Kr328 No worries! I've updated the branch.

@Kr328 Kr328 merged commit ef5d6a8 into RikkaApps:main Oct 27, 2023
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.

2 participants