Community
Participate
Working Groups
This is from a private email sent to me as a follow up to and commenting on the solution for bug 273770. --------------------------- I've run a bunch of tests (described below) and have spent some time thinking about the implementation from a UI perspective in order to provide some constructive feedback. I think one of the fundamental problems here is in the use of classpath containers to hold the Aspect Path but I appreciate that this is not something that can be rectified. From what I've understood, the way AJDT is using the classpath is to see it as a superset of the aspect path by tagging classpath entries with the "org.eclipse.ajdt.aspectpath" attribute. Although this approach does not align perfectly with AJC, conceptually it is intuitive. The issue I have is that the steps taken to populate the classpath with ajdt attributes results in ambiguity and fragility (see tests), especially with the 'Add Library' mechanism. From the UI side would it be possible to take the following approach: * leave the existing behaviour of adding to the aspect path as-is * i.e. entries added by existing mechanism will be added to classpath. * provide an option to 'Add Classpath Entry'. * dialogue box pops up to select a classpath entry. * implementation does the same by adding the aspectpath attribute. For me this would be the easiest to understand on the UI side that allows the existing behaviour to be preserved whilst also allowing my use-case to be dealt with easily. On the filtering side I think there are two reasons why you would want to filter: * If there are multiple aspects found, some of which you don't want. * If there is a performance degradation having jars on the aspect path that are not used. On the performance side I would hope that AJDT caches the aspects it finds on the classpath, and updates this cache when the build classpath is updated. For the other use-cases I feel filtering on a jar name is a little fragile and does not work with dependency mechanisms such as ivy in which you ideally don't have to worry about jar names. From a user's perspective I care about the aspects (not the jars) hence I would want to filter on the qualified name of the aspect and not the jar name itself. I hope this feedback helps, please find the tests I ran below. Test 1 (PASS): basic add library test ------------------------------------------------------------- Steps taken: * Java Build-Path -> Add Library -> IvyDE Managed Dependencies. * Select all ivy configurations. * ivy.xml container added to classpath. * Aspect-Path -> Add Library -> IvyDE Managed Dependencies. * Select all ivy configurations. * Existing ivy.xml container has AP next to each jar. * Aspects correctly weaved in. Test 2 (PASS): verify add/remove dependencies on classpath container ------------------------------------------------------------- Steps taken: * Follow steps in Test 1. * Remove a dependency from the ivy.xml container. * Re-resolve dependencies * Aspect path preserved, aspects weaved in correctly Test 3 (FAIL): verify update to properties on classpath container ------------------------------------------------------------- Steps taken: * Follow steps in Test 1. * Alter the container by updating properties or adjusting configurations used Fails because the ivy entry in the .classpath was added then removed resulting in the "org.eclipse.ajdt.aspectpath" being removed. Would I be correct in thinking that IvyDE should be keeping these attributes according to the Eclipse specifications? Test 4 (FAIL): verify different ivy conf can be used on aspect path ------------------------------------------------------------- Steps taken: * Java Build-Path -> Add Library -> IvyDE Managed Dependencies. * Select all ivy configurations * ivy.xml container added to classpath. * Aspect-Path -> Add Library -> IvyDE Managed Dependencies. * Select specific config which contains aspect jars already on classpath. * End up with duplicate classpath entry error. Test 5 (FAIL): verify exclusion filter ------------------------------------------------------------- Steps taken: * Java Build-Path -> Add Library -> IvyDE Managed Dependencies. * Select all ivy configurations. * ivy.xml container added to classpath. * Aspect-Path -> Add Library -> IvyDE Managed Dependencies. * Add inclusion filter for specific jars. * AP icon still on all jars in the ivy container on the classpath. * Expecting just the jar being included to have AP icon. As mentioned I would prefer to filter by the aspect and not by the jar.
Here's my response: Thanks for the feedback and running the tests. This gives me a clear idea on how you would like to use the aspect path and its current limitations. First, the reason why classpath attributes are used to specify aspect/inpath entries is so that we can fit in with the JDT classpath model properly. By doing things this way, we get classpath validation and storage/updates for free. > From the UI side would it be possible to take the following approach: > > * leave the existing behaviour of adding to the aspect path as-is > * i.e. entries added by existing mechanism will be added to classpath. > * provide an option to ?Add Classpath Entry?. > * dialogue box pops up to select a classpath entry. > * implementation does the same by adding the aspectpath attribute. I'm not exactly sure what you are suggesting here. Are you saying that you want the option to open a dialog box. This dialog box contains all existing classpath entries (in the .classpath file). Then perhaps you can choose via checkboxes which entries should be added to the aspect path? If so, this might be a nice feature. This way it is explicit that you are not actually adding new entries to the aspect/inpath, but rather augmenting existing entries with the new path attribute. --------------- The next thing you mention is about filtering by aspect, rather than by classpath entry. This is something that is possible to do. There is a split between AJDT and the compiler. AJDT knows about Eclipsy things, like projects, classpath entries, etc. It knows how to translate them into classpath inpath, and aspectpath for the compiler. However, it doesn't know what the aspects are in the jars on the classpath. So, the filtering you are describing would have to be handled by aspectj (with AJDT passing aspectj the necessary information). Currently, although we have made some big improvements recently, incremental compilation is more expensive when you have many things on your aspect path. So, in general, you want to have the smallest aspect path possible. So, even with aspect-granularity filtering as you suggest, the compiler would still need to analyze the entire aspect path to find which aspects are actually being applied. It is worth creating an AspectJ enhancement request for this, though. Please do so and we can continue this discussion. --------------------- And lastly, to comment on your tests: Test 3: This does seem like an Ivy issue. Ivy should be keeping the existing classpath attributes on the classpath entry as that entry is updated. I guess this is showing my ignorance of the ivy plugin. Here, are you removing the old ivy classpath container and then re-adding it? If so, then you will need to also re-add the aspectpath entry for it. I can't think of a way that we can store this information if the original classpath entry is deleted. Test 4: Can you have two ivy classpath containers in the same project? I guess this is showing my ignorance of the ivy plugin, again. In the test that you tried, is there overlap between the two containers? If so, is that valid in a pure Java project? My less than completely informed guess here is that the correct thing to do here would be to 1. add the main ivy classpath container to the aspect path and 2. add a filter so that only the jars that you want to be on it will be. Please explain why if this is not possible. Test 5: This is a UI bug. The label provider that displays the AP icon is not looking at the filter.
Thanks Andrew. Your understanding on the UI side is correct. Having a dialog box that allows you to select the correct classpath container makes AJDT's intention very clear, there is no ambiguity and this should work with any dependency mechanism. I've followed up on the ivy mail group to get the ball rolling on IvyDE deleting the classpath attributes. In my case I personally am not removing the container, simply updating properties much like you can attach source code attachments to an external jar which will update the classpathentry element. I'm a little unclear on what feature I need to submit for the filtering of aspects. If I've understood correctly, currently the aj compiler does not support a list of aspects to weave in (like the loadtime weaver's aop.xml file) to filter the aspect path. If this is the case, I'll submit the enhancement request.
I haven't been keeping up to speed on all the comments here but your last point sounds like bug 124460
Andy, thanks for pointing out this bug entry to me. This looks to address what we need at the aspectj compiler level. I remember requesting this a long time ago because we were hitting issues with non production aspects leaking out. In the group I work in, we provide toolkits which may include aspect libraries. Without this feature we have had to split aspects into jars by use-case which is not ideal. Andrew, would it be possible to integrate this into the UI somehow? i.e. provide the option to supply the aop.xml files through the UI, ideally with the option of allowing the files to be relative to the project too?
Andy, I forgot about bug 124460, but it does seem to be what we need here. I just raised bug 279701 to talk about rich editing support for aop.xml. Would be quite useful for LTW.
It seems the critical requirement here is just a way to pass them through to AspectJ initially. Which bug is covering that now? Is it continuing with this bug? or is it part 3 of that new enhancement you raised? Although sexy editing is nice for aop.xml files, I don't know it is as critical as just having a way to pass them through. So I might have spun that critical requirement out into another smaller enhancement (having this minimal thing would also make my testing job easier as I work on 124460). If you look at ICompilerConfiguration, you will see it already mentions: /** * @return a list of those files that should be used to configure a build */ public List /* String */getProjectXmlConfigFiles(); which can be implemented by AJDT to pass the right things through. Unfortunately on the AspectJ side there is still design work to be done, not just coding, so the outlook for it being addressed is probably not AspectJ 1.6.5. The basic support that is there *may* be sufficient for some simple cases now, but it certainly doesn't respect all parts of the aop.xml file (I think it ignores the weaver section). And ironically the extended 'scope=' syntax the compiler recognizes in aop.xml doesn't get handled correctly by ltw.
Andrew, Is there anything else you need from my end? The main initial feature enhancement I'd like to see is the ability to select an existing container for the Aspect Path. This will be a big usability win for us. At the moment I'm finding a lot of people are struggling to use AJDT at the firm because we use IvyDE managed dependencies. FYI, I followed up with the ivy developers. It is a known issue in Ivy which will be fixed but not right now due to them going into final stages for a new minor release. Jeff
Hi Jeffrey, I am working on completing the 2.0 release and so I will not be able to get to this enhancement until after the release is completed. The first paragraph of c2 is something I can implement, but the rest has a dependency on the compiler. Can you make a note on bug 124460 about what your requirements are?
Hi Jeffrey, I just added some improvements to the way that the aspect path and in path can be managed from the package explorer. Now, when right-clicking inside of a jar inside of a classpath container (such as ivy's dependency manager), you get the option to Update the aspect path restriction and update the in path restriction. This allows you to specify a restriction in the same way that you can from inside the project properties page. Additionally, when adding a jar inside of a classpath container, the restriction is populated for you automatically, so you can effectively add only a single jar to the aspect/inpath. Also, now the Aspect Path and In Path icons only appear if the jar is not excluded from the path (via the restrictions attribute). This will not allow you to filter by aspect, but there is now more intuitive control over how to add and manage restriction filters on aspect and in path elements. Please try this out and let me know what you think of it.
Created attachment 143012 [details] Screenshot showing prev and new approach I've just updated, and re-added the library using the new context menu. This is certainly a lot better user experience. I think the dialog which asks for the name to match on needs some clarification, as it wasn't entirely obvious whether wildcards were allowed, and whether what I entered would do the job. How about an example in that dialog such as: "e.g. use 'org.mystuff.aspects' if you want to match org.mystuff.aspects-1.0-SNAPSHOT.jar'"
As shown in the above screenshot, there could be some confusion during migration to this approach. I can't see where the previous AspectPath entry was coming from, and I can't delete it.
(In reply to comment #10) > How about an example in that dialog such as: > "e.g. use 'org.mystuff.aspects' if you want to match > org.mystuff.aspects-1.0-SNAPSHOT.jar'" I was planning on going through the dialog again before the next release. This is a good suggestion on how to change it. (In reply to comment #11) > As shown in the above screenshot, there could be some confusion during > migration to > this approach. I can't see where the previous AspectPath entry was coming > from, and > I can't delete it. Yes, this aspect path entry is produced by m2eclipse plugin. You can manage it from the pom.xml. Maybe there could be some confusion, but I am not sure how to address it. Any ideas?
Ah, okay. That'll be: <plugin> <groupId>org.codehaus.mojo</groupId> <artifactId>aspectj-maven-plugin</artifactId> <version>1.0</version> <dependencies> <!-- NB: You must use Maven 2.0.9 or above or these are ignored (see MNG-2972) --> <dependency> <groupId>org.aspectj</groupId> <artifactId>com.springsource.org.aspectj.runtime</artifactId> <version>${aspectj.version}</version> </dependency> <dependency> <groupId>org.aspectj</groupId> <artifactId>com.springsource.org.aspectj.tools</artifactId> <version>${aspectj.version}</version> </dependency> </dependencies> <executions> <execution> <goals> <goal>compile</goal> <goal>test-compile</goal> </goals> </execution> </executions> <configuration> <aspectLibraries> <aspectLibrary> <groupId>org.springframework</groupId> <artifactId>org.springframework.aspects</artifactId> </aspectLibrary> </aspectLibraries> <source>1.5</source> <target>1.5</target> </configuration> </plugin> Deleting the aspectLibraries element clears out the old entry, but isn't what we actually want, as it'll break the maven build, so I've stuck with my old approach. Good news is that I've just run my Spring JPA tests as m2e launch and as Eclipse JUnit launch (after clean each time) and nothing has got broken. Seems to me that an eventual solution here is that your UI changes would eventually change aop.xml and that .classpath and pom.xml wouldn't need to duplicate that info. For now, I'd just leave the m2e AJ use case as is (if it works for what you need for Roo).
Move to the 2.0.2 release.
Try to solve for 2.1.0.
Determining what will be tackled for 2.1.1 release.
