Bug 217442 - [aspects] Eclipse-Supplement supplements more bundles than it should
Summary: [aspects] Eclipse-Supplement supplements more bundles than it should
Status: RESOLVED FIXED
Alias: None
Product: Equinox
Classification: Eclipse Project
Component: Incubator (show other bugs)
Version: unspecified   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: equinox.incubator-inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords: contributed
Depends on:
Blocks:
 
Reported: 2008-02-01 09:33 EST by Avatar CLA
Modified: 2009-04-29 15:29 EDT (History)
3 users (show)

See Also:


Attachments
Patch which solves the problem (2.51 KB, patch)
2008-02-20 10:22 EST, Avatar CLA
mlippert: iplog+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Avatar CLA 2008-02-01 09:33:51 EST
Build ID: M20071023-1652

Steps To Reproduce:
 


More information:
Methods Eclipse-SupplementExporter and Eclipse-SupplementBundle (I use these two) often propagate aspects configuration (aop.xml) to other bundles, that should not be weaved. It denies for example using pointcut within(*) because it will be propagated and used in other bundles, not exactly in the ones declared as Eclipse-Supplement.
For example log check:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=217037
Comment 1 Martin Lippert CLA 2008-02-05 18:26:31 EST
Can you provide some more details on this? Currently I am not able to find out which bundles are supplemented in the wrong way.
Comment 2 Martin Lippert CLA 2008-02-13 15:15:59 EST
Wojciech, can you check if this observation is still true using the new supplementing mechanism?
Comment 3 Avatar CLA 2008-02-18 11:32:57 EST
I've found origin of this problem.
We often use in our bundles' manifests line:
DynamicImport-Package: *
If there is such line in manifest, bundle see all aop.xml files in all bundles and  is weaved by them.
It'd be nice to ommit all dynamic imported packages and weave only bundles which have statically imported package/bundle with aop.xml (or which uses eclipse-supplement convention).
Comment 4 Martin Lippert CLA 2008-02-18 11:45:01 EST
This seems to be a tricky problem. The aspect weaving implementation uses the standard classloader mechanisms to load the aop.xml files. Therefore dynamically imported packages are found as well.

Do you really need to use "Dynamic-ImportPackage: *" ?
Comment 5 Avatar CLA 2008-02-20 10:22:12 EST
Created attachment 90192 [details]
Patch which solves the problem

Yes we need to use dynamic imports. But I solved problem, by changing OSGiWeavingContext class. It's partially a hack, but consider using it.
Comment 6 Martin Lippert CLA 2008-02-20 15:58:11 EST
You are right. I thought about this problem a lot too complicated.

Since aspect bundles are added to the bundle as a required bundle (supplementer-mechanism) its enough to check if the aop.xml files that are used to configure the weaver are coming from required bundles.

Comment 7 Martin Lippert CLA 2008-02-20 16:00:55 EST
I need to check some other usage scenarios and try to solve this problem using your patch. Thanks a lot for your contribution!!!

Comment 8 Chris Aniszczyk CLA 2008-02-20 16:58:56 EST
btw, I noticed your BREE was set to:

+Bundle-RequiredExecutionEnvironment: J2SE-1.3,J2SE-1.4,J2SE-1.5

This is wrong, it should just be J2SE-1.3, a BREE implies minimum execution env
Comment 9 Martin Lippert CLA 2008-02-21 16:01:51 EST
Ah, okay. Will fix this. Thanks!!!
Comment 10 Avatar CLA 2008-02-27 05:18:54 EST
I found problem with my patch. If bundle contains aop.xml, it isn't used to weave own classes. But it's easy to repair. Just modify the following code:

String bundleName = getBundleIdFromURL(xml);
for (int i=0; i<requires.length; i++){

by adding right "if", just like that:

String bundleName = getBundleIdFromURL(xml);
if (bundle.getSymbolicName().equals(bundleName)){
	modified.add(xml);
	continue;
}
for (int i=0; i<requires.length; i++){

It works much better.
Comment 11 Martin Lippert CLA 2008-02-27 17:42:14 EST
Hi Wojciech,

I tried your patch and your small addition to the patch and it looks good. I added the handling of fragments to it (needed for opt-in model). Will test some more stuff with it and release the code then. I am not sure if we need to add handling of imported (not dynamically imported) packages, but I will find out.

Thanks again for your contribution!!!
Comment 12 Martin Lippert CLA 2008-03-05 07:07:57 EST
Fixed - code releaed into HEAD.

Applied the contributed path with additional fragment handling.