-
-
Notifications
You must be signed in to change notification settings - Fork 272
fix: Allow patcher to run on dex-less APKs #360
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
base: dev
Are you sure you want to change the base?
Conversation
| * @param patches The patches to add. | ||
| */ | ||
| operator fun plusAssign(patches: Set<Patch<*>>) { | ||
| // Filter out bytecode patches if bytecodeContext is null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldnt be done. What if I want to add and operate on dex files using bytecode patches
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't anyway with a dexless APK. This just acts as a safeguard since the apis are gonna throw exceptions left and right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mean is add a new class/dex to a dex less APK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean now. I'm currently a little busy but I'll try to get address these sometime soon
| * The context for patches containing the current state of the bytecode. | ||
| */ | ||
| internal val bytecodeContext = BytecodePatchContext(config) | ||
| internal val bytecodeContext : BytecodePatchContext? = try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of controlling flow with exception handling you should check if the APK has a dex. I wouldn't use the manifest to determine that. Checking if the APK has dex files seems also weird. Open to suggestions. Maybe check where EmptyMultiDexContainerException is raised
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I am not sure if we want this to be null, or bytecodecontext initialized but something inside it empty or null. There is three scenarios:
- APK
- APK with dex classes
- APK with dex file but empty list of classes
Right now you don't handle the third case and I don't know if this needs to be handled, or how.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of controlling flow with exception handling you should check if the APK has a dex.
Why? This is a good way to detect a dexless APK that wouldn't involve practically repeating the logic of androlib.
Right now you don't handle the third case and I don't know if this needs to be handled, or how.
The third case doesn't make sense. If you have a dex file, it must at least have the app entrypoint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the other comment motivates this one. What if I want to add classes to dexless APKs? I need a bytecodepatch context, i also would be able to empty the list of classes or add classes, so the three cases apply.
oSumAtrIX
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple changes needed
Resolves #359