347256 - AUS2 and patcher need to support update metadata

Status: RESOLVED FIXED

(AUS Graveyard :: Administration, task)

Description

[Mike Schroepfer]

Bug 329729 implements major updates on the client so the FF1.5.0.x users can be offered a minor update to the latest 1.5.0.x version or a major update (with user confirmation) to 2.0.0.x.   

We need to make sure AUS2 can publish major updates and patcher2 knows how to generate these easily.

Comment 1

[Michael Morgan [:morgamic]]

Adjusting this to encompass the real problem, which is generating then delivering metadata for the <update> element in our AUS2 XML.

The fields we are missing support for that are currently coming from defaults in the AUS2 config are <update ...:
* type
* version
* extensionVersion
* isSecurityUpdate

For more information on these fields are for and the XML format, see:
http://wiki.mozilla.org/Software_Update:updates.xml_Format

As was done w/ the detailsURL and buildID, we could find a place for this in the build snippets and easily adjust the XML output if that data is found.

Comment 2

[J. Paul Reed [:preed]]

If we're going to be adding values to the snippet files, I think we should bite the bullet here and using an extensible format.

Right now, the snippets work based upon certain data being in a certain position/order: http://lxr.mozilla.org/mozilla/source/tools/patcher/patcher2.pl#1107

This constrains us to only being able to use default values for so many variables (currently, for instance, detailsURL is not required, but we can't put any other values in its place).

I think we should change this to a key=value system, so we can have something like:

ver=1
url=http://foo
type=complete
scope=major
detailsUrl=http://bar

etc.

As changes go, it's pretty big, but we'll only have to do this once.

Thoughts?

Comment 3

[Mike Schroepfer]

Why is there an intermediate snippet format at all?  Why not send along data in the same xml dialect that is published via AUS?

Comment 4

[J. Paul Reed [:preed]]

(In reply to comment #3)
> Why is there an intermediate snippet format at all?  Why not send along data in
> the same xml dialect that is published via AUS?

I don't know the history here, so I'm probably not the best person to ask, but it's probably because it was easier to store/manipulate the data in something that wasn't XML. All the stupid find tricks we do only work because we're *not* parsing/futzing with XML.

Basically separation of content (snippets) and presentation (xml).

Comment 5

[Michael Morgan [:morgamic]]

Yeah, last year the patcher scripts were not ready to have that responsibility tacked on top of them.  At the time, partial patches were hardly working at all.

Today, the segregation and flow of the complete and partial scripts doesn't allow for easy generation of a single XML document per complete+partial pairing.

We'd have to bridge that gap first -- maybe by making the patcher2 script (the second leg of the flow) responsible for generating the XML and placing it in the appropriate path.

It's arguable, though, that the build scripts should do their job only, and offload data into a storage engine somewhere else so it can be repackaged as needed for stuff like:
* syndicating version information, available builds, notifying people when a new build is completed
* automatic updating of other dependent systems (like bouncer, addons)

Looking ahead, I think the ideal situation would be having a build database to store the information.  The way it is now, the structure is kind of a cheesy-hack(tm) DB using a filesystem.

The AUS2 PHP script does a query based on product, version, platform, build, locale and channel to get update info, and each directory in the path is a major relationship, leading up to the filenames themselves, which designate patch type.

I think the original intent behind the current AUS2 was to act as a stopgap between AUS1.  Chase would be the first to say that the current system was a step towards a more streamlined build and release process that would involve our blue-skies database and automation where appropriate.

Comment 6

[J. Paul Reed [:preed]]

Attachment: New parser for the snippet files, take 1

Please be kind; I haven't hacked PHP in awhile...

This handles the parsing of snippet files only; morgamic: let's get together to figure out what the XML output should look like.

Comment 7

[J. Paul Reed [:preed]]

Comment on attachment 236908 [details] [diff] [review]
New parser for the snippet files, take 1

Crap; there's a typo in this patch. That's what I get for renaming variables.

morgamic: if you test this, you'll need to make this minor change.

I'll post a new patch on Monday.

>Index: patch.class.php
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/aus/xml/inc/patch.class.php,v
>retrieving revision 1.12
>diff -u -8 -r1.12 patch.class.php
>--- patch.class.php	28 Aug 2006 19:59:13 -0000	1.12
>+++ patch.class.php	6 Sep 2006 02:01:36 -0000

>@@ -140,19 +153,41 @@
>             }
>             
>             if ($this->isComplete() && isset($file[8])) {
>                 $this->setVar('detailsUrl',$file[8],true);
>                 $this->setVar('hasDetailsUrl',true,true);
>             }
>             
>             return true;
>-        }
>+        } elseif (CURRENT_SNIPPET_SCHEMA_VER == $snippetSchemaVersion) {
>+            for ($fileNdx = 0; $fileNdx < count($file); $fileNdx++) {
>+                if (preg_match('/^(\w+)=(.+)$/', $file[$index], $matches)) {

Change the above line to:

if (preg_match('/^(\w+)=(.+)$/', $file[$fileNdx], $matches)) {

Comment 8

[Michael Morgan [:morgamic]]

Comment on attachment 236908 [details] [diff] [review]
New parser for the snippet files, take 1

* not sure why the define() statements are not in the config -- there are no defines anywhere else aside from the config
* would be nice to add more comments -- I've done this in another patch I'll post shortly, fwiw
* would not handle case where the snippetSchemaVersion was not found -- which could produce a notice for an undeclared variable $snippetSchemaVersion when calling error_log
* if-elseif-else could just be two if's and a default return of false with the appropriate error log
* to be consistent, open/close brackets on if() statements should be put in
* for(;;) could be a foreach(), which is a bit more readable
* it would be a lot easier if the attributes were named similarly in the classes as they are in the snippet file
* it sets variable that are not previously declared in the class so I had to figure out which variables we were expecting :(

Comment 9

[Michael Morgan [:morgamic]]

Attachment: Parser patch, v2

Added some comments, addressed some of my notes in the above comment.  Also added the logic to actually assign the passed updateType to the update object so it gets to the XML output.

In a related change, I committed acceptance tests for these -- see the latest changes to the aus/tests/ directory to see what I mean:
http://lxr.mozilla.org/mozilla/source/webtools/aus/tests/data/3/Synthetic/1.0/platform/8000000001/locale/channel/complete.txt

In general it'd be nice to clean up some of this stuff... the way the update info is passed from a complete.txt, then to index.php, then finally the update object itself seems completely wrong.  Seems like there should be a better way.

Until we discuss/find that, this patch works and was tested using the acceptance tests.

I still didn't move the define() statements to the config -- I'm not too worried about it, but was just curious why they were put in the patch class and not in the config with all other defines.

I'm making the previous patch obsolete, since the updateType would never get pushed to the XML output (missing logic).  This patch is just a suggestion -- up to you what you want to do with my suggestion(s), preed.  :)

Comment 10

[Michael Morgan [:morgamic]]

Attachment: Parser patch, v3

Changed things a bit based on our discussion in IRC.
* added our if-elseif-else statement back in

Still am not sure about:
+            // Store information found only in complete snippets.
+            // This information is tied to the <update> element.
+            if ($this->isComplete()) {
+                if (isset($this->appv) && isset($this->extv)) {
+                    $this->setVar('updateVersion', $this->appv, true);
+                    $this->setVar('updateExtensionVersion', $this->extv, true);
+                    $this->setVar('hasUpdateInfo', true, true);
+                }

Question: Why appv and extv when in the class they are called updateVersion and updateExtensionVersion?

Comment 11

[J. Paul Reed [:preed]]

(In reply to comment #10)

> Still am not sure about:
> +            // Store information found only in complete snippets.
> +            // This information is tied to the <update> element.
> +            if ($this->isComplete()) {
> +                if (isset($this->appv) && isset($this->extv)) {
> +                    $this->setVar('updateVersion', $this->appv, true);
> +                    $this->setVar('updateExtensionVersion', $this->extv,
> true);
> +                    $this->setVar('hasUpdateInfo', true, true);
> +                }
> 
> Question: Why appv and extv when in the class they are called updateVersion and
> updateExtensionVersion?

This is a direct translation of:

             if ($this->isComplete() && isset($file[6]) && isset($file[7])) {
                 $this->setVar('updateVersion',$file[6],true);
                 $this->setVar('updateExtensionVersion',$file[7],true);
                 $this->setVar('hasUpdateInfo',true,true);
             }

From the original code (translated, that is, to do the right thing given the proper metadata input).

Did I translate incorrectly?

Comment 12

[Michael Morgan [:morgamic]]

(In reply to comment #11)
> This is a direct translation of:
> 
>              if ($this->isComplete() && isset($file[6]) && isset($file[7])) {
>                  $this->setVar('updateVersion',$file[6],true);
>                  $this->setVar('updateExtensionVersion',$file[7],true);
>                  $this->setVar('hasUpdateInfo',true,true);
>              }
> 
> From the original code (translated, that is, to do the right thing given the
> proper metadata input).
> 
> Did I translate incorrectly?
Right, except appv and extv being set in the patch object would mean that in the block for schema version 1, you would have set them because you'd have the following in the snippet file it read:
appv=1.0
extv=1.0

So my question is really why appv and extv are being set at all, and why they just don't have keys in the snippet file that match what the patch object expects?  appv and extv are not vars in the Patch class, so I found them confusing.

Instead I would have expected:
if ($this->isComplete
    && isset($this->updateVersion) 
    && isset($this->updateExtensionVersion)
    && isset($this->updateType)) { 

Comment 13

[(not reading, please use [e-mail] instead)]

once this is up and running on AUS2, for the planned "major update / eula test" for, we'll want to use:

http://www.mozilla.com/test/sample-details.html
http://www.mozilla.com/test/sample-eula.html

(note, bug #352397 covers the pushing of these test pages.)

Comment 14

[J. Paul Reed [:preed]]

Comment on attachment 237891 [details] [diff] [review]
Parser patch, v3

Looks good! r=preed

Comment 15

[Michael Morgan [:morgamic]]

Working with sspitzer this afternoon to verify behavior.  ETA is tonight.

Comment 16

[J. Paul Reed [:preed]]

This was pushed to production this morning (bug 352864) and we're doing a test this afternoon.

Resolving fixed.

Comment 17

[Michael Morgan [:morgamic]]

Attachment: Added licenseUrl support

This adds support for licenseUrl in XML output, which wasn't updated in this bug when the client bug was updated (around the time isSecurityUpdate was dropped, looks like).

Comment 18

[Michael Morgan [:morgamic]]

Comment on attachment 237891 [details] [diff] [review]
Parser patch, v3

This actually isn't obsolete.  This has already been checked in.

Comment 19

[J. Paul Reed [:preed]]

Comment on attachment 239218 [details] [diff] [review]
Added licenseUrl support

Bring on the licenseUrls!

Comment 20

[J. Paul Reed [:preed]]

Reopen to address the patcher side.

Looks like I was slightly overzealous in resolving this.

Comment 21

[J. Paul Reed [:preed]]

Attachment: Add v1 schema support to patcher2

AUS v1 schema support for patcher2:

-- teach patcher how to write out v1 schemas snippets
-- add ability to specify schema version in patcher2.cfg
-- Keep default schema support as an option

I tested this by regenerating the 2.0b2 -> 2.0rc1 snippets, and they were all generated as expected.

Comment 22

[Robert Helmer [:rhelmer]]

Comment on attachment 240576 [details] [diff] [review]
Add v1 schema support to patcher2

I like the key=value pairs in the new schema.

Comment 23

[J. Paul Reed [:preed]]

Alright, I think we're good for this.

We probably should add v1 schema support to Tinderbox at some point too, but... I'm not in a huge hurry to do that, and it's a good test of the schema fallback code.