Closed Bug 300731 Opened 17 years ago Closed 17 years ago

change app/extension version scheme going forward

Categories

(Toolkit :: Application Update, defect, P1)

x86
All
defect

Tracking

()

RESOLVED FIXED
mozilla1.8final

People

(Reporter: vlad, Assigned: benjamin)

References

Details

(Keywords: fixed1.8)

Attachments

(4 files, 3 obsolete files)

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.
Assignee: nobody → benjamin
(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. 
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → Firefox1.1
I believe that this code is backwards-compatible with basically all of our
present requirements.
Attachment #189814 - Flags: review?(darin)
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.
Attachment #189814 - Attachment is obsolete: true
Attachment #189814 - Flags: review?(darin)
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.
Attachment #190126 - Flags: review?(shaver)
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.
Attachment #190126 - Flags: review?(shaver) → review+
xpcom/base/nsVersionComparatorImpl.h needs #ifndef/#define/#endif include guards.
Fixes nits, adds real testcase runner (ifndef CROSS_COMPILE).
Attachment #190126 - Attachment is obsolete: true
Attachment #190166 - Flags: approval1.8b4?
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
Attachment #190166 - Flags: superreview+
Attachment #190166 - Flags: review+
Attachment #190166 - Flags: approval1.8b4?
Attachment #190166 - Flags: approval1.8b4+
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?
Attachment #190166 - Attachment description: Move version-checking capabilities into XPCOM, rev. 2.1 → Move version-checking capabilities into XPCOM, rev. 2.1 [checked in]
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 ''.
Attachment #190253 - Flags: review?(cls)
Attachment #190253 - Flags: review?(cls) → review+
Attachment #190253 - Attachment description: Fix the testrunner on unix → Fix the testrunner on unix [checked in]
Blocks: 287098
Attached file PHP version of the version comparator (obsolete) —
Blocks: 304857
Attachment #192830 - Attachment is patch: false
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.
Attachment #192830 - Attachment is obsolete: true
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?
Attachment #193013 - Flags: review? → review?(shaver)
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.
Flags: blocking1.8b4?
Flags: blocking1.8b4? → blocking1.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!)
Attachment #193013 - Flags: review?(shaver) → review+
Flags: blocking1.8b5+
Attachment #193013 - Flags: approval1.8b4?
Attachment #193013 - Flags: approval1.8b4? → approval1.8b4+
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.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
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.*
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.