Community
Participate
Working Groups
When in a feature a plug-in version increases but the qualifier decreases, the generated feature qualifier will decrease causing some problems. Example: initially the plugin is versionned 3.2.0.v2006 in the next iteration it is versionned 3.2.1.r321 We should consider the version number in the computation of the feature qualifier.
Assigning to David as he agreed to fix this bug.
My plan to fix this is as follows. Add up separately the major, minor, and service parts of the version numbers of all the plugins and included features of the feature in question. Encode each of those sums as a base 64 string, and prefix each one with the length of the encoded string. Put those strings at the beginning of the qualifier suffix. Including the length in the encoded string allows for variable length strings such that larger numbers always result in lexicographically greater strings. This avoids overflow problems that a fixed length string would have and uses short strings to encode small numbers. I would expect that in the typical PDE Build case each part of the version number would be encoded in one or two characters, and virtually never more than three characters.
Created attachment 54951 [details] Patch with new qualifier suffix algorithm Here is a patch with my rewrite of the qualifier suffix algorithm. It fixes this bug plus #162020.
David does this patch also handles bug #162022?
I have not dug into the details of the algorithm. I'll leave this fun to Andrew :) However from a quick glance I have a few questions: - I did not see the place where we automagically add the version number of the algorithm. - Move the declaration of the 64 bits chars to the top of the class and make it final. - Format the code.
No, this patch does not contain a fix for bug #162022. Fixing that would result in much longer suffixes for any feature that includes another feature. I am waiting to get some agreement that the cost of the longer suffixes is worth fixing the problem. The version number of the algorithm is not added into the suffix because the version number of the algorithm at this point is zero. (Including the algorithm version can increase the length of the suffix, so there is an incentive to keep the algorithm version as small as possible.) The static string has been moved to the top of the class as suggested. I'll submit a new patch shortly, once any other issues are cleared up.
Even if the version number is 0.0.0 for the current version of the algorithm, it is necessary to include the constant and add it to the computation because otherwise, the next time we will change the algorithm we will not think about adding it.
Created attachment 55704 [details] Patch with new qualifier suffix algorithm A new patch for the fix. This patch is against a more recent version of the file, and incorporates all of Pascal's suggestions. There are no functional changes, just formatting changes and cleaning up the code in a couple places.
I'm assuming the patches have been applied to the 322 base builder too? Namely, r322_v20070104 (but not r321_v20060824). I'm asking, because we did see this glitch again, using the r321 base builder, so I'll try updating to the r322 builder and re-run the build. For the error, if you look soon (e.g. today) see the Versioning Information link on http://download.eclipse.org/webtools/committers/drops/R1.5/M-1.5.3-200701181259/ Namely http://download.eclipse.org/webtools/committers/drops/R1.5/M-1.5.3-200701181259/versioningReport_1.5.3.html (This build link may not exist after tomorrow, 1/19/2007)
This patch has not yet been applied. I will try and find some time soon for it. It is at the top of our list for pde.build in M5.
Even if this patch had been already been applied, it would have been for 3.3 only, not for 3.2.2. This is too big of a change for a maintenance release. It will cause the qualifier suffix (and therefore the version number) of every single feature to change.
Comments on patch: 1) lengthPrefixBase64, we should be able to get the length without a for loop, I think it is (for a 0-based length): int length = (number >> 6) + (((number % 64) < 8) ? 0 : 1); 2) Even though charValue is returning 1-64, I believe we still want to normalize later with base 64, not base 65. Doing it base 65 effectively just adds characters that could have been rolled in with the carry. 3) Regarding bug 162022, I wonder if we can handle this by treating the feature version as 2 qualifiers. ie, the regular qualifer and the suffix: 3.2.0.v2006_abcd123 becomes 3.2.0.v2006 (represents the feature) and 0.0.0.abcd123 (represents nested plugins)
> 1) lengthPrefixBase64, we should be able to get the length without afor loop, > I think it is (for a 0-based length): > int length = (number >> 6) + (((number % 64) < 8) ? 0 : 1); I don't see how you can avoid a loop of some sort (or a bunch of nested if statements). Your suggestion doesn't give the desired value for length. What we want is: 0 <= number < 8 (2^3) --> length = 0 8 <= number < 0x200 (2^9) --> length = 1 0x200 <= number < 0x8000 (2^15) --> length = 2 0x8000 <= number < 0x200000 (2^21) --> length = 3 0x200000 <= number < 0x8000000 (2^27) --> length = 4 etc. I don't know how to do that in a single line of code without an intergral log function. > 2) Even though charValue is returning 1-64, I believe we still want to > normalize later with base 64, not base 65. Doing it base 65 effectively just > adds characters that could have been rolled in with the carry. I spent a *long* time thinking about that one, and tried every way imaginable to normalize as base-64 rather than base-65. It can't be done. The input has to have 65 values, to distinguish between a trailing '-' and shorter qualifier (problem (3) of bug 162020). If that is normalized as base-64, then problem (1) of bug 162020 can still happen. If we had a fixed-length string, then it would be possible to convert the entire string, as a whole, from base-65 to base-64. But with a variable-length string that isn't possible, because the characters at the beginning of the result string would be influenced by the length of the longest qualifier. The cost of outputting 65 values using a scheme with just 64 possible characters at each position is that two of the 65 value result in a two-character representation. This will result in slightly longer qualifier suffixes, but I cannot think of any way to avoid that while still being correct. > 3) Regarding bug 162022, I wonder if we can handle this by treating > the feature > version as 2 qualifiers. ie, the regular qualifer and the suffix: > 3.2.0.v2006_abcd123 becomes 3.2.0.v2006 (represents the feature) > and 0.0.0.abcd123 (represents nested plugins) That's not a bad idea. It won't solve the problem completely. The question is whether it will help in enough situations. The problematic situation is when a plugin is removed from the included feature. In that case, the context part of the qualifier will increase, but the suffix part of the qualifier will likely decrease. The algorithm in the patch uses only the suffix (as if the included feature's version were 3.2.0.abcd123), so the outer feature's suffix will most likely decrease. With Andrew's suggested change, the increasing context will contend with the decreasing suffix for the overall directing of the outer feature's suffix. I think the decreasing suffix will usually win (because the change is more likely to be in the first couple digits, where the context change is usually toward the end). But the right thing will happen in some cases, and I can't think of any drawbacks, so I am inclined to make this change.
(In reply to comment #13) > > 1) lengthPrefixBase64, we should be able to get the length without ... > I don't see how you can avoid a loop of some sort (or a bunch of nested if > statements). Your suggestion doesn't give the desired value for length. Yes, your right, sorry. > > 3) Regarding bug 162022, I wonder if we can handle this by treating ... > > 3.2.0.v2006_abcd123 becomes 3.2.0.v2006 (represents the feature) > > and 0.0.0.abcd123 (represents nested plugins) > > That's not a bad idea. It won't solve the problem completely. Actually, now that you've pointed this out, I'm not sure that it will ever solve the problem. With the major.minor.service versions encoded at the beginning of the suffix, the suffix will always decrease in the most significant digits when a plugin is removed. With the practice of using the date in the feature qualifier, it will generally increase in the least significant digits. (Out of order because I have to disagree on this one:) > > 2) Even though charValue is returning 1-64, I believe we still want to > > normalize later with base 64, not base 65. Doing it base 65 effectively > > just adds characters that could have been rolled in with the carry. > > I spent a *long* time thinking about that one, and tried every way imaginable > to normalize as base-64 rather than base-65. It can't be done. The input has > to have 65 values, to distinguish between a trailing '-' and shorter qualifier > (problem (3) of bug 162020). If that is normalized as base-64, then problem > (1) of bug 162020 can still happen. As I see it, doing the sums with values 1-64 is sufficient to address 162020-3. 162020-1 seemed to be caused by wrong values for the shift in the original implementation, this is fixed by the use of the BASE_64_ENCODING string. Both of these problems were really about getting the correct values in the qualifierSums (which I believe we now have). The normalization step is more about encoding those sums into a string. I suggest that the appendEncodedCharacter is in fact equivalent to a normalization base 64, with the difference that the "carry" values are encoded in separate characters instead of being added to the more significant digit. I see the normalization based 65 followed by the appendEncodedCharacter just being a less efficient way of normalizing directly to base 64. Base 64: 1.2.3.aaa-0z-aaa -> 012-bbb02-0bbb 1.2.3.aaa-10-aaa -> 012-bbb0210bbb Base 65: 1.2.3.aaa-0z-aaa -> 012-bbb01z00bbb 1.2.3.aaa-10-aaa -> 012-bbb0210bbb
If we change the qualifiers in the example, we can get a case of the qualifier suffix being unchanged or going backwards when using base-64 normalization. Base 64: 1.2.3.aaa-0z-aaa -> 012-bbb02-0bbb 1.2.3.aaa-1 -> 012-bbb02 <== **backwards** Base 65: 1.2.3.aaa-0z-aaa -> 012-bbb01z00bbb 1.2.3.aaa-1 -> 012-bbb02 <== OK (These suffix computations were done in my head. I hope I got them right.) Triggering this behavior requires that a qualifier change from a 'z' to nothing in one position (i.e. it has to get shorter) while increasing exactly one character in the previous position. I prefer to err on the side of correctness. (Algorithms that fail in just a few edge cases can be extremely annoying and hard to debug if you happen to hit one of those edge cases.) But since this failure mode is extremely rare and almost guaranteed to not happen in real life (given current practices for creating plugin qualifiers), I could live with base-64 normalization. If we switch to base-64 normalization, we'll have to make sure the overflow character is handled correctly, in particular when the overflow character itself overflows.
Alright David, you've managed to shoot down everything I've come up with so far, so I've gone ahead and released this to HEAD. First build for M5 is monday, so it would be good to try this out maybe with tonight's nightly build before we tag.
