Ibotta / gradle-aspectj-pipeline-plugin

A Gradle plugin for Android projects which performs AspectJ weaving using Android's bytcode manipulation pipeline.
Apache License 2.0
75 stars 15 forks source link

aopWeave filter option is broken or misleading #2

Closed jdvp closed 3 years ago

jdvp commented 3 years ago

In the README, the following way of limiting which aspects are weaved is given:

// This block is optional, but recommended. The "filter" will be used to limit the scope of classes
// that AspectJ will look at. For performance reasons, and to avoid accidentally picking up AspectJ
// annotations from other libraries, specify a filter which will target only your code.
aopWeave {
    filter = "com/example/myapp"
}

In the sample apps, this filter value is "com/ibotta/gradle/aop". Based on the README, I expected that changing this to some other random package, which is not specified in the project at all (such as "com/jdvp/fake") would ensure that no aspects are weaved at all, since the filter would never be matched.

However, when using any filter value, the output is always the same in the sample app. In other words, the weaved aspects as specified in aop.log always remains the same.

Internally, the filter flag is being used to generate a list of files for the -aspectpath option. I'm not sure if I am mis-reading the documentation, but this option seems to be used for specifying additional .jar or .zip files to use for declaring aspects outside of the regular source code:

In AspectJ tools, the aspectpath is where to find binary aspects. Like the classpath, it can include archives (.jar and .zip files) and directories containing .class files in a package layout (since binary aspects are in .class files). These aspects affect other classes in exactly the same way as source-level aspects, but are themselves not affected. When deploying programs, the original aspects must be included on the runtime classpath.

I took the above from here, please let me know if it isn't an appropriate resource.

Since I am unsure of the real intention for the filter option, I'm unsure of which option would be warranted (if any):

Please let me know if I have misunderstood the option! Thanks

eschlenz commented 3 years ago

@jdvp So sorry for the late reply. I must have missed the email notification from Github.

A little background on why this feature exists. We were finding that without the filter, AspectJ was hunting everywhere for Aspects. This wasn't really what we wanted, as it's unnecessary, and we wanted AspectJ to focus on a certain area of the codebase.

My understanding of -aspectpath is that is not additive to some other set of aspect paths, rather it's the only one. If that makes sense.

However, as I'm sure you've experienced, the parameters for AspectJ are kind of confusing. So it's very possible I'm wrong in my understanding.

I'd have to research this a little more. If you changed the value of the filter to something nonsensical, and it still picked up the sample app's aspects, it would certainly suggest it's not doing what I expected.

jdvp commented 3 years ago

@eschlenz No worries, thanks for getting back to me!

I was also confused on the parameters, because this quote

In AspectJ tools, the inpath is where to find binary input - aspects and classes that weave and may be woven. Like the classpath, it can include archives and class directories. Like the aspectpath, it can include aspects that affect other classes and aspects. However, unlike the aspectpath, an aspect on the inpath may itself be affected by aspects, as if the source were all compiled together.

from here mentions that inpath can have aspects but it doesn't really mention if there is a way to turn that on or off or whether aspects on the inpath will always be woven

eschlenz commented 3 years ago

@jdvp I did some more research on this to get some clarification, and also remembered something that is relevant here.

First of all, the difference between aspectpath and inpath is just whether the binaries found in either may be modified (woven) as part of the weave output. Binaries in the aspectpath are not modified, but are used as a source of weaving information for target classes. inpath may be modified (woven), and as such, you must make sure the woven versions of these binaries are available at runtime.

Things get interesting in a multi-module project. In the example code, the filter options is being set to essentially signal to the plugin that it should find aspects/advice either in the current module's source code which is being built, or in a neighboring (dependency) module. But nowhere else (not in 3rd party libraries for example).

Say you have two project modules A and B, and A depends on B. Now also say that B is the home for your aspects/advice. An in addition, B has classes that are targets for being woven. Also say that A has classes that are targets for being woven. Also assume we have a filter set to have aspectpath only look at files from our app's package structure.

When B is woven the inpath will only contain compiled classes from B's source code, and aspectpath will only have classes on from the classpath that belong to our project. In the case of B, I think aspectpath would be empty since it's the first module to be built. The output would contain compiled aspect/advice classes, and also woven classes that depended on those aspects/advice.

When A is woven the inpath will only contain compiled classes from A's source code. The aspectpath, however, will include the classes from B's output (including aspects/advice). But nothing else. Now when A is woven, only aspects/advice from B will be contributing to the weave process.

I'm remembering that we had some issues with aspects/advice getting picked up from the Butterknife library, affecting our weave output, and breaking things at runtime. I believe what was happening is that aspects/advice from Butterknife were in the aspectpath, and were affecting our classes during the weave process. And as a result, we needed to find a way to avoid that. Limiting the aspectpath so that it only focused on our own classes seemed to do the trick.

Butterknife still works since it performs its own weaving on classes at a different point in time. We just don't want its aspects/advice to be picked up during our plugin's weave process.

Hopefully this at least paints the picture of where filter came from, and possibly how it works. My assumption is that it works as described because we haven't had any more issues with Butterknife aspects/advice negatively affecting our output. It's always possible that it's not doing what we think it is though.

eschlenz commented 3 years ago

Not 100% sure what to do with this issue, but I do see the thumbs up in my last response. I'll close this out for now, but be on the lookout for any future issues that may have anything to do with this.

jdvp commented 3 years ago

@eschlenz That totally makes sense. Thanks again!