1060597 - [titanic] extract version number from build directory

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Gautam Akiwate [:gakiwate]]

Titanic (https://github.com/gakiwate/titanic/) is a tool to bisect test failures.

Titanic takes revision under investigation - which can be supplied to it using the APIs or command line and then works by pulling data about the revision and revisions before it from TBPL(https://tbpl.mozilla.org/) to bisect the failure.

Presently, titanic needs a version number to locate the appropriate build name for the tests to run. However, for now we have hard code the version here. https://github.com/gakiwate/titanic/blob/master/titanic.py#L560

We would like to add a function that will be able to extract the version number automatically from the build directory that has been located.

For example, we may locate this build directory http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-linux64/1409342280/ and need to figure out that the version number is '34.0a1'.

The version can be extracted out of any file that is formatted like firefox-(version).en-US.*.*

Comment 1

[Benjamin Smedberg]

Careful not to mark the security-bug flag when you don't mean it!

Comment 2

[Gautam Akiwate [:gakiwate]]

Whoops! Sorry about that!

Comment 3

[Daniel Y Lee]

Hi,

I think I have an idea for implementing the feature required for this ticket. I would like to give it a try!

Comment 4

[Gautam Akiwate [:gakiwate]]

Great! The bug is yours! :)

Comment 5

[Daniel Y Lee]

Hi,

I made my first attempt at a solution. The code is viewable here:

https://github.com/devty1023/titanic/blob/master/titanic.py#L566

I imported two more python modules (glob and re) in order to implement the patch. Please let me know if this is undesirable. Other comments/questions I have are made directly on the code itself.

Looking forward to your review!

Daniel

Comment 6

[Gautam Akiwate [:gakiwate]]

Hi Daniel,

Can you use the 'git format-patch' command to upload a patch here. It makes it much easier for me to give feedback and at the same time keep track of the progress of the BUG.

I'll review the code once you upload a patch here!

Gautam

Comment 7

[Daniel Y Lee]

Hi,

I tried generating a git format-patch file. Hopefully, this will help in the review process!

Daniel
---


From 525dfb241459e5d6bc2b8b8576ce358e8eb1efba Mon Sep 17 00:00:00 2001
From: devty <[e-mail]>
Date: Mon, 1 Sep 2014 01:00:46 -0700
Subject: [PATCH] Bug 1060597 - [titanic] extract version number from build
 directory

---
 titanic.py | 36 ++++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/titanic.py b/titanic.py
index a01641a..d9a5ae5 100755
--- a/titanic.py
+++ b/titanic.py
@@ -6,6 +6,8 @@ import urllib
 import argparse
 import sys
 import datetime
+import glob
+import re
 import bisect
 import requests
 
@@ -107,6 +109,12 @@ b2g_mozilla-central_helix_periodic opt
 b2g_mozilla-central_wasabi_periodic opt
 '''
 
+#
+# The following strings are used to 
+# read version infromation from the build directory
+#
+VERSION_GLOB = "firefox-*.en-US.*.*"
+VERSION_RE = '(?<=firefox-).+(?=.en.US)'
 
 def getPushLog(branch, startDate):
     # Example
@@ -555,13 +563,37 @@ def findBuildLocation(branch, buildername, revision):
 
     return result[5]
 
+def getVersionInfo(buildLocatio):
+    files = glob.glob('%s/%s' % (buildLocation, VERSION_GLOB))
 
-def getBuildInfo(branch, buildername, revision):
-    version = '34.0a1'
+    ## TODO:
+    ## what should happen should a file to get the 
+    ## version information does not exist?
+    if not files:
+      return 'unknown'
+
+    targetFile = files[0] ## any one of the file is fine. Using the first one
+    version_re = re.search(VERSION_RE, targetFile)
 
+
+    ## TODO:
+    ## what should happen when our regular expression
+    ## cannot find any match? (i.e. cant find the version number)
+    if not version_re:
+      return 'unknown'
+
+    ## TODO:
+    ## Technically, this is not gauranteed to return the version number
+    ## Should there be a safeguard to check if somewhat appropriately value
+    ## has been collected?
+    return version_re.group(0)
+
+def getBuildInfo(branch, buildername, revision):
     runArgs = populateArgs(branch, buildername, revision, 1)
     ftp = findBuildLocation(branch, buildername, revision)
 
+    version = getVersionInfo(ftp)
+
     if platformXRef[runArgs['platform'][0]] == 'winxp' or \
             platformXRef[runArgs['platform'][0]] == 'win7':
         extension = 'zip'
-- 
1.9.1

Comment 8

[Gautam Akiwate [:gakiwate]]

Can you check the upload as a patch when uploading the patch? Also, make sure you add me to the review? section when you do that. That way I will be able to comment on your code.

Sorry, to make you do this so many times! But this will be a good run for the other BUGs you work on! :)

Comment 9

[Daniel Y Lee]

Attachment: 0001-Bug-1060597-titanic-extract-version-number-from-buil.patch

Comment 10

[Daniel Y Lee]

I see! I appreciate your help - still learning how the review process works. 

Please let me know if I uploaded the patch correctly this time!

Comment 11

[Gautam Akiwate [:gakiwate]]

Comment on attachment 8482160 [details] [diff] [review]
0001-Bug-1060597-titanic-extract-version-number-from-buil.patch

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

This is an excellent attempt! Nearly there! :)

::: titanic.py
@@ +109,5 @@
>  b2g_mozilla-central_wasabi_periodic opt
>  '''
>  
> +#
> +# The following strings are used to 

Make sure there are no trailing spaces.

@@ +563,4 @@
>  
>      return result[5]
>  
> +def getVersionInfo(buildLocatio):

typo. buildLocation*

@@ +569,5 @@
> +    ## TODO:
> +    ## what should happen should a file to get the 
> +    ## version information does not exist?
> +    if not files:
> +      return 'unknown'

For now let's return '34.0a1' if we don't get anything. Additionally, let's make a comment here documenting the same.

@@ +585,5 @@
> +    ## TODO:
> +    ## Technically, this is not gauranteed to return the version number
> +    ## Should there be a safeguard to check if somewhat appropriately value
> +    ## has been collected?
> +    return version_re.group(0)

I think that would be a sensible safeguard to put in place. Maybe, cross validating 2 revisions extracted from the first two files? This way we can avoid corner cases. What do you think?

Comment 12

[Daniel Y Lee]

Hi Gautam,

Thanks for the review - sorry about the pesky editing errors I made!

In regards to the cross-checking references. Is there some sort of gaurantee that at least two files with embedded version info exists in a repository? I feel that "requiring" at least two of such file for the user of the titanic in order to retrieve their build version info is a strange one. Furthermore, what if I get two different "versions"? What should the behaivor of the code be?

I would suggest that we go on and craft another regular expression that checks whether the extracted version conforms so a loose format - something like <number>*\.<alphanumeric>*. Is this sensible, or will this be too limiting?

Comment 13

[Gautam Akiwate [:gakiwate]]

Greetings,

Yes. We should have atleast two files with embedded version info. However, I agree with you that "requiring" at least two matches is a strange one. I think you should go ahead and try the other regular expression and see how it works!

For testing, you should go to this link https://tbpl.mozilla.org/?tree=Mozilla-Inbound and click on any 'green B' symbol. You will get a small box at the bottom which will read "go to the build directory". In the code we basically access this same build directory linked here for that revision.

Let me know how it goes!

Comment 14

[Daniel Y Lee]

Hi Gautam,

While looking through https://tbpl.mozilla.org/?tree=Mozilla-Inbound, I noticed that for some builds, the files of the format "firefox-(version).en-US.*.*" does not always exists. For instance, consider http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-inbound-android/1409712307/ where we have "fennec-(version).en-US.*.*". 

Is this something I should consider? I would simply need to modify my regex.

Comment 15

[Gautam Akiwate [:gakiwate]]

Yes. That is something you should consider. 

Thing is, there might be a lot of corner cases. However, our goal should be to account for as many as we can.

Joel, what do you think?

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

Daniel, great question- yes we should account for what we can.  right now we have been focused on firefox (desktop builds), but Android (fennec) is next.  If you can get a good start on that, it would be very helpful!

Comment 17

[Daniel Y Lee]

Hi Joel!

Thank you for your comments. 

I noticed that there is also a possibility for "b2g" (firefox os). I am going to craft a generic regular expression that will be able to capture any file with the form

*-(version).en-US.*.*

to accomodate for any possibility in the future.

Comment 18

[Daniel Y Lee]

Actually, a bit more issue -

b2g builds have pretty weird version file format. Something like:

b2g-35.0a1.multi.mac64.checksums

Currenlty, this "exception" will be caught and the whole build directory will be given a default version number (which is set to 35.0a1). But I wonder if this is truly a good way to handle this kind of cases. For instance:

http://ftp.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-central-emulator/1409776763/

This build doesn't really have a version information, and the code I've crafted will return 35.0a1 as its version. Returning a false-positive is something I do not want to do. Any comments?

Comment 19

[Joel Maher ( :jmaher ) (UTC -8)]

oh b2g is interesting.  So the version we could match as <product>-[0-9][0-9].[0-9]a1.*

What we could do is default to a version, document it and file a bug with the edge cases we know of documented.

Comment 20

[Gautam Akiwate [:gakiwate]]

Seconded.

Comment 21

[Daniel Y Lee]

Hi Joel,

Can I ask what you mean by "documenting" a bug? Would you like for me to submit a bug on bugzilla?

It bothers me to submit a code that I KNOW contains a logical bug :( . But if it is okay with you and Gautam, I go ahead and continue on the aforementioned implementation.

Comment 22

[Joel Maher ( :jmaher ) (UTC -8)]

Daniel,

Great question.  Many times we split a task into pieces (usually by filing bugs with the remaining bits of work to do) so we can make forward progress.  It helps things not get stale and it helps spread the workload.

Use your judgement, if you prefer to do it 100% now, lets do it- we can keep the dialog going until we have all the questions answered!

I really appreciate your work on this bug.  titanic is starting to get used more and more :)

Comment 23

[Daniel Y Lee]

Attachment: 0001-Bug-1060597-titanic-extract-version-number-from-buil.patch

Hi,

I generalized the vesion regular expression to handle several different cases. This regular expression will work with "fennect-..." "firefox-..." and "b2g-...".

There is a case of build directory like

http://ftp.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-central-emulator/1409776763/

where I cannot parse out any meaningful version number from it. In such case, the default version will be returned (is this okay?).

Please review and comment!

Comment 24

[Gautam Akiwate [:gakiwate]]

Comment on attachment 8485549 [details] [diff] [review]
0001-Bug-1060597-titanic-extract-version-number-from-buil.patch

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

Nearly there. Just had a single comment. Let me know if you had any follow up comments about it?

::: titanic.py
@@ +679,5 @@
>  
> +    version = version_re.group(0)
> +
> +    ## final sanity check to see if version number
> +    ## is somewhat appropriate

Is there any particular reason why you are doing a two-stepped extraction? I am not sure I understand if your first check does any work considering that the second check pretty much overrides it?

Comment 25

[Daniel Y Lee]

Attachment: 0001-Bug-1060597-titanic-extract-version-number-from-buil.patch

Ah thanks for pointing that out.

That is kinda "legacy" code - I had previously used a pattern like 

1) retrieve words between "firefox" and "en.US"
2) Confirm that it is some combination of alphanumeric

Since (1) is no longer valid, I had changed its regular expression to be a "combination of alphanumeric". So confirmation looks redundant now. 

I removed the code on the attached patch.

Comment 26

[Gautam Akiwate [:gakiwate]]

Comment on attachment 8485747 [details] [diff] [review]
0001-Bug-1060597-titanic-extract-version-number-from-buil.patch

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

Looks good to me. Can you send me a pull request on Github?

Next time, make sure that you review? me for the review. That way it shows up on list.

Comment 27

[Daniel Y Lee]

https://github.com/gakiwate/titanic/pull/14

Got it!

Comment 28

[Gautam Akiwate [:gakiwate]]

Awesome! Excellent work as always!

https://github.com/gakiwate/titanic/pull/14/