[webkit-reviews] review denied: [Bug 188712] Fix python dependency issue for launch time benchmark : [Attachment 347416] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Ben Richards
<benton_richards at apple.com>'s request for review:
Bug 188712: Fix python dependency issue for launch time benchmark
https://bugs.webkit.org/show_bug.cgi?id=188712

Attachment 347416: Patch

https://bugs.webkit.org/attachment.cgi?id=347416&action=review




--- Comment #2 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 347416
  --> https://bugs.webkit.org/attachment.cgi?id=347416
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=347416&action=review

How did you come to the decision to use pip(1)? Is there a reason we cannot
make use of the existing autoinstall logic in webkitpy? The apporach tak

> PerformanceTests/ChangeLog:9
> +	   Got rid of autoinstaller for installing tornado package in favor of
installing
> +	   to the autoinstalled directory using pip.

Why? What is the issue that you are running into?

> PerformanceTests/LaunchTime/thirdparty/__init__.py:-22
> -# Copyright (C) 2010 Chris Jerdonek (cjerdonek at webkit.org)
> -# Copyright (C) 2018 Apple 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.
> -#
> -# THIS SOFTWARE IS PROVIDED BY APPLE INC. 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 INC. 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.

Why are we removing the license block given that this file is non-empty?

> PerformanceTests/LaunchTime/thirdparty/__init__.py:33
> +    if not os.path.isdir(os.path.join(_SITE_PACKAGES_DIR, 'tornado')):
> +	   subprocess.check_output(['pip', 'install', '--install-option',
'--prefix={}'.format(_AUTOINSTALLED_DIR), 'tornado'])

r-; pip(1) is not part of a default installation on macOS.