[webkit-reviews] review denied: [Bug 38886] Add script to check for minimum python version and install if missing on Tiger : [Attachment 55666] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue May 11 00:44:02 PDT 2010
Mark Rowe (bdash) <mrowe at apple.com> has denied Eric Seidel <eric at webkit.org>'s
request for review:
Bug 38886: Add script to check for minimum python version and install if
missing on Tiger
https://bugs.webkit.org/show_bug.cgi?id=38886
Attachment 55666: Patch
https://bugs.webkit.org/attachment.cgi?id=55666&action=review
------- Additional Comments from Mark Rowe (bdash) <mrowe at apple.com>
> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> index
2a7d2eec0cec020427e124077d574d830c5a1b6c..b189ac0f631048162a62692eaa87ab05359fe
462 100644
> --- a/WebKitTools/ChangeLog
> +++ b/WebKitTools/ChangeLog
> @@ -1,3 +1,30 @@
> +2010-05-10 Eric Seidel <eric at webkit.org>
> +
> + 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..cfb13f0283f908fab3c7465ce12cb135f3ab1
2c2
> --- /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.
More information about the webkit-reviews
mailing list