Bug 252711

Comment 1 Andrew Niefer 2008-10-30 12:53:28 EDT

Created attachment 116539 [details]
test against head

Patch on HEAD pde.build.tests

I have not been able to reproduce this, it either succeeds or fails with an error depending on if the bundle is singleton.  See attached test which can be run as a junit plugin test.  You can either debug this test, or you can debug a full build by launching an Eclipse Application using the org.eclipse.ant.core.antRunner application and arguments like you would use on the command line:
 -buildfile ${resource_loc:/org.eclipse.pde.build/scripts/build.xml} -Dbuilder=...
This will then stop at breakpoints in the generator code.

getRegistry().getSortedBundles() is all resolved bundles.  If the bundle in question is a singleton, then this is only the highest version, otherwise all versions will be in there.  All of the "plugins" set should be in that list, otherwise we would have expected an exception from computerElements().

Therefore, the only reasons I can see for the empty all.plugins list is one of:
1) Bundle was not marked as compiled, or bundleProperties == null
2) There were no configs that the feature entry matched

Comment 2 Stephan Herrmann 2008-10-31 18:35:21 EDT

Created attachment 116670 [details]
Testcase reprocing the issue

I first had to debug the full build in order to find the cause,
but here finally is a test that actually reproduces the issue.
(Patch is against 3.4 maintenance but should be fine in HEAD, I hope).
The test is green, but if you catch the generated build.xml its
all.plugins target is indeed empty.

Background: we have to fiddle around with version qualifiers in order
to publish plugins from a branch of original eclipse plugins
(installed using a patch feature, but that's irrelevant here).

Given a plugin in the baseLocation called 
    a_3.4.2.v_833 
we are building a new plugin called 
    a_3.4.2.Branch_qualifier
where PDE nicely expands the qualifier. From experiments I learned 
that this combination is broken, whereas building 
    a_3.4.2.v_Branch_qualifier
on top of the v_833 thing is OK!
(Note that "v_" > "Br", i.e., which plugin is greater than the other
is only discriminated by the lexical comparison of the service segment).

From here you could say that this situation (patch has version < original)
just shouldn't occur. Granted. It was an accident on my side. But note
that the test doesn't even use a patch feature.

So, if you still want to look at the code:
When FeatureBuildScriptGenerator#computeElements() calls
PDEState#getBundle(String,String,boolean) a bundle with a similar version
is actually used and this bundle is not checked for resolution-success. 
Boom ;-)

The detection of similar qualifiers is actually a bit strange I'd say:
major,minor,micro must be identical; qualifer must be greater or equal
to the qualifier prefix from the request, mh? Instead of compareTo
I would have expected a startsWith, no? 
But that was not the cause of trouble in my case, just being puzzled.

As a side note: if you remove the plugin b_1.0.0 from the test you should
see a nice NPE in BuildDirector#generateModels(List) ;-}
Reason: Utils#extractPlugins finds two lists with disjoint contents,
but equal length => the initialList is returned, which feeds the wrong
plugin to the BuildDirector.

Comment 3 Andrew Niefer 2009-07-10 18:43:23 EDT

Thanks Stephan, sorry we did not get to this in 3.5

The bug is returning unresolved bundles from PDEState.getBundle when resolved==true.

Also, I agree that the matching of the qualifier is slightly wrong.  I have changed this logic to use a version range constructed by Utils.getVersionRange(String) which was introduced as part of bug 247091. 
       3.4.2.Branch_qualifier -> [3.4.2.Branch_, 3.4.2.Brancha)  
       ('a' is one character bigger than '_')
 
The test expects to fail with an "Another singleton version selected" error since the bundle is a singleton and v_833 gets resolved since it is the higher version.

The NPE should just go away now that getBundle is not returning unresolved bundles.  I have added a check just in case, the null set is handled the same way we were handling the empty set.

This turns out to be somewhat related to bug 84351.