401170 - Minotaur needs to be more forgiving of paths, especially on windows

Status: RESOLVED FIXED

(Testing Graveyard :: Minotaur, defect)

Description

[cmtalbert]

Attachment: A minotaur.sh shell script that is hacked into supporting cygwin (using defualt cygwin location)

I don't think that Minotaur should require cygwin, especially since we no longer require cygwin for builds.  But, I do think that if someone runs Minotaur from a Cygwin shell, it ought to somehow realize that and use proper paths.

One constant difficulty of this is that the profile directory setting for the -CreateProfile call MUST use a fully specified native path.  That means that ~/tmp/profile will NOT work on Mac/Linux.  And that /cygdrive/c/tmp will NOT work on Windows.  

One way to get around this issue is to require Minotaur (perhaps as an optional parameter) to take the profile path on the input line.  It may also have to have a special parameter for when to run in "cygwin" mode, since the ability to detect that we are in a cygwin shell may or may not be discoverable, depending on which tools we use.

There are further difficulties on windows because were a cygwin shell may want /cygdrive/c/foo paths, the mozilla-build shell will want /c/foo style paths.

I will attach a minotaur.sh that has been hacked to default support running inside a cygwin shell.  It makes a blanket assumption that cygwin is installed at c:\cygwin.  This should be determined by looking at various environment variables.

Comment 1

[Rob Campbell [:rc] (:robcee)]

can we use uname in combination with the getOsInfo.py script to get a better picture of what we're running on? this should be pretty easy to catch and then the modified version of minotaur.sh should run on windows under cygwin.

I wouldn't put too much extra effort into this as we should really be using MSYS.

Comment 2

[cmtalbert]

Attachment: Diff containing partner automation

This contains all the code necessary to run the partner automation stuff.

Windows notes...
You'll need cygwin.  You'll also need the diffutils package from Cygwin under the utils package.

Comment 3

[cmtalbert]

Attachment: Minotaur Full partner automation

This is the full minotaur patch that now does NOT use cygwin, and can install and download its own builds.  This also has been modified to work properly with the Places infrastructure for bookmarks and to handle Partner build EULAs as well as a host of other fixes.

I need to get this baseline of code checked in so that I can submit sane patch diff's when working on the other minotaur bugs.

Comment 4

[[e-mail] (:bhearsum)]

Comment on attachment 294778 [details] [diff] [review]
Minotaur Full partner automation

>Index: testing/release/minotaur/checkReleaseChannel.py
> aus2link = re.compile(".*https:\/\/aus2.mozilla.org.*")
>

Should this be "https:\/\/aus2.mozilla.org.*"?


>Index: testing/release/minotaur/getOsInfo.py

> def getPlatform():
>   print platform.system()
> 
> def getFxName(os):
>   if os == "Darwin":
>     print "firefox-bin"
>   elif os == "Linux":
>     print "firefox"
>-  elif os == "Windows":
>+  else:
>     print "firefox.exe"
> 

In Talos, we discovered that platform.system() returns "Microsoft" on Vista. If there's a chance that 'os' could be passed in a value from that you may want to include it.
(For reference, the Talos code I mentioned: http://mxr.mozilla.org/mozilla/source/testing/performance/talos/ffprocess.py#50).
I see that you're using platform.system() calls further down in this patch. I'm wondering if it would be good if you just returned values here, and printed them out in the actual code when you want to. So basically, s/print/return/ here. This is especially helpful in getPlatform() as you can handle the Vista case more easily. ie:
def getPlatform():
    # Make this return 'Windows' on Vista, like other Microsoft operating systems do.
    return platform.system().replace('Microsoft', 'Windows')

And then...when you need to check the platform, you can just do:
if getPlatform() == 'Windows' instead of 'getPlatform in ('Microsoft', 'Windows').


>Index: testing/release/minotaur/minotaur.sh

>+# Python sometimes mis-reports cygwin shells (depending on python interpreter)
>+if [ $OSTYPE = "cygwin" ]; then
>+  OS=Cygwin
>+  FFX_EXE=firefox.exe
> fi

Given that the cygwin dependency is being removed, does this need to be here?


>   # Check to see if we fail or pass
>-  if [ -s ${minotaurdir}/$fxname-$locale/results.log ]; then
>+  if [ -s $resultsdir/results.log ]; then
>     echo !!!!!!!!!!!!! TEST FAILS !!!!!!!!!!!!!!
>   else
>     echo ------------- TEST PASSES -------------
>   fi
> fi

In the future, it might be nice if there was a quick summary of what test failed.


>Index: testing/release/minotaur/mozDownload.py
(I'm going to be tougher on this class because it's more general purpose :)

>+class MozDownloader:
I'd like to see this stuff done with Python's urllib (http://docs.python.org/lib/module-urllib.html). You'll have to use the FancyURLOpener and override prompt_user_passwd() to get this to work. I think it's good to not depend on wget, but I'll leave that choice up to you. I won't r- based on this.

>+  def __init__(self, **kwargs):
>+    assert kwargs['url'] != ""
>+    assert kwargs['dest'] != ""
>+    self.url = kwargs['url']
>+    self.dest = kwargs['dest']
>+    self.error = 0
>+    self.user = kwargs['user']
>+    self.passwd = kwargs['password']
>+
>+  def download(self):
>+    try:
>+      args = "wget -nv -N"
>+      if self.user:
>+        args += " --user=" + self.user + " --password=" + self.passwd + " " + self.url
>+      else:
>+        args += " " + self.url
>+
>+      print args
>+      proc = subprocess.Popen(args, shell=True)
>+      proc.wait()
>+    except:
>+      print "error in download"
>+      self.error = 1
>+    self.moveToDest()
>+
>+  def moveToDest(self):
>+    try:
>+      # Grab the name of the downloaded file from the URL
>+      parsedUrl = urlparse.urlparse(self.url)
>+      path = parsedUrl[2]
>+      pathElements = path.split("/")
>+      filename = pathElements[len(pathElements) - 1]
>+      print filename

Better to use os.path.basename/os.path.dirname for these.

>+
>+      # Create directory - we assume that the name of the file is the last
>+      # parameter on the path
>+      if self.dest[0] == "~":
>+        self.dest = self.dest.replace("~", "${HOME}", 1)

I don't think '~' will work on Windows.


>Index: testing/release/minotaur/mozInstall.py

>+class MozUninstaller:
>+  def __init__(self, **kwargs):
>+    debug("uninstall constructor")
>+    assert kwargs['dest'] != ""
>+    # TODO: If we don't have to patch the startup scripts then we may not
>+    # need the product name
>+    assert kwargs['productName'] != ""
>+    assert kwargs['branch'] != ""
>+    self.dest = kwargs['dest']
>+    self.productName = kwargs['productName']
>+    self.branch = kwargs['branch']
>+
>+    # Handle the case where we haven't installed yet
>+    if not os.path.exists(self.dest):
>+      return
>+
>+    if platform.system() == "Windows":

See my above comments about getOsInfo.

>+  def removeDirectories(self):

Can you use shutil.rmtree for this?

>+  def normalizePath(self, path):
>+    if path[0] == "~":
>+      path = path.replace("~", "${HOME}", 1)

I don't think ${HOME} works on Windows :(


>Index: testing/release/minotaur/partner.py

>+def getPlatformDirName():
>+  os = platform.system()
>+  if (os == "Darwin"):
>+    return "mac"
>+  elif (os == "Linux"):
>+    return "linux-i686"
>+  else:
>+    return "win32"
>+

If you make the changes to getOsInfo you can use getPlatform here.



Overall this seems fine. I'm going to r- specifically because of the things that will break Windows (~/, ${HOME}). I think the MozDownloader changes are important for cross-platform support but it seems like it's fine for your purposes.

Comment 5

[cmtalbert]

(In reply to comment #4)
> (From update of attachment 294778 [details] [diff] [review])
> >Index: testing/release/minotaur/checkReleaseChannel.py
> > aus2link = re.compile(".*https:\/\/aus2.mozilla.org.*")
> >
> 
> Should this be "https:\/\/aus2.mozilla.org.*"?
> 
No, I'm looking for the line containing the constructed address for the AUS ping, so that I can pull out the AUS channel used.  The HTTP log usually has headers that occur before the address.  For example, here is the line from one of the HTTP log files:
-1610559488[507230]: uri=https://aus2.mozilla.org/update/3/Firefox/3.0b2pre/2007110804/Darwin_Universal-gcc3/en-US/nightly/Darwin%208.10.1/default/default/update.xml?force=1
So, this regex is correct.
> 
> >Index: testing/release/minotaur/getOsInfo.py

> In Talos, we discovered that platform.system() returns "Microsoft" on Vista. If
> there's a chance that 'os' could be passed in a value from that you may want to
> include it.
> (For reference, the Talos code I mentioned:
> http://mxr.mozilla.org/mozilla/source/testing/performance/talos/ffprocess.py#50).
> I see that you're using platform.system() calls further down in this patch. I'm
> wondering if it would be good if you just returned values here, and printed
> them out in the actual code when you want to. So basically, s/print/return/
> here. This is especially helpful in getPlatform() as you can handle the Vista
> case more easily. ie:
> def getPlatform():
>     # Make this return 'Windows' on Vista, like other Microsoft operating
> systems do.
>     return platform.system().replace('Microsoft', 'Windows')
> 
> And then...when you need to check the platform, you can just do:
> if getPlatform() == 'Windows' instead of 'getPlatform in ('Microsoft',
> 'Windows').
I agree with you.  This code uses "print" instead of "return" because getOsInfo is only used from the Minotaur.sh file so that the shell script can determine what OS we are running on.  I'll keep the print statements, but I'll clean these up so that it reports properly on Vista and Cygwin, and I'll remove that other cygwin specific item from the Minotaur.sh file.

> >Index: testing/release/minotaur/minotaur.sh
> 
> >+# Python sometimes mis-reports cygwin shells (depending on python interpreter)
> >+if [ $OSTYPE = "cygwin" ]; then
> >+  OS=Cygwin
> >+  FFX_EXE=firefox.exe
> > fi
> 
> Given that the cygwin dependency is being removed, does this need to be here?
Even though it doesn't depend on cygwin any longer, I don't want Minotaur to break if someone decides to run it on a cygwin shell.  So, this will still need to be handled, but I think it makes sense to put it into the getOSInfo file and remove it from the mintoaur.sh script.
> 
> >   # Check to see if we fail or pass
> >-  if [ -s ${minotaurdir}/$fxname-$locale/results.log ]; then
> >+  if [ -s $resultsdir/results.log ]; then
> >     echo !!!!!!!!!!!!! TEST FAILS !!!!!!!!!!!!!!
> >   else
> >     echo ------------- TEST PASSES -------------
> >   fi
> > fi
> 
> In the future, it might be nice if there was a quick summary of what test
> failed.
> 
Hmmm...true.  That might not be hard to do.  I capture the results into a log file, so I could just cat the results file to the screen if the test fails.
> 
> >Index: testing/release/minotaur/mozDownload.py
> (I'm going to be tougher on this class because it's more general purpose :)
> 
Fair enough :-)

> >+class MozDownloader:
> I'd like to see this stuff done with Python's urllib
> (http://docs.python.org/lib/module-urllib.html). You'll have to use the
> FancyURLOpener and override prompt_user_passwd() to get this to work. I think
> it's good to not depend on wget, but I'll leave that choice up to you. I won't
> r- based on this.
> 
My original thought was to use the urllib also.  I fought with it quite a bit and could not get it to work properly.  Then I looked at the Talos code and saw that ya'll used wget, so I thought perhaps that would be a better way.  However, since making that change, I've discovered that wget only existed on one OS natively (windows, after the mozilla build install) and that made me rather sad to have to depend on it.

I didn't try overriding FancyURLOpener, instead I had tried to use urllib2 since it seemed to support authentication.  I'll go another round with urllib and try it this way.  It'd be much better if I could get it to work without wget.

> >+  def moveToDest(self):
> >+    try:
> >+      # Grab the name of the downloaded file from the URL
> >+      parsedUrl = urlparse.urlparse(self.url)
> >+      path = parsedUrl[2]
> >+      pathElements = path.split("/")
> >+      filename = pathElements[len(pathElements) - 1]
> >+      print filename
> 
> Better to use os.path.basename/os.path.dirname for these.
Ok, will do

> 
> >+
> >+      # Create directory - we assume that the name of the file is the last
> >+      # parameter on the path
> >+      if self.dest[0] == "~":
> >+        self.dest = self.dest.replace("~", "${HOME}", 1)
> 
> I don't think '~' will work on Windows.
You're right, it won't, hence the "if" statement.  I found that the os.path module doesn't interpret ~/blah style directories properly.  Instead of redirecting to the home directory, it was creating a directory named "~/blah" from the current working directory.  This code finds out if we've been passed a ~/blah style directory path for our destination and replaces "~" with "${HOME}" if it exists.  This if statement will always be false on Windows.

Hm...maybe that was your point?  What if someone actually passes in "~" on windows and *wants* us to create a directory starting with ~?

Please explain your point, maybe I'm missing something.

> 
> 
> >Index: testing/release/minotaur/mozInstall.py
> >+    # Handle the case where we haven't installed yet
> >+    if not os.path.exists(self.dest):
> >+      return
> >+
> >+    if platform.system() == "Windows":
> 
> See my above comments about getOsInfo.
Gotcha.
> 
> >+  def removeDirectories(self):
> 
> Can you use shutil.rmtree for this?
Hm...I had lots of trouble with removing directories.  I think I tried shutil.rmtree and it didn't work, but I did so many different things, I don't remember.  I'll try it again.  If it works, I'll nuke this code.
> 
> >+  def normalizePath(self, path):
> >+    if path[0] == "~":
> >+      path = path.replace("~", "${HOME}", 1)
> 
> I don't think ${HOME} works on Windows :(
Right, I don't think it will either.  But, this won't be called unless someone is trying to use a directory that begins with ~ on windows, which I tend to think would not be very likely.  

> 
> >Index: testing/release/minotaur/partner.py
> 
> >+def getPlatformDirName():
> >+  os = platform.system()
> >+  if (os == "Darwin"):
> >+    return "mac"
> >+  elif (os == "Linux"):
> >+    return "linux-i686"
> >+  else:
> >+    return "win32"
> >+
> 
> If you make the changes to getOsInfo you can use getPlatform here.
Right.  I'll make the same changes here.  My entire use for getOsInfo was for the shell script.  I really hadn't considered using it outside of that, since it seemed overkill to construct a class around a very simple platform call.
> 
> 
> 
> Overall this seems fine. I'm going to r- specifically because of the things
> that will break Windows (~/, ${HOME}). I think the MozDownloader changes are
> important for cross-platform support but it seems like it's fine for your
> purposes.

Sounds fair.  What do you think about my points above?  That ~/ Home stuff isn't called unless the mozDownloader/mozInstall is given a path that begins with "~", which I don't think is very likely to occur on a Windows box.

The wget issue is definitely an issue, and I'll take another look at trying to get by without it.

Thanks a bunch for the review.

Comment 6

[[e-mail] (:bhearsum)]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 294778 [details] [diff] [review] [details])
> > >Index: testing/release/minotaur/checkReleaseChannel.py
> > > aus2link = re.compile(".*https:\/\/aus2.mozilla.org.*")
> > >
> > 
> > Should this be "https:\/\/aus2.mozilla.org.*"?
> > 
> No, I'm looking for the line containing the constructed address for the AUS
> ping, so that I can pull out the AUS channel used.  The HTTP log usually has
> headers that occur before the address.  For example, here is the line from one
> of the HTTP log files:
> -1610559488[507230]:
> uri=https://aus2.mozilla.org/update/3/Firefox/3.0b2pre/2007110804/Darwin_Universal-gcc3/en-US/nightly/Darwin%208.10.1/default/default/update.xml?force=1
> So, this regex is correct.

K

> > 
> > >Index: testing/release/minotaur/getOsInfo.py
> 
> > In Talos, we discovered that platform.system() returns "Microsoft" on Vista. If
> > there's a chance that 'os' could be passed in a value from that you may want to
> > include it.
> > (For reference, the Talos code I mentioned:
> > http://mxr.mozilla.org/mozilla/source/testing/performance/talos/ffprocess.py#50).
> > I see that you're using platform.system() calls further down in this patch. I'm
> > wondering if it would be good if you just returned values here, and printed
> > them out in the actual code when you want to. So basically, s/print/return/
> > here. This is especially helpful in getPlatform() as you can handle the Vista
> > case more easily. ie:
> > def getPlatform():
> >     # Make this return 'Windows' on Vista, like other Microsoft operating
> > systems do.
> >     return platform.system().replace('Microsoft', 'Windows')
> > 
> > And then...when you need to check the platform, you can just do:
> > if getPlatform() == 'Windows' instead of 'getPlatform in ('Microsoft',
> > 'Windows').
> I agree with you.  This code uses "print" instead of "return" because getOsInfo
> is only used from the Minotaur.sh file so that the shell script can determine
> what OS we are running on.  I'll keep the print statements, but I'll clean
> these up so that it reports properly on Vista and Cygwin, and I'll remove that
> other cygwin specific item from the Minotaur.sh file.
> 

K

> > >Index: testing/release/minotaur/minotaur.sh
> > 
> > >+# Python sometimes mis-reports cygwin shells (depending on python interpreter)
> > >+if [ $OSTYPE = "cygwin" ]; then
> > >+  OS=Cygwin
> > >+  FFX_EXE=firefox.exe
> > > fi
> > 
> > Given that the cygwin dependency is being removed, does this need to be here?
> Even though it doesn't depend on cygwin any longer, I don't want Minotaur to
> break if someone decides to run it on a cygwin shell.  So, this will still need
> to be handled, but I think it makes sense to put it into the getOSInfo file and
> remove it from the mintoaur.sh script.

K

> > >+class MozDownloader:
> > I'd like to see this stuff done with Python's urllib
> > (http://docs.python.org/lib/module-urllib.html). You'll have to use the
> > FancyURLOpener and override prompt_user_passwd() to get this to work. I think
> > it's good to not depend on wget, but I'll leave that choice up to you. I won't
> > r- based on this.
> > 
> My original thought was to use the urllib also.  I fought with it quite a bit
> and could not get it to work properly.  Then I looked at the Talos code and saw
> that ya'll used wget, so I thought perhaps that would be a better way. 
> However, since making that change, I've discovered that wget only existed on
> one OS natively (windows, after the mozilla build install) and that made me
> rather sad to have to depend on it.
> 
> I didn't try overriding FancyURLOpener, instead I had tried to use urllib2
> since it seemed to support authentication.  I'll go another round with urllib
> and try it this way.  It'd be much better if I could get it to work without
> wget.
> 

Sounds good.


> > >+  def removeDirectories(self):
> > 
> > Can you use shutil.rmtree for this?
> Hm...I had lots of trouble with removing directories.  I think I tried
> shutil.rmtree and it didn't work, but I did so many different things, I don't
> remember.  I'll try it again.  If it works, I'll nuke this code.

Hm... I seem to recall something about rmtree() not working on Windows sometimes (now that you mention it...).

Ah yes, Buildbot works around this, too. See:
http://mxr.mozilla.org/mozilla/source/tools/buildbot/buildbot/slave/commands.py#59

It sounds like the win32 minotaur machines run MozillaBuild, so you probably have 'rm' on them. If you want to have a pure Python solution, you probably lift the code (with credit) from the link above.

> > >Index: testing/release/minotaur/partner.py
> > 
> > >+def getPlatformDirName():
> > >+  os = platform.system()
> > >+  if (os == "Darwin"):
> > >+    return "mac"
> > >+  elif (os == "Linux"):
> > >+    return "linux-i686"
> > >+  else:
> > >+    return "win32"
> > >+
> > 
> > If you make the changes to getOsInfo you can use getPlatform here.
> Right.  I'll make the same changes here.  My entire use for getOsInfo was for
> the shell script.  I really hadn't considered using it outside of that, since
> it seemed overkill to construct a class around a very simple platform call.

Okay.

> The wget issue is definitely an issue, and I'll take another look at trying to
> get by without it.
> 

Great :)




With regard to the ~ stuff, it looks like i misread there.

Comment 7

[cmtalbert]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Can you use shutil.rmtree for this?
> > Hm...I had lots of trouble with removing directories.  I think I tried
> > shutil.rmtree and it didn't work, but I did so many different things, I don't
> > remember.  I'll try it again.  If it works, I'll nuke this code.
> 
> Hm... I seem to recall something about rmtree() not working on Windows
> sometimes (now that you mention it...).
> 
> Ah yes, Buildbot works around this, too. See:
> http://mxr.mozilla.org/mozilla/source/tools/buildbot/buildbot/slave/commands.py#59
> 
> It sounds like the win32 minotaur machines run MozillaBuild, so you probably
> have 'rm' on them. If you want to have a pure Python solution, you probably
> lift the code (with credit) from the link above.
I just used their code on the grounds that it is far more well tested and more complete than what I had.  I gave credit.  Should I include a link to buildbot source or is a link to our own mxr source for buildbot sufficient?

New patch forthcoming...

Comment 8

[cmtalbert]

Attachment: Addresses review comments

Here is a new patch for the stuff. It address the review comments and uses urllib to perform the download functionality instead of the wget.   In particular, checkout the way that I source the code from the buildbot project in mozInstall.py -- I want to be sure that is done properly.

Comment 9

[[e-mail] (:bhearsum)]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > Can you use shutil.rmtree for this?
> > > Hm...I had lots of trouble with removing directories.  I think I tried
> > > shutil.rmtree and it didn't work, but I did so many different things, I don't
> > > remember.  I'll try it again.  If it works, I'll nuke this code.
> > 
> > Hm... I seem to recall something about rmtree() not working on Windows
> > sometimes (now that you mention it...).
> > 
> > Ah yes, Buildbot works around this, too. See:
> > http://mxr.mozilla.org/mozilla/source/tools/buildbot/buildbot/slave/commands.py#59
> > 
> > It sounds like the win32 minotaur machines run MozillaBuild, so you probably
> > have 'rm' on them. If you want to have a pure Python solution, you probably
> > lift the code (with credit) from the link above.
> I just used their code on the grounds that it is far more well tested and more
> complete than what I had.  I gave credit.  Should I include a link to buildbot
> source or is a link to our own mxr source for buildbot sufficient?

I think it's best to link to the Buildbot repository (though, the code is credited to someone else, so I'm not really sure about this...).

http://buildbot.net/trac/browser/buildbot/slave/commands.py#L61

Comment 10

[[e-mail] (:bhearsum)]

Comment on attachment 295311 [details] [diff] [review]
Addresses review comments

>Index: testing/release/minotaur/mozDownload.py
>===================================================================
>+class MozDownloader:
>+  def __init__(self, **kwargs):
>+    assert kwargs['url'] != ""
>+    assert kwargs['dest'] != ""

You should add 'is not None' to these checks, too. 

>+      try:
>+        os.makedirs(headpath)
>+      except:
>+        #TODO: Determine exception thrown here
>+        print "Error creating directory for download - already exists?"

I keep getting this error, even though my downloads are successful.


>Index: testing/release/minotaur/mozInstall.py
>===================================================================

Make sure you check for 'is not None' in these classes as well.

>+class MozUninstaller:
>+  def __init__(self, **kwargs):
>+    debug("uninstall constructor")
>+    assert kwargs['dest'] != ""
>+    # TODO: If we don't have to patch the startup scripts then we may not
>+    # need the product name
>+    assert kwargs['productName'] != ""
>+    assert kwargs['branch'] != ""
>+    self.dest = kwargs['dest']
>+    self.productName = kwargs['productName']
>+    self.branch = kwargs['branch']
>+
>+    # Handle the case where we haven't installed yet
>+    if not os.path.exists(self.dest):
>+      return
>+
>+    if platform.system() == "Windows":

Should this use getOsInfo?


Other than that, things are looking good. r=bhearsum with the above changes. (You can just carry this review forward).

Comment 11

[cmtalbert]

(In reply to comment #10)
> (From update of attachment 295311 [details] [diff] [review])
> >Index: testing/release/minotaur/mozDownload.py
> >===================================================================
> >+class MozDownloader:
> >+  def __init__(self, **kwargs):
> >+    assert kwargs['url'] != ""
> >+    assert kwargs['dest'] != ""
> 
> You should add 'is not None' to these checks, too. 
> 
Done!

> >+      try:
> >+        os.makedirs(headpath)
> >+      except:
> >+        #TODO: Determine exception thrown here
> >+        print "Error creating directory for download - already exists?"
> 
> I keep getting this error, even though my downloads are successful.
Fixed!

> >Index: testing/release/minotaur/mozInstall.py
> >===================================================================
> 
> Make sure you check for 'is not None' in these classes as well.
Done!

> 
> >+class MozUninstaller:
> >+ ..snip..
> >+    if platform.system() == "Windows":
> 
> Should this use getOsInfo?
> 
Yeah, you're right.  It should.  But, I really want mozInstall.py to be a general-use module and not require other, non-standard python modules.  So, I copied the "getPlatform" function from getOsInfo.py and changed it to "return" instead of "print" the OS.  That duplicates five lines of code but, I think that the benefit (having a standalone python script) outweighs that issue.
> 
> Other than that, things are looking good. r=bhearsum with the above changes.
> (You can just carry this review forward).
Thanks, I'll do that! 

Comment 12

[cmtalbert]

Attachment: Minotaur Final Patch

Addresses Ben's last review comments and carries his review forward.

Comment 13

[cmtalbert]

Patch checked in on trunk:

h-227:~/code/fx-trunk/mozilla clint$ cvs commit -m "bug 401170 New Changes for Minotaur test tool reviewer: bhearsum" testing/release/minotaur 
cvs commit: Examining testing/release/minotaur
Checking in testing/release/minotaur/README.txt;
/cvsroot/mozilla/testing/release/minotaur/README.txt,v  <--  README.txt
new revision: 1.2; previous revision: 1.1
done
Checking in testing/release/minotaur/checkReleaseChannel.py;
/cvsroot/mozilla/testing/release/minotaur/checkReleaseChannel.py,v  <--  checkReleaseChannel.py
new revision: 1.2; previous revision: 1.1
done
Checking in testing/release/minotaur/diffBookmarks.py;
/cvsroot/mozilla/testing/release/minotaur/diffBookmarks.py,v  <--  diffBookmarks.py
new revision: 1.2; previous revision: 1.1
done
Checking in testing/release/minotaur/getOsInfo.py;
/cvsroot/mozilla/testing/release/minotaur/getOsInfo.py,v  <--  getOsInfo.py
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/testing/release/minotaur/grabEULA.sh,v
done
Checking in testing/release/minotaur/grabEULA.sh;
/cvsroot/mozilla/testing/release/minotaur/grabEULA.sh,v  <--  grabEULA.sh
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/testing/release/minotaur/installdmg-expect.ex,v
done
Checking in testing/release/minotaur/installdmg-expect.ex;
/cvsroot/mozilla/testing/release/minotaur/installdmg-expect.ex,v  <--  installdmg-expect.ex
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/testing/release/minotaur/installdmg.sh,v
done
Checking in testing/release/minotaur/installdmg.sh;
/cvsroot/mozilla/testing/release/minotaur/installdmg.sh,v  <--  installdmg.sh
initial revision: 1.1
done
Checking in testing/release/minotaur/logAppender.py;
/cvsroot/mozilla/testing/release/minotaur/logAppender.py,v  <--  logAppender.py
new revision: 1.2; previous revision: 1.1
done
Checking in testing/release/minotaur/minotaur.sh;
/cvsroot/mozilla/testing/release/minotaur/minotaur.sh,v  <--  minotaur.sh
new revision: 1.2; previous revision: 1.1
done
Checking in testing/release/minotaur/minotaur.xul;
/cvsroot/mozilla/testing/release/minotaur/minotaur.xul,v  <--  minotaur.xul
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/testing/release/minotaur/mozDownload.py,v
done
Checking in testing/release/minotaur/mozDownload.py;
/cvsroot/mozilla/testing/release/minotaur/mozDownload.py,v  <--  mozDownload.py
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/testing/release/minotaur/mozInstall.py,v
done
Checking in testing/release/minotaur/mozInstall.py;
/cvsroot/mozilla/testing/release/minotaur/mozInstall.py,v  <--  mozInstall.py
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/testing/release/minotaur/partner.py,v
done
Checking in testing/release/minotaur/partner.py;
/cvsroot/mozilla/testing/release/minotaur/partner.py,v  <--  partner.py
initial revision: 1.1
done
Checking in testing/release/minotaur/quit.xul;
/cvsroot/mozilla/testing/release/minotaur/quit.xul,v  <--  quit.xul
new revision: 1.2; previous revision: 1.1
done
Checking in testing/release/minotaur/sb.js;
/cvsroot/mozilla/testing/release/minotaur/sb.js,v  <--  sb.js
new revision: 1.2; previous revision: 1.1
done
Checking in testing/release/minotaur/tests.manifest;
/cvsroot/mozilla/testing/release/minotaur/tests.manifest,v  <--  tests.manifest
new revision: 1.2; previous revision: 1.1
done
h-227:~/code/fx-trunk/mozilla clint$ 

Comment 14

[[e-mail] (:bhearsum)]

> > >+class MozUninstaller:
> > >+ ..snip..
> > >+    if platform.system() == "Windows":
> > 
> > Should this use getOsInfo?
> > 
> Yeah, you're right.  It should.  But, I really want mozInstall.py to be a
> general-use module and not require other, non-standard python modules.  So, I
> copied the "getPlatform" function from getOsInfo.py and changed it to "return"
> instead of "print" the OS.  That duplicates five lines of code but, I think
> that the benefit (having a standalone python script) outweighs that issue.

That sounds reasonable to me.

Comment 15

[(not currently active) Ted Mielczarek]

Mass moving Minotaur bugs to Testing : Minotaur. Filter on MinotaurMassMove to ignore.