Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 329375 - Support for arbitrary matching attributes on Require-Bundle/Fragment-Host causes bundle to not resolve
Summary: Support for arbitrary matching attributes on Require-Bundle/Fragment-Host cau...
Status: RESOLVED WONTFIX
Alias: None
Product: Equinox
Classification: Eclipse Project
Component: Framework (show other bugs)
Version: 3.7   Edit
Hardware: PC Windows 7
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: equinox.framework-inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 343843 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-11-03 12:55 EDT by Olivier Thomann CLA
Modified: 2011-04-26 09:50 EDT (History)
9 users (show)

See Also:


Attachments
new option to disable bundle attribute matching (11.11 KB, patch)
2010-11-03 15:03 EDT, Thomas Watson CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Olivier Thomann CLA 2010-11-03 12:55:19 EDT
I had osgi bundle from HEAD inside my workspace and I ended up not being able to run any launching configurations anymore.
Any Require-Bundle statements that incorrectly specify "version" attr will not resolve anymore. This seems to be the root cause of the issues I am having as if I removed osgi from my workspace, I can run self-hosting workspace again.

For some reason, it looks like 3.7 I-builds are less impacted by this change than 4.1 I-builds. There is not yet a 4.1 I-build that contains the version of OSGi bundle that is more restrictive on attributes being misused (bug 328508).
Comment 1 Olivier Thomann CLA 2010-11-03 13:00:15 EDT
It also looks like some orbit bundles don't have a consistent range that let them resolve.
org.apache.batik.css (1.6.0.v200912221622)
refers to the version range:
Require-Bundle: org.apache.batik.util;version="[1.6.0,1.7.0)",org.w3c.
 css.sac;version="[1.3.0,1.4.0)",org.w3c.dom.svg;version="[1.1.0,1.3.0
 )"
But the util bundle comes with version:
org.apache.batik.util (1.7.0.v200903091627)

So it should not resolve. I guess it used to because version was ignored.
Comment 2 BJ Hargrave CLA 2010-11-03 13:05:22 EDT
I think we just need to fix the broken manifests. It is clear the intention of the manifest is to match versions on the required bundles. However, the wrong attribute name was specified, so it was ignored. 

The change/clarification to the OSGi spec and thus Equinox will now root out these incorrect manifests which can then be fixed.

I am not sure what change to the framework this bug is requesting? The changes that need to be made are to the bundles with incorrect manifests.
Comment 3 Olivier Thomann CLA 2010-11-03 13:14:44 EDT
I think it would be good to have a temporary mode that can log which bundles are using the wrong attributes names and check the corresponding version range to see if they match.
Like that it would be easier to fix all orbit bundles that have wrong manifest attribute names and wrong version ranges.
Comment 4 BJ Hargrave CLA 2010-11-03 13:24:10 EDT
(In reply to comment #3)
> I think it would be good to have a temporary mode that can log which bundles
> are using the wrong attributes names and check the corresponding version range
> to see if they match.
> Like that it would be easier to fix all orbit bundles that have wrong manifest
> attribute names and wrong version ranges.

So some temporary code which would be remove before we finalize 3.7? Seems unusual. Perhaps a better solution is to use a tool (e.g. bnd) to analyze the manifests of the orbit bundles for problems  rather than stumbling over them one at a time in the runtime.
Comment 5 Thomas Watson CLA 2010-11-03 14:07:09 EDT
DJ opened bug329376 to fix up the orbit bundles.

One possibility is to have the framework alias the version attribute to bundle-version for Require-Bundle and Fragment-Host.  The misuse of the "version" attribute is going to be the most common mistake so we could try to be nice and alias that to bundle-version.  But even that is problematic because it sounds like some bad bundles don't even specify the correct ranges in their incorrectly specified "version" attributes.  So if we aliased them to bundle-version they would still not be able to resolve because their range is bad anyway.  Are we any better off?

The only option I see is to have a flag that disables arbitrary matching attributes for Require-Bundle and Fragment-Host.  It would be enabled by default.  PDE would still complain that the bundles cannot resolve in your workspace, but at least you can configure the runtime to allow the bad bundles to resolve.  Keep in mind that the bad bundles will be allowed to resolve to bundle-versions that the developer did not intend.  So either way we must fix all bad bundles in the end.
Comment 6 Thomas Watson CLA 2010-11-03 15:03:34 EDT
Created attachment 182320 [details]
new option to disable bundle attribute matching

Adds a new resolver option that can be used to disable bundle attribute matching.  Olivier can you try this patch out?

In your launch config set the following system property:

-Dosgi.resolve.bundle.attributes=false

The default is true so you must specify "false" any other value is treated as "true".
Comment 7 BJ Hargrave CLA 2010-11-03 15:10:36 EDT
How does this bug block bug329382? bug329382 needs to be fixed regardless of what happens to this bug.
Comment 8 BJ Hargrave CLA 2010-11-03 15:13:57 EDT
(In reply to comment #6)
> Created an attachment (id=182320) [details]
> new option to disable bundle attribute matching
> 
> Adds a new resolver option that can be used to disable bundle attribute
> matching.  Olivier can you try this patch out?
> 
> In your launch config set the following system property:
> 
> -Dosgi.resolve.bundle.attributes=false
> 
> The default is true so you must specify "false" any other value is treated as
> "true".

These sort of options are a bad idea. We don't even have a handle on the scope of the situation and we are proposing options that will violate the spec! It would seem that if there is such an option as this that the value of the option should be the BSNs of the bundles which are broken and should have their attributes ignored.

This option, as currently proposed, will break correct bundles which depend upon matching attributes working!
Comment 9 Thomas Watson CLA 2010-11-03 15:32:03 EDT
(In reply to comment #8)
> This option, as currently proposed, will break correct bundles which depend
> upon matching attributes working!

Only if you set the option to false!  The default behavior is to correctly pay attention to declared matching attributes.
Comment 10 BJ Hargrave CLA 2010-11-03 15:52:21 EDT
(In reply to comment #9)
> (In reply to comment #8)
> > This option, as currently proposed, will break correct bundles which depend
> > upon matching attributes working!
> 
> Only if you set the option to false!  The default behavior is to correctly pay
> attention to declared matching attributes.

Well of course. But as soon as you set it to false, any bundle which relies upon specified behavior will be broken. Seems a bad idea to have an option that helps broken bundles by breaking correct bundles. Your option will need to handle a mixed environment of broken bundles and bundles using the matching attributes.
Comment 11 Remy Suen CLA 2010-11-03 15:54:38 EDT
(In reply to comment #6)
> Created an attachment (id=182320) [details]
> new option to disable bundle attribute matching
> 
> Adds a new resolver option that can be used to disable bundle attribute
> matching.  Olivier can you try this patch out?

I can confirm that I can now run tests again with org.eclipse.osgi from HEAD after applying said patch and appending said argument to my VM arguments in my launch configuration.
Comment 12 Thomas Watson CLA 2010-11-03 17:03:08 EDT
This does not block bug 329382.  The manifests need to be fixed.

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > This option, as currently proposed, will break correct bundles which depend
> > > upon matching attributes working!
> > 
> > Only if you set the option to false!  The default behavior is to correctly pay
> > attention to declared matching attributes.
> 
> Well of course. But as soon as you set it to false, any bundle which relies
> upon specified behavior will be broken. Seems a bad idea to have an option that
> helps broken bundles by breaking correct bundles. Your option will need to
> handle a mixed environment of broken bundles and bundles using the matching
> attributes.

Sigh, if that is required I would rather do nothing.  The problem I have with complex configuration options is that we now need to manage the property each time a bad bundle is installed.  It is not that I disagree with your assessment.  It is not pragmatically anything that I think our adopters will ever be able to manage within their products.  If it is not easily managed and there is no easy way out then adoption of 3.7 could be lessened.

The number of bugs we have opened just for eclipse community bundles has me worried that products built on top of eclipse will be effected.  see:

bug329376
bug329361
bug329392
bug329382
Comment 13 Jeff McAffer CLA 2010-11-03 20:46:52 EDT
My 2c.  We should NOT make any special allowances for malformed headers.  My main assumption is that most people are using the tooling and the tooling has been doing the right thing. Those who have not will need to do more work. Doing special stuff in the framework just adds to bloat, increases the testing load and leads to confusion in what is right.

While I agree that some folks moving to 3.7 will be affected, I'm not that concerned about adoption per se. The fix is very easy for people and if we are really slick we could see about PDE providing quick fixes and the like in PDE for common cases (the things we would have aliased).

At the very least we should defer any work here until it is seen to be absolutely necessary (e.g., hundreds of important existing bundles failing to run on 3.7).
Comment 14 Paul Webster CLA 2010-11-04 07:52:14 EDT
If you want to maintain strict checking (I'm all for adhering to the spec) can the error message be improved?  bug 329376 comment #6

Right now when you get into this state, a bundle that loads fine in 3.5 and 3.6 will simply not work in 3.7, and possibly not even generate an error log.

PW
Comment 15 Olivier Thomann CLA 2010-11-04 08:32:59 EDT
I think the only thing we need to do is to find all bundles with malformed headers and fix them.
We should not accept malformed headers, but we need to make sure we have time to fix the malformed headers in bundles.
Comment 16 Thomas Watson CLA 2010-11-04 09:19:47 EDT
(In reply to comment #14)
> If you want to maintain strict checking (I'm all for adhering to the spec) can
> the error message be improved?  bug 329376 comment #6
> 
> Right now when you get into this state, a bundle that loads fine in 3.5 and 3.6
> will simply not work in 3.7, and possibly not even generate an error log.
> 
> PW

We could decide to log every time we find unresolved bundles.  This will cause log entries even in cases that are valid.  For example, there are a few bundles in the SDK that require Java SE 6.  When on Java SE 5 you will not get unresolved errors in your log.  This has caused issues in the past.
Comment 17 Olivier Thomann CLA 2010-11-04 13:33:30 EDT
I quickly checked the 4.1 bundles with a small tool I wrote.
I found that:
org.apache.batik.css
has wrong attribute names for required bundles: org.apache.batik.util, org.w3c.css.sac and org.w3c.dom.svg.

org.apache.batik.util
has wrong attribute names for required bundle: org.apache.batik.util.gui

and org.w3c.dom.svg
has wrong attribute names for required bundle: org.w3c.dom.smil

and only the range for required bundle org.apache.batik.util in org.apache.batik.css is wrong according to the provided bundles.

So we don't need to get a lot of orbit bundles fixed to be able to run 4.1 builds.
Comment 18 Olivier Thomann CLA 2010-11-04 13:33:56 EDT
Hopefully I didn't miss any, but I cannot be 100% sure.
Comment 19 David Williams CLA 2010-11-10 00:48:03 EST
For what its worth, a "quick test" check was made on all bundles in Orbit and Indigo M2 repo, and the broken bundles fixed (most related to the "batik family". See bug 329376. 

But still, I as worried that there was no "escape" flag to restore old, broken behavior, so there would not be true "binary compatibility" if some "old product" tried to move onto new platform. 

So, I checked both the Helios repo's and several versions of some large several-thousand-bundle adopter products and there were no new findings, with one exception. Back in the Galileo stream (I think) these two jars also had invalid manifest: 

org.eclipse.ocl_1.2.0.v200806091438.jar
org.eclipse.ocl_1.2.3.v200901071407.jar

but, seems long ago fixed in helios stream (if not also Ganymede? didn't check there) 

org.eclipse.ocl_3.0.1.R30x_v201008251030.jar

So ... I'm less worried. I do not think it is a wide spread problem. 

My little investigation is no guarantee, of course, but I do not think "we" (i.e. you) need to do anything preemptive to allow old broken behavior to be restored, and can safely wait to see if anyone runs into problems. Seems unlikely.
Comment 20 Jeff McAffer CLA 2010-11-10 08:46:32 EST
Great.  Thanks David for doing that investigation.
Comment 21 Thomas Watson CLA 2010-11-10 09:06:32 EST
Thanks David.

Renaming the bug to reflect the issue.  The patch attached may be used to disable this new feature.  I agree we should avoid adding such an option until it is proven to be an real issue.

My main concern is any products that pulled in the batik bundles from Helios will be broken if they upgrade the core framework to Indigo without picking up the fixed batik bundles from Indigo.  Is that not realistic?  Would they always get the fixed batik bundles from Indigo?
Comment 22 Thomas Watson CLA 2011-02-17 16:32:08 EST
We will not add this option.  If this proves to be an issue in practice then we can reopen the bug to reconsider adding the option.
Comment 23 Thomas Watson CLA 2011-04-26 09:50:57 EDT
*** Bug 343843 has been marked as a duplicate of this bug. ***