Bug 250942 - Incorrect character ordering in feature qualifier
Summary: Incorrect character ordering in feature qualifier
Status: RESOLVED FIXED
Alias: None
Product: PDE
Classification: Eclipse Project
Component: Build (show other bugs)
Version: 3.4.1   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: 3.5 M3   Edit
Assignee: pde-build-inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords: noteworthy
Depends on:
Blocks:
 
Reported: 2008-10-15 09:51 EDT by David Olsen CLA
Modified: 2008-12-02 17:08 EST (History)
3 users (show)

See Also:


Attachments
patch (8.54 KB, patch)
2008-10-20 18:12 EDT, Andrew Niefer CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Olsen CLA 2008-10-15 09:51:58 EDT
When comparing version numbers to determine which one is greater, Eclipse does a simple string compare when looking at the qualifier part of the version number.  (See org.osgi.framework.Version.compareTo() in plugin org.eclipse.osgi.)  In that ordering, an underscore is in between the capital letters and the lower case letters, i.e. 'Z' < '_' < 'a'.

But when creating the feature qualifier suffix (that hash-like string at the end of a feature's version number), the underscore character is treated as if it were between the digits and the capital letters, i.e. '9' < '_' < 'A'.  (See org.eclipse.pde.internal.build.Utils.BASE_64_ENCODING and how it is used in plugin org.eclipse.pde.build.)

This bug can result in feature version numbers that go backwards when plugin version numbers are increased correctly.  That can result in the update manager refusing to install the newer build of a feature because it thinks the newer build has a lower version than what is already installed.

As a specific example, someone at our company changed a plugin version number from 7.2.0.v20081013 to 7.2.0.v20081014.  When that happened, the feature version number went from 7.2.0.v20080908-7E2-9oA55S7D9_ to 7.2.0.v20080908-7E2-9oA55S7D9A.
Comment 1 Alan Boxall CLA 2008-10-15 11:20:35 EDT
I think this should be a higher severity.

In the product that I am working on there are 350+ features
Of those features 230+ use the feature qualifier version feature of PDE.
A quick count shows about 30 have "_" in the portion of the qualifier added by PDE.

I think there is a good chance of this bug causing a feature to not install especially since many components rely on this important feature.
Comment 2 David Olsen CLA 2008-10-15 14:19:47 EDT
Given that the bug has been there for two and a half years without being noticed, I don't think the severity needs to be raised.  The times when this bug causes actual grief are rare enough and the workaround is easy enough.

Fixing this needs to be done carefully.  The quick fix of changing just BASE_64_ENCODING to have the correct ordering would cause almost all feature version numbers to change when a new PDE Build is adopted, and would cause some of them to go backwards.  So I think the fix should happen in Eclipse 3.5 only, to prevent feature version numbers from changing in people's maintenance streams.  BuildDirector.QUALIFIER_SUFFIX_VERSION should probably be increased at the same time.  But even that won't guarantee that changed version numbers will increase, since fixing BASE_64_ENCODING would change the way that QUALIFIER_VERSION is encoded.
Comment 3 Andrew Niefer CLA 2008-10-15 15:48:48 EDT
In 3.5, the BASE_64_ENCODING is also used to construct a range which build uses to choose a feature or plugin when replacing qualifiers. (This came out of bug 247091)

Wrong range: 3.2.1.v1_qualifier -> [3.2.1.v1_, 3.2.1.v1A)
Right range: 3.2.1.v1_qualifier -> [3.2.1.v1_, 3.2.1.v1a)

The incorrect range [3.2.1.v1_, 3.2.1.v1A) is invalid since v1_ > v1A, this will result in a build failure.  This will likely be much more common than the problem with suffix ordering.


As far as version suffix ordering, I'm not sure that QUALIFIER_SUFFIX_VERSION is at all useful as it stands.  It is encoded together with the major sum and quickly becomes insignificant.  

For it to be useful, it should be encoded separately as the first couple of characters.  For this change itself to be an increase over the old method, we should store the version unencoded, and choose a value such that the encoded sum of the major versions is likely to be smaller.
For example:
lengthPrefixBase64((big # of plugins in a feature)*(high major version number))
lengthPrefixBase64(50000 * 1000) = Wyj1-

Therefore choosing a version "a0" should be an increase over the suffix for any reasonable feature.  (All future version should then be restricted to 2 characters).

This would of course change all suffixes, and build teams should then only pick up the new pde.build at a point where incrementing all feature version numbers is not unreasonable.  (ie, not in point releases).
Comment 4 David Olsen CLA 2008-10-15 17:04:59 EDT
When QUALIFIER_SUFFIX_VERSION was created, we were working hard to limit the length of the qualifier suffix.  Having QUALIFIER_SUFFIX_VERSION incorporated into the sum of the major versions has the advantage of usually not increasing the length of the suffix.  It still allows the qualifier to always increase when changing the suffix algorithm as long as the encoding scheme for that first part of the suffix doesn't change.  Unfortunately this fix will change the encoding scheme for that first part of the suffix.  It seemed like a reasonable decision at the time, but it is coming back to bite us now.

Having the qualifier suffix version be an explicit two character string will work better at managing changes to the suffix algorithm.  But it will lengthen every qualifier suffix by two characters.
Comment 5 Andrew Niefer CLA 2008-10-15 18:03:43 EDT
I am reluctant to lengthen the qualifier more than it already is.
We do have problems when the suffixes are truncated to a certain length and we lose small increments.  This happens every once in a while during milestone weeks, see for example bug 247749.

We are considering in bug 208143 for the build to take a list of versions from the previous build as "advice" and using it to ensure the suffix increases.

I think I prefer leaving the qualifier version strategy as it is.  It has simply be too long since I've thought about this in depth :)  It may be better than I had first feared, even though it may be insignificant compare to the major versions, it is an increase.  Any disruptive changes to the major versions should be accompanied by a feature retag.

We can increment the version, and anyone unlucky enough to have this land on the '_' after encoding can just retag their features.
Comment 6 Andrew Niefer CLA 2008-10-20 18:12:24 EDT
Created attachment 115638 [details]
patch
Comment 7 Andrew Niefer CLA 2008-10-20 18:16:34 EDT
I have released the attached patch.  I added tests including old corner cases I took from bug 162020 (which may no longer be very relevant).  

I don't want to mess with the suffixes very much, so am leaving the versioning as is.  We will add a comment to the new & noteworth for M3 for builders adopting the new build.