304857 - AMO should use new version-comparison code

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Developer Pages, defect)

Description

[Benjamin Smedberg]

Bug 300731 introduced a new form of extension version number which allows our
old "1.8a1" numbering scheme. AMO 1 should support this scheme, as well as AMO 2.

In attachment 192830 [details] I have PHP code which parses these version numbers in an
equivalent manner to the C++ code.

I don't really know the AMO code, but I'm hoping that this function can simply
be plugged in where the current version-comparison code lives.

Comment 1

[Benjamin Smedberg]

https://bugzilla.mozilla.org/attachment.cgi?id=193014 has an updated PHP
function which handles a negative-number edge case.

Comment 2

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

*** Bug 305162 has been marked as a duplicate of this bug. ***

Comment 3

[alanjstr]

Deer Park needs this for testing

Comment 4

[Michael Morgan [:morgamic]]

*** Bug 308157 has been marked as a duplicate of this bug. ***

Comment 5

[Benjamin Smedberg]

Attachment: Updated with "*" syntax

Comment 6

[Michael Morgan [:morgamic]]

Working on this today, this was up after the maintenance scripts were implemented, and that happened on Friday.

Comment 7

[Justin Bradford]

Comment on attachment 200612 [details]
Updated with "*" syntax

Looks good. You might consider adding a "*" test to the list at the bottom. If nothing else, it helps to document the version format.

Michael: This will obviously need to be slightly modified for the AMO/UMO code. What do you need specifically?

Comment 8

[Michael Morgan [:morgamic]]

I think I can fly with this.  I'll give it a shot and ask questions if I have any.  Thanks for your help, guys.

Comment 9

[Michael Morgan [:morgamic]]

Could someone please elaborate on which specific pieces of UMO are breaking because of improper/incorrect version comparisons?  I'm guessing additem.php (developer RDF submission script) and VersionCheck.php (extensions update script).  Is that correct?

Comment 10

[alanjstr]

I'm not sure why additem would have a problem with version-comparison, per se.  However, additem.php and appmanager.php need to be able to handle versions properly.  

Comment 11

[Benjamin Smedberg]

I don't know how UMO works at all: I'm concerned that UMO should be able to recognize and accept extensions with maxVersion="1.5.0.*" and "1.5.0.999" and other variants that were unrecognized under the old extension version numbering scheme.

Comment 12

[alanjstr]

moreinfo.php should be using some sort of comparison to show the latest version.

Comment 13

[Michael Morgan [:morgamic]]

Attachment: Code to be committed, with comments.

I'll add the PHP file for version comparisons to /core, then I will figure out for myself where it needs to go.

I'll start with the application manager and its corresponding DB changes, since it's most critical, then work my way towards VersionCheck.php.  Sorting, etc. in public listings is last in priority.

Honestly, I don't see why this is spread out over 5 different bugs.  The other bugs relating to verison numbers should probably be marked as duplicates.

Comment 14

[Benjamin Smedberg]

*** Bug 255803 has been marked as a duplicate of this bug. ***

Comment 15

[Benjamin Smedberg]

bug 313605 is about using the administration interface to add Firefox 1.5 and allowing extensions with maxVersion="1.5.0.*" to be added.

Bug 284025 is unrelated, AFAICT, and is not relevant to Firefox 1.5 where the app version will always be the same as the extension version.

Comment 16

[Michael Morgan [:morgamic]]

I am removing blocking status from this bug, because the major problem was found to be bug 313605.  

This bug will be fixed by completely revamping how application versions are stored in the database and then making subsequent changes in all version comparisons throughout the application.  Once a correct data structure exists along with proper validation (in admin tool) implementing attachment 200756 [details] [diff] [review] will be fairly simple.

Please see my comments on bug 313605 for more info:
https://bugzilla.mozilla.org/show_bug.cgi?id=313605#c7

Please let me know if this is not accurate -- it's open for discussion, but either way I don't see it as a blocker.

Comment 17

[O. Atsushi (Torisugari)]

Then this is a 1.5.1 blocker and is not a 1.5.0.1 blocker, right?

Mozilla.org might be going to release 1.5.0.1 the next day of 1.5
release, if someone found a serious security issue. If a user with a
1.5.0.1 agent can download a 1.5.0.* app from u.m.o without any
problem, I agree that this bug is not a blocker. All addon authors
need would be the 1.5.0.* approval between 1.5rc - 1.5 period.

Comment 18

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

*** Bug 271269 has been marked as a duplicate of this bug. ***

Comment 19

[Jez]

I think this should not only be used for version comparisons, but also for verifying version numbers in extension submissions.  It doesn't really make sense to have an enumerated list of 'allowed' versions (given in the addons.m.o FAQ) as you're bound to miss some, this makes more sense.  Maybe you could limit it to not being higher than 3.0 until a 4.0 branch appears or something so version numbers don't get far out.

What say you?

Comment 20

[Michael Morgan [:morgamic]]

> Please see my comments on bug 313605 for more info:
> https://bugzilla.mozilla.org/show_bug.cgi?id=313605#c7

I think finite versions are mostly useful for min versions (as a cutoff) and max versions that are too specific (down to a particular subversion) are not very helpful.

The problem here is more a matter of storage and sorting of versions in MySQL and how that scales.  Using a proper validation regexp combined with the use of auto_increment primary keys to sort and select app versions (grouped by GUID) might be an easier way to handle things.

We have to take a serious look at the `applications` table and how it ties into the `version` table -- not only for version sorting in public listings and add-on submission but also for updates needed for add-on blacklisting (bug 290759).

So - short version - the C++ comparison code is great, but this will start with the database.  Take a look at https://update-staging.mozilla.org/~colin/dev/ to see what I'm talking about... (wear a helmet). :\

Comment 21

[Jez]

(In reply to comment #20)
> I think finite versions are mostly useful for min versions (as a cutoff) and
> max versions that are too specific (down to a particular subversion) are not
> very helpful.
> 

I disagree, and so does the Toolkit Version Standard.  When comparing 2 versions, you should perform a bytewise comparison on any string part of a section.

Comment 22

[Michael Morgan [:morgamic]]

(In reply to comment #21)
> I disagree, and so does the Toolkit Version Standard.  When comparing 2
> versions, you should perform a bytewise comparison on any string part of a
> section.
> 
Fair enough, but it doesn't sound like you've really read my previous comment.  I'm not negating the standard at all.  I was attempting to explain context differences between what you'd see in the Toolkit vs. what AMO was created with.  :\

Anyway, the goal for upcoming db changes for storage of app and add-on versions would be to allow for us to adhere more closely to the standard and achieve consistency without sacrificing performance.  It should be possible if we store these version strings correctly.

Comment 23

[Justin Scott [:fligtar]]

This was apparently committed a while back (http://lxr.mozilla.org/mozilla/source/webtools/addons/shared/lib/version_comparison.inc.php) and carried over to Remora (http://svn.mozilla.org/addons/trunk/site/app/controllers/components/versioncompare.php).

Resolving... please re-open if there is still an issue.