The current version scheme that's supported is x.y.z.w(+), which is very limiting. Also, the "+" has no useful meaning -- it just means "some version greater than this one". Suggest we drop the plus (services code still should handle it), and allow an arbitrary number of numerical components. For good measure, toss in an epoch number at the start, in case we ever need to change versioning schemes. (If not specified, it should be "0"). So: [K:]a.b.c.d.e..... where K would be optional, and any components after a optional. Any components that are not specified should be treated as 0's when comparing versions.

<vlad> it's not clear what people want to accomplish with any versioning scheme <brendan> vlad: partial order <brendan> vlad: also the ability to insert versions between well-known fenceposts <brendan> arbitrarily <vlad> brendan: to insert versions between you need to have an arbitrary number of components <vlad> that part's easy <vlad> brendan: I'm not sure what you mean by partial order <brendan> vlad: reflexive, antisymmetric, transitive <vlad> brendan: ok <vlad> brendan: but neither of those things lets you handle the case of "we have 1.0 out now, we want to release previews of 1.1, but we don't want to call them 1.1" <vlad> which is why the linux crowd does even/odd, or at least gives themselves an extra # in between there <brendan> vlad: don't care about that case <vlad> no? <vlad> that's what the + supposedly solves <brendan> but it doesn't <vlad> if we don't care about that (and I don't think we should, because you can "solve" it via policy) <vlad> right <vlad> in that case, you just want to ditch the plus <brendan> but add arbitrary depth <vlad> allow arbitrary number of components <vlad> and possibly add support for epochs <vlad> all of which is backwards compatible <brendan> indeed -- and fix UMO/AMO <brendan> someone file a bug

When should we do this? Before beta, or after, but before first RC? /be

I'd say before beta (as soon as possible). If we want to have the final product called "1.1", we're going to have to call the first beta build 1.0.99.1 or similar.

Benjamin, you were mentioning some 1.1.-100 scheme on IRC today. /be

One rule for which I argued in bug 297295 was "a dot means a branch". Benjamin's idea preserves that; the arbitrarily dotted number scheme does not. So I am now questioning the value of this rule. Thoughts? /be

There is already real code deployed that assumes that versions matching "1.0." correspond to the 1.0 firefox release branch. You will break people badly if you use "1.0." as a prefix for anything other than the 1.0 release branch. How about we call the next firefox release 1.5, and allow 1.1.x to mean the beta release? Wait, we've had this discussion already haven't we? What's wrong with the "+" again? :-/ Have you solved the "marketing wants to control the version number" problem?

I intend to implement my negative-number support for b4, and move the code into XPCOM glue so that the GRE-finding code can use it for xulrunner. I can use this bug to do that, or file a separate bug. On the question of policy, I would like us to consider revving the app.extensions.version for every 1.1.x, but extension authors should have the opportunity to release extensions *now* with a maxVersion of 1.1.999. Then, if we discover that an extension has become incompatible by some absolutely-required security fix, UMO can "downrev" the maxVersion appropriately. This may require some small additional knowledge by EM that it should check for extension updates on upgrade even when the currently installed extensions appear to be fine (also related to the extension blacklist).

> On the question of policy, I would like us to consider revving the > app.extensions.version for every 1.1.x, but extension authors should have the > opportunity to release extensions *now* with a maxVersion of 1.1.999. > > Then, if we discover that an extension has become incompatible by some > absolutely-required security fix, UMO can "downrev" the maxVersion What if the incompatible extension sets maxVersion to 1.1.999? I think we cannot avoid having to preserve binary compatibility on release branches. I understand that it may be valuable to allow extension authors to have the ability to rev their extension with each security update of firefox, but I think you'd be hard pressed to find an extension author that would enjoy having to re-enter development each time Mozilla wants to push a security update to firefox. Instead, extension authors want some promise of stability on the release branch. We want this too, so we can *quickly* deploy security updates without having to worry about informing each extension author to give them ample time to go through development, testing, and release cycles *prior* to the firefox security updating going live. We really don't want to go there.

> What if the incompatible extension sets maxVersion to 1.1.999? I think we What I mean is that they *should* do that, and we should try our darnedest not to break them. But if we do break them, AMO can go back "after the fact" and at least patch the symptoms.

> What I mean is that they *should* do that, and we should try our darnedest not > to break them. But if we do break them, AMO can go back "after the fact" and at > least patch the symptoms. OK. I get what you're saying. "them" of course means *every* extension, which means that it would be incredibly bad to ever exercise this mechanism. So, why implement it? Why not just cut a 1.2 branch off the 1.1 branch if it ever came to that point?

Darin, I still think you're misunderstanding me: the AMO bits would only affect the particular extension that was broken (by downgrading its maxVersion arc), all other extensions would remain at 1.1.999.

Ah, you're only talking about a solution that works for extensions hosted by AMO. I see. Sorry for pummeling you with "my message" ... it does make sense for EM to phone-home for each installed extension when the application is updated. I'm not sure quite how the UI would work for that though. It would be a poor user experience if silent update still resulted in extension compatibility checking UI. Anything we can do about that?

> Ah, you're only talking about a solution that works for extensions hosted by > AMO. No, I guess this would work for anyone who wanted to implement similar logic given that EM phones home on app update. I'm still worried about the UI.

(In reply to comment #6) > There is already real code deployed that assumes that versions matching "1.0." > correspond to the 1.0 firefox release branch. You will break people badly if > you use "1.0." as a prefix for anything other than the 1.0 release branch. Hmm, good point. I guess we have to worry about that breakage for beta releases. > How about we call the next firefox release 1.5, and allow 1.1.x to mean the beta > release? Wait, we've had this discussion already haven't we? What's wrong with > the "+" again? :-/ We could also call it 1.2, which should be equivalent to 1.1 from a marketing perspective. I'm fine with either, but as you say, we've had that discussion before ;) The problem with the "+" is that it has no meaning from a versioning perspective. "1.0+" just means some version past 1.0 -- it doesn't tell you anything about where that version comes in relation to a 1.1 or even a 1.2 or whatever. All of my Deer Park nighlies show up as "1.0+", even though they're very different actual builds... > Have you solved the "marketing wants to control the version number" problem? Nope, but I think a better versioning scheme lets us allow them to control it as well, as long as they give us a free minor version number in between that we can use :)

Note that if I'm reading this code in the update service right, the in-app stuff does the same thing I did for the u.m.o version comparison code -- the "+" is just treated as an extra ".1" at the end of the 4-component version number. This means that right now, "1.0+" is less than "1.0.5", which is nonsensical..

(In reply to comment #15) > the same thing I did for the u.m.o version comparison code -- the "+" is > just treated as an extra ".1" at the end of the 4-component version number. > This means that right now, "1.0+" is less than "1.0.5", which is nonsensical.. If you need to emulate it numerically the "+" seems to be more like padding out the version with MAXINT (1.0+ = 1.0.MAXINT.MAXINT, 1.0.5+ = 1.0.5.MAXINT) But I wouldn't cry if we got rid of the plus and did something numeric like an odd-even/dev-release numbering scheme for the minor digit. Stable branch sub-releases like 1.0.x should still increment by one though.

(In reply to comment #16) > (In reply to comment #15) > > the same thing I did for the u.m.o version comparison code -- the "+" is > > just treated as an extra ".1" at the end of the 4-component version number. > > This means that right now, "1.0+" is less than "1.0.5", which is nonsensical.. > > If you need to emulate it numerically the "+" seems to be more like padding out > the version with MAXINT (1.0+ = 1.0.MAXINT.MAXINT, 1.0.5+ = 1.0.5.MAXINT) That's what we thought it did, but if you read the EM code, it seems that it's more like 1.0+ = 1.0.0.0.1, which is < 1.0.5.0.0 (aka 1.0.5). Are we misreading the code? /be

It has been my experience that update's version check code considers an item as compatible when installing an extension with a targetApplication maxVersion of 1.0.5 when using 1.0+ which would mean 1.0+ is < 1.0.5.0 Also note, the version check code in update only allows for major, minor, release, build, and the plus. I also noticed a typo in the code where it should read: return this.major + "." + this.minor + "." + this.release + "." + this.build + (this.plus ? "+" : ""); http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/update/src/nsUpdateService.js.in#2203 I doubt that version toString is used for anything critical.

How about incrementing the number previous to the + and set the value of plus to -1? Then a version with a + that ties would be less than and when 1.0.5 is compared to 1.0+ then 1.0+ would be greater than. This would break toString for the prototype but that is already broken. Something like this: var plus = 0; if (aVersion.charAt(aVersion.length-1) == "+") { aVersion = aVersion.substr(0, aVersion.length-1); plus = -1; } var parts = aVersion.split("."); if (plus = -1) parts[parts.length - 1] = this._getValidInt(parts[parts.length - 1]) + 1; etc.

I don't really see the need for the +; if we change the scheme, we should just drop it. The problem is mainly how past versions of apps will handle any new versioning scheme, but even that I think is straightforward -- we simply stop using the + and say that old versions are just "x.y.z.w", and all new versions we give an epoch of "1" to (after all epochs are supposed to be used to help change versioning schemes...). That gives our update services an easy way to distinguish between old-style clients and new-style.

(reassigned to bsmedberg due to him claiming to be rewriting the version checker) When this is done, we will need some scheme to store human-readable version numbers in the appropriate spots so people don't become confused. I filed https://bugzilla.mozilla.org/show_bug.cgi?id=301250 on this issue.

I believe that this code is backwards-compatible with basically all of our present requirements.

Comment on attachment 189814 [details] [diff] [review] Move version-checking capabilities into XPCOM, rev. 1 >Index: xpcom/glue/nsVersionChecker.cpp >+PRInt32 >+CompareVersions(const char *A, const char *B) >+{ >+ char *A2 = strdup(A); >+ char *B2 = strdup(B); >+ >+ if (!A2 || !B2) { >+ // OOM is hard >+ return 1; What if only B2 is null? Then you'd leak A2. >+ if (OnlyZeros(a)) >+ result = 0; >+ else >+ result = 1; nit: result = !OnlyZeros(a); >+PRInt32 NS_COM_GLUE >+CompareVersions(const char *A, const char *B); I think this function should be prefixed with NS_ so as not to stomp on the namespace of consumers. (i.e., NS_CompareVersions) >Index: xpcom/base/Makefile.in >+ nsCOMVersionChecker.cpp \ How about taking this opportunity to replace "version checker" with "version comparator" since that's more clearly says what this thing does. Also, I'm not sure I like the nsCOM prefix to this file and class name. It is pretty inconsistent with the rest of the classes in xpcom/. How about nsVersionComparatorImpl.cpp instead for the XPCOM component? >Index: xpcom/base/nsCOMVersionChecker.cpp >+nsCOMVersionChecker::Compare(const nsACString& A, const nsACString& B, >+ PRInt32 *aResult) >+{ >+ *aResult = CompareVersions(PromiseFlatCString(A).get(), >+ PromiseFlatCString(B).get()); nit: indentation >Index: xpcom/base/nsCOMVersionChecker.h >+#define NS_VERSIONCHECKER_CONTRACTID "@mozilla.org/xpcom/version-checker;1" >Index: xpcom/base/nsIVersionChecker.idl >+/** >+ * Version strings are dot-separated sequences of base-10 numbers. e.g. >+ * 1.0.4, 1.5.10, 1.-20 I'm not sure about this negative parts thing. What is the use case? Is "1.-20 > 0.99" a true statement? >+ * >+ * As a backwards-compatibility measure, numbers may be suffixed with >+ * a letter or combination of letters. A version number with a suffix is >+ * "less than" an unsuffixed number, and suffixes are compared bytewise. >+ * >+ * 1.0a < 1.0 >+ * 1.0a1 < 1.0a2 < 1.0b >+ * 1.0a10 < 1.0a2 !!! CAUTION This seems unfortunate to me. Why not implement 1.0a10 > 1.0a2? Also, is 1.0a1 = 1.0A1 ? >+ * >+ * For additional backwards compatibility, a version number may be >+ * suffixed with "+", which increments the last version-part and adds "a" >+ * >+ * 1.0+ is equivalent to 1.1a >+ * >+ * Although not required by this interface, it is recommended that >+ * version-part numbers remain within the limits of a signed char, i.e. >+ * -127 to 128. >+ */ This practically screams for a unit test. Please add one to xpcom/tests. >+[scriptable, uuid(e6cd620a-edbb-41d2-9e42-9a2ffc8107f3)] >+interface nsIVersionChecker : nsISupports >+{ >+ /** >+ * Compare two version strings >+ * @param A The first version >+ * @param B The second version >+ * @returns < 0 if A < B >+ * = 0 if A == B >+ * > 0 if B > A Shouldn't the last one be "A > B"? typo, right? >+ */ >+ long compare(in AUTF8String A, in AUTF8String B); Why UTF-8? Why not ASCII (i.e., ACString) instead? >Index: toolkit/mozapps/update/public/nsIUpdateService.idl >-interface nsIVersionChecker : nsISupports ... >- boolean isValidVersion(in AString version); So, this is no longer needed?

Also, why not make |compare| actually return constants, e.g. A_LESSTHAN_B = -1 A_EQUALS_B = 0 B_LESSTHAN_A = 1 or some such... it'd make writing code much easier so you don't have to constantly be checking the IDL to see what the return value is.

This allows for a much wider variety of existing and future version numbers, including 1.8bN, 1.9pre1, and 1.0+. I do not think that IDL constants are any great advantage over a number <=> 0, and it makes the coding harder because I can't return values from strcmp directly.

Comment on attachment 190126 [details] [diff] [review] Move version-checking capabilities into XPCOM, rev. 2 ParseVP looks like it should be static as well. Is 1.0+ == 1.1pre non-controversial? I'm OK with it, just thought I'd check. -1/0/+1 are pretty standard in everything from Perl/JS sort to memcmp/strcmp/qsort, so I think that's fine here as well. Do you want to actually, you know, add some tests in addition to the test utility? Please call out explicitly that 1.1pre1 < 1.1pre10 in the IDL comment. r=shaver with those.

xpcom/base/nsVersionComparatorImpl.h needs #ifndef/#define/#endif include guards.

Fixes nits, adds real testcase runner (ifndef CROSS_COMPILE).

Comment on attachment 190166 [details] [diff] [review] Move version-checking capabilities into XPCOM, rev. 2.1 [checked in] porting forward my/darin's review, and marking a=shaver

Comment on attachment 190166 [details] [diff] [review] Move version-checking capabilities into XPCOM, rev. 2.1 [checked in] The versioncomparator is checked in. Shall we close this bug or leave it open?

This checkin breaks my linux build because the LD_LIBRARY_PATH isn't set. If we're going to run binaries during the build, they should use the run-mozilla.sh wrapper script. /home/cls/src/moz/main/obj-ff-opt/config/nsinstall -R ../../../mozilla/xpcom/tests/test.properties ../../dist/bin/res Running TestVersionComparator tests ../../dist/bin/TestVersionComparator: error while loading shared libraries: libxpcom.so: cannot open shared object file: No such file or directory Testing '1.0pre1 1.0pre2' expected '1.0pre1 < 1.0pre2', got ''.

This caused part of bug 301875

This code doesn't work correctly with negative numbers. For example, it gives these conflicting responses: 2.0 < 2.0.-1 (this is wrong) 2.0 = 2.0.0 2.0.0 > 2.0.-1 The problem is the "OnlyZero" logic to short-circuit the comparison loop: it assumes that any further dotted sub-versions will be larger than 0.0.0.etc, which isn't true if the longer version has negatives. (This affects the PHP version, too). The fix is to simply remove the OnlyZero logic, and let the comparison loop get to the end of both strings (not just the shortest). The ParseVP function will continue to return a "0" struct for the shorter string's sub-versions for the sake of comparison.

Comment on attachment 193013 [details] [diff] [review] fix to properly handle negative numbers in version This looks good to me. Can you provide an update PHP version also, perhaps?

oh, you already did, nevermind

(In reply to comment #26) > Please call out explicitly that 1.1pre1 < 1.1pre10 in the IDL comment. Is he calling out that "1.1pre1 < 1.1pre10" or that "1.1pre10 < 1.1pre2"? (It seems that that's still the case from my reading of the code, but it's after midnight.. -- if 1.1pre10 > 1.1pre2 now, ignore this comment!) If there's still interest in tweaking the version comparison, what RPM does might be useful -- it compares runs of numbers numerically, and then runs of non-numbers using strcmp rules. Given "1pre10" and "1pre2" the comparisons that would happen are "1" vs "1", "pre" vs "pre" and "10" vs "2", which would leave less potential pitfalls for authors.

> (In reply to comment #26) > if 1.1pre10 > 1.1pre2 now, ignore this comment! The code currently returns 1.1pre10 > 1.1pre2 > Given "1pre10" and "1pre2" the comparisons that would happen > are "1" vs "1", "pre" vs "pre" and "10" vs "2" That's how it currently works. Also, regarding my pending patch: while it's certainly possible no one will use the negative number feature, the current code does claim to support it, such as this comment from nsIVersionComparator.idl: + * Although not required by this interface, it is recommended that + * numbers remain within the limits of a signed char, i.e. -127 to 128. Support for negatives, but with bugs, seems like one of those "potential pitfalls for authors". Considering that it's also a trivial patch, I'm putting in a query for blocking 1.8b4.

Comment on attachment 193013 [details] [diff] [review] fix to properly handle negative numbers in version r=shaver. (Man, I have to read a lot of code very slowly every time we touch this file!)

bsmedberg: can you land this ASAP, so that we can get it trunk baking before landing it on the branch? Version-comparator changes have a scary rep in these parts. Thanks!

Attachment 193013 [details] [diff] landed on trunk and branch. Closing this bug because it's longer than it should be already.

Would it be accurate to say that the max application version for extension authors wishing to support Firefox 2 should be set to 2.1.-1, or is there a better way to specify this? Should it be 2.0.1.-1?

The AMO faq says 2.0.0.*