722501 - Regex in testrun_release.py prevents downloading of ESR builds

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Automation, defect)

Description

[u279076]

In particular the staging script does not support ESR releases. The nomenclature is as follows:

ftp.mo/10.0esr-candidates/build1/

Right now the staging script (testrun_release.py) terminates saying it can't find "http://stage.mozilla.org/pub/mozilla.org/firefox/releases/10.0/win32/en-US"

A sample config file will follow.

Comment 1

[u279076]

Attachment: Sample config

Comment 2

[u279076]

FWIW, we should check to see if the other scripts (release_push, release_bft, release_update) and the Dashboard have support for ESR.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #0)
> "http://stage.mozilla.org/pub/mozilla.org/firefox/releases/10.0/win32/en-US"

This is for the final release of Firefox 10.0, which doesn't exist yet under releases. If you want to test a candidate build please use '--type=candidate'.

./download.py --type=candidate --version=10.0esr --platform=mac

For the config file you should not forget about the # in the version number, which makes a version a candidate.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The config file shows that you are using it correctly. I have an idea why it is not working. Checking it now.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

It's the regex for getting the version which let us fail in getting the right bits here. A patch will follow immediately.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1

Makes the version detection in a config ini file more flexible by removing the digit requirement.

Comment 7

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 592882 [details] [diff] [review]
Patch v1

Looks good. We'd have needed that anyway because of the idiosyncratic 11.0redo type names they give when a build gets trashed. r+

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

> Looks good. We'd have needed that anyway because of the idiosyncratic
> 11.0redo type names they give when a build gets trashed. r+

Hm, not sure what you are talking about.

Landed as:
http://hg.mozilla.org/qa/mozmill-automation/rev/2cdea2b5eff3

Comment 9

[u279076]

Updated all the machines and it works good now. Thanks for the quick turnaround.

Comment 10

[u279076]

I think this change has caused a regression. 
3.6.26#2.cfg now stages Firefox 3.6.exe
I've not tried other config files yet.

Comment 11

[u279076]

This seems to only affect 3.6.* config files.

Comment 12

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

(In reply to Henrik Skupin (:whimboo) from comment #8)
> > Looks good. We'd have needed that anyway because of the idiosyncratic
> > 11.0redo type names they give when a build gets trashed. r+
> 
> Hm, not sure what you are talking about.

Sorry, not redo. For example:

ftp://ftp.mozilla.org/pub/mozilla.org/firefox/releases/3.0.19-real-real/

We haven't had one in a while, but point is sometimes the releases have words in the name.

Comment 13

[u279076]

So it's not just 3.6.* releases that can't be properly staged.  It's anything with a .point release. For example, 5.0.1, 6.0.2, etc.

Comment 14

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #11)
> This seems to only affect 3.6.* config files.

OK, taking a stab at parsing the original and new:

Original:

(?P<version>[\d\.]+(?:\w\d+)?)(?:#(?P<build_number>\d+))?

version: a single digit followed by a period, one or more times, optionally followed by any alphanumeric/underscore ending with a number.

optional separator of #. If there, then:

build_number: one digit, one or more times (i.e. a number)

So, 3.6.26 succeeds. I'd actually expect 10.*, 11.*, etc. to fail, though, since we've specified single digit major/minors only. The other kicker was that the third part of the version had to end in a number.

New:

(?P<version>(\d)+\.(?:\w+)?)(?:#(?P<build_number>\d+))?

version: a number followed by a period optionally followed by any number of alphanumeric/underscores

build number same way.

However, 3.6.26 fails because because it's grabbing 3.6 (number, period, alphanumeric) and stopping at the . because it's not a valid \w candidate (alphanumeric and underscore).

I *think* we want:

(?P<version>(?:\d+\.)+[^#\s]+)(?:#(?P<build_number>\d+))?

That's:

version: number followed by a period, one or more times, followed by any number of characters until it hits # or whitespace.

build: same as above.

That should work for:

3.6.26
10.0
10.0esr
3.0.19-real-real

It's a little loose in that it'll also allow 10.foo@!@^$ and 10.3.6.3.6.25 but it means we can handle anything that starts with a version number and doesn't have a # in it.

Unfortunately, not in a position to test a change right now, but maybe that helps Henrik tonight?

Comment 15

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #13)
> So it's not just 3.6.* releases that can't be properly staged.  It's
> anything with a .point release. For example, 5.0.1, 6.0.2, etc.

Yeah, confirms my parse. We're stopping at the last . because it's not a valid \w candidate. Also means we're ignoring #s on versions with three parts.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1 (follow-up)

Yeah, sorry for it. In the future when we are working on the 2.0 support for automation scripts we should implement tests to ensure everything works as expected.

This patch makes it even more loosely and as Geo pointed out lets grab everything before a '#' or '=' as version number. If the user makes a failure we will bail out when trying to find the folder.

Comment 17

[David Burns :automatedtester]

Comment on attachment 593002 [details] [diff] [review]
Patch v1 (follow-up)

Review of attachment 593002 [details] [diff] [review]:
-----------------------------------------------------------------

this captures what was mentioned in comment 13. We don't have #.#.#-word-word type releases so we don't need to support them yet

r+

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed as:
http://hg.mozilla.org/qa/mozmill-automation/rev/2c8c155a1ddd

Comment 19

[u279076]

This appears to be working good now. Thanks.

Comment 20

[u279076]

Actually, I might have spoke too soon. When testing the cfg_update/10.0#1.cfg file I get the following output:

No entries found: http://stage.mozilla.org/pub/mozilla.org/firefox/releases/platform/win32/win32

Adding a txt file containing the terminal output and a snippet of the config file.

Comment 21

[u279076]

Attachment: Config & Output

Comment 22

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

(In reply to David Burns :automatedtester from comment #17)
> Comment on attachment 593002 [details] [diff] [review]
> Patch v1 (follow-up)
> 
> Review of attachment 593002 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this captures what was mentioned in comment 13. We don't have
> #.#.#-word-word type releases so we don't need to support them yet
> 
> r+

Wanted to point out that we do indeed have them; I posted the version path for one up above.

Releng hasn't had to do it lately, but that's their standard way to disambiguate. 

So it's part of the spec. YAGNI didn't apply here, and we probably should have supported "-" as a valid character. Otherwise, it's going to suck the first release they have to do it and then we have to scramble.

Comment 23

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #20)
> Actually, I might have spoke too soon. When testing the
> cfg_update/10.0#1.cfg file I get the following output:
> 
> No entries found:
> http://stage.mozilla.org/pub/mozilla.org/firefox/releases/platform/win32/
> win32
> 
> Adding a txt file containing the terminal output and a snippet of the config
> file.

We've loosened it so much that now it thinks platform= is a version number, and is looking for the win32 locale of version platform.

Working on a patch with my original regex suggestion.

Comment 24

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Attachment: Revert to regex that looks for valid version number.

Uses regex in comment 14. Tested against the following config file:

[testrun]
application=firefox
directory=10.0#1
script=update

[vista]
platform=win32
4.0=en-US fr
4.0.1=en-US it
10.0#1=en-US de
10.0esr#1=en-US
3.0.19-real-real=en-US

Output follows:

moco:mozmill-automation geo$ ./testrun_release.py -c testconfig
Initializing directory: /Users/geo/hg/mozmill-automation/10.0#1/update_logs
Initializing directory: /Users/geo/hg/mozmill-automation/10.0#1/update_vista
Downloading build: http://stage.mozilla.org/pub/mozilla.org/firefox/releases/4.0/win32/en-US/Firefox Setup 4.0.exe
Downloading build: http://stage.mozilla.org/pub/mozilla.org/firefox/releases/4.0/win32/fr/Firefox Setup 4.0.exe
Downloading build: http://stage.mozilla.org/pub/mozilla.org/firefox/releases/4.0.1/win32/en-US/Firefox Setup 4.0.1.exe
Downloading build: http://stage.mozilla.org/pub/mozilla.org/firefox/releases/4.0.1/win32/it/Firefox Setup 4.0.1.exe
Retrieving list of candidate builds from http://stage.mozilla.org/pub/mozilla.org/firefox/nightly/10.0-candidates/
Downloading build: http://stage.mozilla.org/pub/mozilla.org/firefox/nightly/10.0-candidates/build1/win32/en-US/Firefox Setup 10.0.exe
Retrieving list of candidate builds from http://stage.mozilla.org/pub/mozilla.org/firefox/nightly/10.0-candidates/
Downloading build: http://stage.mozilla.org/pub/mozilla.org/firefox/nightly/10.0-candidates/build1/win32/de/Firefox Setup 10.0.exe
Retrieving list of candidate builds from http://stage.mozilla.org/pub/mozilla.org/firefox/nightly/10.0esr-candidates/
Downloading build: http://stage.mozilla.org/pub/mozilla.org/firefox/nightly/10.0esr-candidates/build1/win32/en-US/Firefox Setup 10.0esr.exe
Downloading build: http://stage.mozilla.org/pub/mozilla.org/firefox/releases/3.0.19-real-real/win32/en-US/Firefox Setup 3.0.19.exe

Note that any further patch tests should take place against a full config file with non-version keys included, as they're also intended to be excluded.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Geo Mealer [:geo] from comment #22)
> Wanted to point out that we do indeed have them; I posted the version path
> for one up above.

Dang, sorry. I have never checked the releases page on FTP that closely:
ftp://ftp.mozilla.org/pub/firefox/releases/3.0.19-real-real/

> So it's part of the spec. YAGNI didn't apply here, and we probably should
> have supported "-" as a valid character. Otherwise, it's going to suck the
> first release they have to do it and then we have to scramble.

Yeah. Agree here now. 

(In reply to Geo Mealer [:geo] from comment #23)
> We've loosened it so much that now it thinks platform= is a version number,
> and is looking for the win32 locale of version platform.

Thanks for spotting this. This regex bug is kinda messy :(

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 593259 [details] [diff] [review]
Revert to regex that looks for valid version number.

Ok, lets take this approach. At least it let us better handle the version entry. I wonder if we should move to a JSON file for the configuration instead which would totally abandon those special parsing in the ini file.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed as:
http://hg.mozilla.org/qa/mozmill-automation/rev/255935e8d585

Comment 28

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

(In reply to Henrik Skupin (:whimboo) from comment #26)

> entry. I wonder if we should move to a JSON file for the configuration
> instead which would totally abandon those special parsing in the ini file.

Maybe. I do like how easy it is to put together a configuration file, and think JSON might be a bit of a speedbump. If we implement a web front end to doing it, JSON would make a lot of sense though.

As it is, the parse has become pretty simple, so I'm not too worried. I think we made it a little too complicated to begin with, and in doing so made it difficult to tell later it was doing. I should have caught this in code review, but I'd forgotten that we were only looking for well-formed keys by design.

In retrospect, I probably should have added a comment that it's looking specifically for "##." at the beginning of the number, but the comment I did add at least speaks in that direction.