Archinamon / android-gradle-aspectj

gradle plug-in adding supports of AspectJ into Android project
Apache License 2.0
365 stars 58 forks source link

Improvements to plugin lifecycle #42

Closed Badya closed 7 years ago

Badya commented 7 years ago

Preconditions: I have a project with AspectJ plugin applied in my app/build.gradle:

classpath "com.github.Archinamon:GradleAspectJ-Android:3.0.0"

I included this dependency right before #41 fix It seems that my gradle caches haven't updated to new version after release, so there've been an NPE problem while gradle configured my project, and while using .jar build from sources everything was OK.

Problems: Even @Archinamon added a check to com.archinamon.utils.AarExploringKt#findPackageNameIfAar:

if (!input.exists()) return "[empty]"

Using com.archinamon.api.AspectJCompileTask.Builder#findCompiledAspectsInClasspath before changing the classpath seems creepy %)

I also think that removing

project.dependencies.add("compile", "org.aspectj:aspectjrt:${settings.ajc}")

from com.archinamon.plugin.PluginSetupKt#configProject and including an update about this dependency into README will be better.

Suggestion: Even if fix is very small it is better to change version to minor update like 3.0.0 -> 3.0.1 - just to be shure that caches won't stuck.

Archinamon commented 7 years ago

3.0.0 -> 3.0.1

That's true, I thought no one been updated before my fix :)

an update about this dependency into README will be better.

My mind it's bad idea 'cause gradle plugin should mostly incapsulate all possible features inside. Runtime jar is critically need to work so why should we deliver this step to the developer? Anyone can simply forget it (most in case of updating to a newer plugin) and then open an issue :)

Thank's for contributing!

Badya commented 7 years ago

@Archinamon i am using it only for androidTestCompile for example, so why do i need a compile dependency, i think it is better when you manage dependencies you need, instead of plugin doin' it behind your back.

Badya commented 7 years ago

@Archinamon In addition, can we throw some Exception with info, if runtime wasn't provided?

Archinamon commented 7 years ago

It's still complicated solution :) In case you're using aj only for androidTest variants I see a solution to explicitly specify the tests-only scope to avoid mutation of classpath of root module. In additional I wanna say that progruard will cut unneeded dependencies (aj rt if you're not using it while building assembleRelease) out of apk.

Badya commented 7 years ago

@Archinamon FYI 3.0.0 from jitpack still behaves bad - consider re-releasing it =)

Badya commented 7 years ago

@Archinamon, waiting for review from you =)

Archinamon commented 7 years ago

One question left :)

Badya commented 7 years ago

Am i missing something? It seems complete. And i've created an issue #45 to make it separated from this changes.

Archinamon commented 7 years ago

pr merged