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.*.*
Careful not to mark the security-bug flag when you don't mean it!
Whoops! Sorry about that!
Hi, I think I have an idea for implementing the feature required for this ticket. I would like to give it a try!
Great! The bug is yours! :)
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
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
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 <firstname.lastname@example.org> 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 +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 ## 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']] == 'winxp' or \ platformXRef[runArgs['platform']] == 'win7': extension = 'zip' -- 1.9.1
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! :)
Created attachment 8482160 [details] [diff] [review] 0001-Bug-1060597-titanic-extract-version-number-from-buil.patch
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 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 > > +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?
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?
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!
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.
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?
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!
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.
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?
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.
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.
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 :)
Created attachment 8485549 [details] [diff] [review] 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 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?
Created attachment 8485747 [details] [diff] [review] 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 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.
Awesome! Excellent work as always! https://github.com/gakiwate/titanic/pull/14/