Add script to check for minimum python version and install if missing on Tiger

• Component: New Bugs
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Add script to check for minimum python version and install if missing on Tiger

[Eric Seidel (no email)]

Created attachment 55666 [details]
Patch

[Mark Rowe (bdash)]

Comment on attachment 55666 [details]
Patch

> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> index 2a7d2eec0cec020427e124077d574d830c5a1b6c..b189ac0f631048162a62692eaa87ab05359fe462 100644
> --- a/WebKitTools/ChangeLog
> +++ b/WebKitTools/ChangeLog
> @@ -1,3 +1,30 @@
> [phone]  Eric Seidel  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Add script to check for minimum python version and install if missing on Tiger
> +        https://bugs.webkit.org/show_bug.cgi?id=38886
> +
> +        Per Maciej's request on webkit-dev:
> +        https://lists.webkit.org/pipermail/webkit-dev/2010-May/012785.html
> +        provide a script which can automatically install Python on Tiger where
> +        the system provided version is too old to be of use.
> +
> +        Note this uses the official Mac Python installer from python.org.
> +        This installs a system copy of Python.  It is not possible to install
> +        a non-system python without building our own copy.

I’m not sure that this is accurate.  The package appears to write Python.framework in to /Library/Frameworks/Python.framework.  That’s not where the system version of Python lives.

If I’m mistake about that and your initial comment is correct then… we really shouldn’t be overwriting the system version of Python.  Overwriting system files is not supported.  I’d really prefer that we didn’t encourage this, and we certainly shouldn’t be doing this without informing the user.

> diff --git a/WebKitTools/Scripts/ensure-valid-python b/WebKitTools/Scripts/ensure-valid-python
> new file mode 100755
> index 0000000000000000000000000000000000000000..cfb13f0283f908fab3c7465ce12cb135f3ab12c2
> --- /dev/null
> +++ b/WebKitTools/Scripts/ensure-valid-python
> @@ -0,0 +1,134 @@
> +#!/usr/bin/perl -w
> +# Copyright (C) 2010 Google Inc. All rights reserved.
> +#
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions
> +# are met:
> +#
> +# 1.  Redistributions of source code must retain the above copyright
> +#     notice, this list of conditions and the following disclaimer. 
> +# 2.  Redistributions in binary form must reproduce the above copyright
> +#     notice, this list of conditions and the following disclaimer in the
> +#     documentation and/or other materials provided with the distribution. 
> +# 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> +#     its contributors may be used to endorse or promote products derived
> +#     from this software without specific prior written permission. 

“Apple Computer, Inc” no longer exists.  It should not be referenced in new license text.

> +#
> +# THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> +# EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> +# WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> +# DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY
> +# DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> +# (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> +# LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> +# THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +
> +use strict;
> +
> +use File::Basename;
> +use File::Spec;
> +use File::Temp qw(tempdir);
> +use FindBin;
> +
> +use lib $FindBin::Bin;
> +use webkitdirs;
> +use VCSUtils;
> +
> +my $macPythonURL = "http://www.python.org/ftp/python/2.6.5/python-2.6.5-macosx10.3-2010-03-24.dmg";
> +my $macPythonMD5 = "84489bba813fdbb6041b69d4310a86da";
> +my $macPythonInstallerName = "Python.mpkg";
> +
> +sub checkPythonVersion()
> +{
> +    # Will exit 0 if python is 2.5 or greater, non-zero otherwise.
> +    `python -c "import sys;sys.exit(sys.version_info[:2] < (2,6))"`;
> +    return exitStatus($?) == 0;
> +}

The comment here mentions Python 2.5 but the check appears to be for v2.6.  Is that intentional?

> +sub downloadFileToPath($$)
> +{
> +    my ($remoteURL, $localPath) = @_;
> +    print "Downloading $remoteURL to $localPath\n";
> +    my $exitCode = system("curl", "-o", $localPath, $remoteURL);
> +    return exitStatus($exitCode) == 0;
> +}
> +
> +sub checkMD5($$)
> +{
> +    my ($path, $expectedMD5) = @_;
> +    my $md5Output = `md5 -q "$path"`;
> +    chomp($md5Output);
> +    my $isValid = $md5Output eq $expectedMD5;
> +    print "'$md5Output' does not match expected: '$expectedMD5'\n" unless $isValid;
> +    return $isValid;
> +}
> +
> +sub mountDMG($)
> +{
> +    my ($dmgPath) = @_;
> +    my $hdiutilOuput = `hdiutil attach $dmgPath`;

This will result in the disk image opening in the Finder and appearing in the sidebar.  hdiutil has a “-nobrowse” flag to prevent this.

> +    # One of the lines of output is: "/dev/disk_name    Apple_HFS    /Volumes/Mount Point"
> +    if ($hdiutilOuput !~ /^(.*)\s+Apple_HFS\s+(.*)$/m) {
> +        die "Failed to find mountpoint in \n$hdiutilOuput\nMay have left a disk image mounted."
> +    }
> +    my $mountPoint = $2;

Rather than doing this regexp hackery you could generate a mount point path yourself (somewhere under $TMPDIR?) and use hdiutil’s “-mountpoint” flag to arrange for it to be mounted there.

> +    return $mountPoint;
> +}
> +
> +sub unmountDMG($)
> +{
> +    my ($mountPoint) = @_;
> +    my $exitCode = system("hdiutil", "detach", $mountPoint);
> +    return exitStatus($exitCode) == 0;
> +}
> +
> +sub runInstaller($)
> +{
> +    my ($installerPackage) = @_;
> +    print "sudo will now ask for your password to run the Python installer.\n";
> +    system("sudo", "installer", "-verbose", "-pkg", $installerPackage, "-target", "/");
> +}

Do we care about the exit status of the installer?

> +sub installMacPython()
> +{
> +    my $mountPoint = downloadAndMountMacPythonDMG($macPythonURL, $macPythonMD5);
> +    print "Mounted python install image at: $mountPoint\n";
> +    my $installerPackage = File::Spec->join($mountPoint, $macPythonInstallerName);
> +    runInstaller($installerPackage);
> +    unmountDMG($mountPoint) or die "Failed to unmount DMG from $mountPoint";
> +}

Should this delete the disk image when it is done?  Can we refer to it as a “disk image” instead of “DMG” in the error message since that is the term by which they’re typically known?

> +
> +sub main()
> +{
> +    # Congrats, your python is fine.
> +    return 0 if checkPythonVersion();
> +
> +    if (!isTiger()) {
> +        print "Your python version is insuficient to run WebKit's python.  Please update.\n";
> +        print "See http://trac.webkit.org/wiki/PythonGuidelines for more info.\n";
> +        return 1;
> +    }
> +
> +    installMacPython();
> +
> +    checkPythonVersion() or die "Final version check failed, must have failed to update python";
> +    print "Successfully updated python.\n";
> +}

I’d suggest that “Python” have an initial capital in these messages as that is how the name of the language and implementation is typically rendered in text.  I’d suggest mentioning “WebKit’s Python code” rather than “WebKit’s Python” for sake of clarity.

[Eric Seidel (no email)]

(In reply to comment #2)
> (From update of attachment 55666 [details])

> I’m not sure that this is accurate.  The package appears to write Python.framework in to /Library/Frameworks/Python.framework.  That’s not where the system version of Python lives.
> 
> If I’m mistake about that and your initial comment is correct then… we really shouldn’t be overwriting the system version of Python.  Overwriting system files is not supported.  I’d really prefer that we didn’t encourage this, and we certainly shouldn’t be doing this without informing the user.

You're correct, the installer appears to install on /Library.  Do you know of any way to perform a DISTROOT install with installer?  So I can actually see every file it dumps?  Since it uses several sub installers, it's confusing (using lsbom Archive.bom) to figure out where they all drop their payloads.

> “Apple Computer, Inc” no longer exists.  It should not be referenced in new license text.

Fixed.

> > +sub checkPythonVersion()
> > +{
> > +    # Will exit 0 if python is 2.5 or greater, non-zero otherwise.
> > +    `python -c "import sys;sys.exit(sys.version_info[:2] < (2,6))"`;
> > +    return exitStatus($?) == 0;
> > +}
> 
> The comment here mentions Python 2.5 but the check appears to be for v2.6.  Is that intentional?

A mistake left over from debugging on Leopard (which already has 2.5).  Fixed.

> This will result in the disk image opening in the Finder and appearing in the sidebar.  hdiutil has a “-nobrowse” flag to prevent this.

Correct.  This was intentional.  If the script failed I didn't want to a random invisible mount around.  I'm happy to change it if you think it's better to hide the mount.

> > +    # One of the lines of output is: "/dev/disk_name    Apple_HFS    /Volumes/Mount Point"
> > +    if ($hdiutilOuput !~ /^(.*)\s+Apple_HFS\s+(.*)$/m) {
> > +        die "Failed to find mountpoint in \n$hdiutilOuput\nMay have left a disk image mounted."
> > +    }
> > +    my $mountPoint = $2;
> 
> Rather than doing this regexp hackery you could generate a mount point path yourself (somewhere under $TMPDIR?) and use hdiutil’s “-mountpoint” flag to arrange for it to be mounted there.

I considered that, however I didn't know how rm would fair trying to delete a mount point.  I guess I'll just have to test it.  The temp directory from File::Temp->createdir automatically deletes itself when perl exits.

> Do we care about the exit status of the installer?

Yes, sorry.  Fixed.

> Should this delete the disk image when it is done?  Can we refer to it as a “disk image” instead of “DMG” in the error message since that is the term by which they’re typically known?

We download the image to our temp directory which gets deleted by File::Temp automatically when python exits.

I've fixed the log to say "disk image", thanks.

> I’d suggest that “Python” have an initial capital in these messages as that is how the name of the language and implementation is typically rendered in text.  I’d suggest mentioning “WebKit’s Python code” rather than “WebKit’s Python” for sake of clarity.

Fixed.

I'll test having it mount into the temp directory with nobrowse and see how that does.  Then I'll post a new patch.

Thank you for the quick review.

[Eric Seidel (no email)]

(In reply to comment #3)
> > This will result in the disk image opening in the Finder and appearing in the sidebar.  hdiutil has a “-nobrowse” flag to prevent this.
> 
> Correct.  This was intentional.  If the script failed I didn't want to a random invisible mount around.  I'm happy to change it if you think it's better to hide the mount.

I've changed it to use -nobrowse in the latest version.  That makes it more difficult to debug if something goes wrong, but makes it quieter while operating.  Not a big deal either way.

> > Rather than doing this regexp hackery you could generate a mount point path yourself (somewhere under $TMPDIR?) and use hdiutil’s “-mountpoint” flag to arrange for it to be mounted there.
> 
> I considered that, however I didn't know how rm would fair trying to delete a mount point.  I guess I'll just have to test it.  The temp directory from File::Temp->createdir automatically deletes itself when perl exits.

rm won't delete the mountpoint.  So the temp directory deletion will fail if for some reason the script were to exit w/o unmounting the DMG.  However I changed the code to use a specific mount point per your suggestion.  I've tested it and it works fine.


I'll post a patch momentarily which I believe addresses all of your previous concerns.

[Eric Seidel (no email)]

Created attachment 55747 [details]
Patch

[Maciej Stachowiak]

Comment on attachment 55747 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 55747 [details]
Patch

Clearing flags on attachment: 55747

Committed r59538: <http://trac.webkit.org/changeset/59538>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.