make update verification run on each platform

RESOLVED FIXED

Status

RESOLVED FIXED
12 years ago
5 years ago

People

(Reporter: rhelmer, Assigned: rhelmer)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 7 obsolete attachments)

8.77 KB, patch
rcampbell
: review+
Details | Diff | Splinter Review
2.22 KB, patch
rcampbell
: review+
Details | Diff | Splinter Review
2.42 KB, patch
preed
: review+
Details | Diff | Splinter Review
(Assignee)

Description

12 years ago
Created attachment 256089 [details] [diff] [review]
require that config is set before verify is invoked, and copy the proper updater binary

We need to run update verification on the proper platform (it all runs on one Mac right now), and we should set it up so this can run automatically from Buildbot (both for nightlies and on-demand).
Attachment #256089 - Flags: review?(rcampbell)
(Assignee)

Comment 1

12 years ago
I've made this work from Buildbot by setting the environment:

updateEnvironment = {
    'release' : '2.0.0.1',
    'product' : 'Firefox',
    'platform': 'Linux_x86-gcc3',
    'build_id' : '2006120814',
    'locales' : 'en-US',
    'channel' : 'betatest',
    'from' : '/firefox/releases/2.0.0.1/linux-i686/%locale%/firefox-2.0.0.1.tar.gz',
    'to' : '/firefox/nightly/2.0.0.2-candidates/rc5/firefox-2.0.0.2.%locale%.linux-i686.tar.gz'}


Note that "from" and "to" are optional, if are specified then it means that they should be downloaded, updates applied, and compared. If both "from" and "to" are not specified, then the verification scripts simply check to make sure that MARs are available and match the desired checksum and size specified in the update.xml

All of the other variables are required.

Once the above it set, verify.sh (checked out from mozilla/testing/release/) can be invoked like so:

f1.addStep(ShellCommand, command=["./verify.sh", "-c"],
                         workdir="build/updates",
                         env=updateEnvironment)


Any ideas on what the best way to get the data in the environment into Buildbot is? We could have Buildbot just pick up an OS-specific config file out of CVS, or we could allow sendchange to specify all of this data (which means that for a release we'd submit ~120 changes, which is probably the most scalable way to do this but also pretty annoying).

For this round, I would be happy to have these running on three machines, one for each OS being tested (we should be testing these on the proper OS anyway, e.g. updater.exe should be used for win32).
Assignee: build → rhelmer
(Assignee)

Comment 2

12 years ago
Created attachment 256108 [details] [diff] [review]
return >0 exit code on failure, remove "tee /dev/stderr"

Same as last time, plus:

* remove the "| tee /dev/stderr" stuff, don't need that anymore
* set exit code to be the same as the number of failures

This patch makes it work nicely in buildbot out-of-the-box, and should work without complaint on Windows. I'll likely have at least one more patch to make it unpack and use the right updater binary on win32 and mac (this patch tested on linux via buildbot)
Attachment #256089 - Attachment is obsolete: true
Attachment #256108 - Flags: review?(rcampbell)
Attachment #256089 - Flags: review?(rcampbell)
Comment on attachment 256108 [details] [diff] [review]
return >0 exit code on failure, remove "tee /dev/stderr"

Need to call the right updater per platform in check_updates.sh (win/mac). 

Otherwise, this looks like it should work. It'll be easier to read in a non-diffed state.
Attachment #256108 - Flags: review?(rcampbell) → review-
(Assignee)

Updated

12 years ago
Assignee: rhelmer → nobody
Component: Build & Release → Testing
Product: mozilla.org → Core
QA Contact: preed → testing
Version: other → unspecified
(Assignee)

Updated

12 years ago
Assignee: nobody → rhelmer
(Assignee)

Comment 4

12 years ago
Created attachment 256846 [details] [diff] [review]
corrected updater path, tested on win32/mac
Attachment #256108 - Attachment is obsolete: true
Attachment #256846 - Flags: review?(rcampbell)
Comment on attachment 256846 [details] [diff] [review]
corrected updater path, tested on win32/mac

double-A-plus-goldstar
Attachment #256846 - Flags: review?(rcampbell) → review+
(Assignee)

Comment 6

12 years ago
Created attachment 257086 [details] [diff] [review]
revert verify.sh changes, make config file configurable instead

I went ahead and landed the bits in the common/ dir from the last patch, thanks!

I haven't checked in any changes to verify.sh; I think that the short-term path to happiness is going to be doing 3 runs of this script, one per OS, each with it's own config file from CVS. The attached patch just makes it possible to optionally specify a config file, e.g.:

./verify.sh -t moz18-branch-updates.cfg

If the config file is not specified:

./verify.sh -t 

Then update.cfg is used (the previous behavior).

All buildbot needs to do is specify the config file (it'll be OS specific, probably something like moz18-branch-osx.cfg); the environment variable trick above isn't needed yet, although I would like to do something more like that in the future.
Attachment #257086 - Flags: review?(rcampbell)
(Assignee)

Comment 7

12 years ago
Created attachment 257198 [details] [diff] [review]
remove "|tee /dev/stderr" and set diff FAIL to WARN

Same as last time, but:

1) remove "| tee /dev/stderr", won't work on cygwin and this was only useful when triggered by a human; might as well just grep for FAIL now
2) just because there are differences it isn't necessarily a FAIL - it should definitely be a WARN, this needs to be decided by a human

I've been meaning to take care of #2 for a while, even though it's not strictly related to this bug. It'd be ideal to make this part of the test smarter so we can confidently pass/fail; some of the issues we found are not the kind of thing you'd want to ignore a file for (e.g. "diff -x filename").
Attachment #257086 - Attachment is obsolete: true
Attachment #257198 - Flags: review?(rcampbell)
Attachment #257086 - Flags: review?(rcampbell)
Attachment #257198 - Flags: review?(rcampbell) → review+
(Assignee)

Comment 8

12 years ago
Created attachment 258601 [details] [diff] [review]
use wget instead of curl for complete update case

Use wget for the normal "complete update" test. This still uses curl for the "quick" headers-only tests, because that has no platform dependencies, and isn't a feature wget has afaict (that is, follow a redirect and return the results of a HEAD request).

This change allows us to run this script on the standard build machines, which include wget but not curl.
Attachment #258601 - Flags: superreview?(rcampbell)
Attachment #258601 - Flags: review?(preed)

Comment 9

12 years ago
Comment on attachment 258601 [details] [diff] [review]
use wget instead of curl for complete update case

The error was:

../common/download_mars.sh: line 7: curl: command not found

This patch doesn't patch download_marsh.sh

Also, I'd use -O instead of >.
Attachment #258601 - Flags: review?(preed) → review-
(Assignee)

Comment 10

12 years ago
Created attachment 258605 [details] [diff] [review]
sorry, this misses the common dir (ran diff too high up)
Attachment #258601 - Attachment is obsolete: true
Attachment #258605 - Flags: review?(preed)
Attachment #258601 - Flags: superreview?(rcampbell)
(Assignee)

Updated

12 years ago
Attachment #258605 - Attachment is obsolete: true
Attachment #258605 - Flags: review?(preed)
(Assignee)

Comment 11

12 years ago
Created attachment 258606 [details] [diff] [review]
use wget, dispense with stdout/redirect
Attachment #258606 - Flags: review?(preed)
Comment on attachment 258606 [details] [diff] [review]
use wget, dispense with stdout/redirect

This is OK, except for the -qO [argument to O].

I'm actually surprised that wget allows this, since it's very confusing; it makes it look like the O (which could be a 0/zero) is an argument to -q.

Can we do -q -O [foo] ?
Attachment #258606 - Flags: review?(preed) → review-
(Assignee)

Comment 13

12 years ago
Created attachment 258608 [details] [diff] [review]
separate -q and -O for readability
Attachment #258606 - Attachment is obsolete: true
Attachment #258608 - Flags: review?(preed)

Updated

12 years ago
Attachment #258608 - Flags: review?(preed) → review+
(Assignee)

Comment 14

12 years ago
Landed:

Checking in common/download_mars.sh;
/cvsroot/mozilla/testing/release/common/download_mars.sh,v  <--  download_mars.sh
new revision: 1.15; previous revision: 1.14
done
Checking in updates/verify.sh;
/cvsroot/mozilla/testing/release/updates/verify.sh,v  <--  verify.sh
new revision: 1.8; previous revision: 1.7
done
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 15

12 years ago
Created attachment 258613 [details] [diff] [review]
use wget --no-check-certificate

Some versions of wget balk at * certificates, and need --no-check-certificate (a *.mozilla.org cert in this case).

The one in common/download_mars.sh that talks to AUS is the important one for the "complete" case, but we might as well make it consistent across-the-board (including the one remaining instance of "curl", the equiv is "-k").
Attachment #258608 - Attachment is obsolete: true
Attachment #258613 - Flags: review?(preed)
Comment on attachment 258613 [details] [diff] [review]
use wget --no-check-certificate

r=preed.

I'll land this for you.
Attachment #258613 - Flags: review?(preed) → review+
(Assignee)

Comment 17

12 years ago
I think to make the whole thing cross-platform (including the "-t" mode) will require more than we can do with wget and bash, and implies bug 373995.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Component: Testing → Release Engineering
Product: Core → mozilla.org
QA Contact: testing → release
Version: unspecified → other
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.