Build Skia for a bare-bones embedded Linux system.

Build Skia for a bare-bones embedded Linux system.

Motivation:

    I have downloaded a barebones Linux system built for 64-bit ARM
    from linaro.org and a ARMv8 Foundation Model from arm.com to run
    it on.  This will let us build and test Skia on ARM64 before we
    aquire hardware to allow that.  This CL introduces the changes to
    the build files necessary to build Skia on a barebones embedded
    Linux system.  I tested it with the aarch64 GCC compiler provided
    by linaro.org.

Changes:

    Add a "barelinux" target_os for the DEPS file.  Add an optional
    git download of zlib.

    Changes to gyp files: these changes abstract out libpng, libz, and
    giflib so that images.gyp doesn't know whether they are static or
    dynamically linked.  I also add the variables skia_giflib_static,
    skia_libpng_static, skia_zlib_static, and skia_freetype_static,
    all of which default to false but when set to true will override
    the behavior of the giflib, libpng, zlib, and freetype build
    targets to require them to build statically.  Also, the
    skia_no_fontconfig variable turns off use of the fontconfig service.

	Scripts in platform_tools/barelinux/bin:

    arm64_download - this script downloads the Linaro's ARMv8 Aarch64
	toolchain and minimal embedded Linux system as well as ARM's
	foundation model.  The required files are mirrored on Google
	Cloud.  The script then starts a emulated Arm64 Linux system in
	the background.  After the boot is complete, you can SSH into the
	system at port 8022 via user@localhost.  The SSH key will be
	downloaded into the working directery as well.

	download_deps - Uses gclient to download Skia's dependencies for a
    bare Linux system (the normal dependecies plus giflib, libpng, and
    zlib.)

	barelinux_make - this script builds a version of skia that does
    not depend on external libraries, perfect for putting in an
    embedded system running Linux.  Assumes you have run download_deps
    first.

To test:
    To build a barelinux target, use the barelinux_make script.

    To build for a armv8 system: skia_arch_type=arm arm_neon=0 armv7=1
    armv8=1 arm_thumb=0 skia_arch_width=64 and set the CC and CXX
    variables to point at the cross-compiler downloaded by
    arm64_download.

Committed: http://code.google.com/p/skia/source/detail?r=13570

[hal.canary, 2014-02-06 15:57:31]

Please take a look.

[scroggo, 2014-02-06 16:22:12]

https://codereview.chromium.org/152513007/diff/1/gyp/common_variables.gypi
File gyp/common_variables.gypi (right):

https://codereview.chromium.org/152513007/diff/1/gyp/common_variables.gypi#ne...
gyp/common_variables.gypi:97: #     and third_party/externals/freetype. 
Requires gclient sync to
What does this mean? You need to do a gclient sync after running gyp? Can you be
more specific?

https://codereview.chromium.org/152513007/diff/1/gyp/common_variables.gypi#ne...
gyp/common_variables.gypi:124: #     version instead..
Only one period.

https://codereview.chromium.org/152513007/diff/1/gyp/freetype.gyp
File gyp/freetype.gyp (right):

https://codereview.chromium.org/152513007/diff/1/gyp/freetype.gyp#newcode5
gyp/freetype.gyp:5: 'target_name': 'freetype',
You'll need to do a rebase onto my recent changes for skia_android_framework.

https://codereview.chromium.org/152513007/diff/1/gyp/freetype.gyp#newcode8
gyp/freetype.gyp:8: [ 'not skia_freetype_static and skia_os in ["linux",
"freebsd", "openbsd", "solaris", "chromeos"]', {
nit: over 100 chars.

https://codereview.chromium.org/152513007/diff/1/gyp/freetype.gyp#newcode25
gyp/freetype.gyp:25: [ 'skia_freetype_static or skia_os in ["android", "nacl"]',
{
Would it make more sense to define skia_freetype_static based on skia_os? Then
this condition will be a lot simpler.

https://codereview.chromium.org/152513007/diff/1/gyp/giflib.gyp
File gyp/giflib.gyp (right):

https://codereview.chromium.org/152513007/diff/1/gyp/giflib.gyp#newcode35
gyp/giflib.gyp:35: 'libraries': [
If this is correctly empty, it can be removed.

https://codereview.chromium.org/152513007/diff/1/gyp/giflib.gyp#newcode45
gyp/giflib.gyp:45: }, {
I assume this is if 'skia_giflib_static' is false? Can you add a comment here
reflecting that? It can be hard to visually grasp a gyp file, with its several
layers of brackets and braces.

https://codereview.chromium.org/152513007/diff/1/gyp/images.gyp
File gyp/images.gyp (right):

https://codereview.chromium.org/152513007/diff/1/gyp/images.gyp#newcode20
gyp/images.gyp:20: 'libpng.gyp:libpng',
nit: should line up with libjpeg.

https://codereview.chromium.org/152513007/diff/1/gyp/images.gyp#newcode124
gyp/images.gyp:124: # Any targets that depend on this target should link in
libpng, libgif, and
This comment is out of date.

https://codereview.chromium.org/152513007/diff/1/gyp/images.gyp#newcode128
gyp/images.gyp:128: 'giflib.gyp:giflib'
It appears that without this change, the images project will have these
libraries as link_settings, but with it, giflib will have -lgif as a
link_setting, and images will depend on giflib. Does that accomplish the same
thing?

https://codereview.chromium.org/152513007/diff/1/gyp/libzlib.gyp
File gyp/libzlib.gyp (right):

https://codereview.chromium.org/152513007/diff/1/gyp/libzlib.gyp#newcode59
gyp/libzlib.gyp:59: 'export_dependent_settings': [
This file has many empty lists. I don't think they're necessary.

[djsollen, 2014-02-06 16:24:33]

https://codereview.chromium.org/152513007/diff/1/gyp/freetype.gyp
File gyp/freetype.gyp (right):

https://codereview.chromium.org/152513007/diff/1/gyp/freetype.gyp#newcode64
gyp/freetype.gyp:64: 'libpng.gyp:libpng',
I don't think you need this if your pulling in the images.gyp:images target as
well.

https://codereview.chromium.org/152513007/diff/1/gyp/giflib.gyp
File gyp/giflib.gyp (right):

https://codereview.chromium.org/152513007/diff/1/gyp/giflib.gyp#newcode7
gyp/giflib.gyp:7: 'conditions': [
I'm not a big fan of defining each target 2x.  Why not have one target with the
condition that contains all differences?  Same goes for all the other files you
added.

https://codereview.chromium.org/152513007/diff/1/gyp/images.gyp
File gyp/images.gyp (right):

https://codereview.chromium.org/152513007/diff/1/gyp/images.gyp#newcode20
gyp/images.gyp:20: 'libpng.gyp:libpng',
spacing

https://codereview.chromium.org/152513007/diff/1/gyp/images.gyp#newcode124
gyp/images.gyp:124: # Any targets that depend on this target should link in
libpng, libgif, and
this comment is out of date and should be removed.

https://codereview.chromium.org/152513007/diff/1/gyp/skflate.gyp
File gyp/skflate.gyp (right):

https://codereview.chromium.org/152513007/diff/1/gyp/skflate.gyp#newcode1
gyp/skflate.gyp:1: # Target for including zlib.
fix this comment

[hal.canary, 2014-02-06 18:01:56]

https://codereview.chromium.org/152513007/diff/1/gyp/common_variables.gypi
File gyp/common_variables.gypi (right):

https://codereview.chromium.org/152513007/diff/1/gyp/common_variables.gypi#ne...
gyp/common_variables.gypi:97: #     and third_party/externals/freetype. 
Requires gclient sync to
On 2014/02/06 16:22:13, scroggo wrote:
> What does this mean? You need to do a gclient sync after running gyp? Can you
be
> more specific?

I was referring to setting the deps_os to make sure that the
third_party/externals/* directories exist.  I can document this elsewhere.

https://codereview.chromium.org/152513007/diff/1/gyp/common_variables.gypi#ne...
gyp/common_variables.gypi:124: #     version instead..
On 2014/02/06 16:22:13, scroggo wrote:
> Only one period.

Done.

https://codereview.chromium.org/152513007/diff/1/gyp/freetype.gyp
File gyp/freetype.gyp (right):

https://codereview.chromium.org/152513007/diff/1/gyp/freetype.gyp#newcode5
gyp/freetype.gyp:5: 'target_name': 'freetype',
On 2014/02/06 16:22:13, scroggo wrote:
> You'll need to do a rebase onto my recent changes for skia_android_framework.

Done, if you mean https://codereview.chromium.org/140503007 .

https://codereview.chromium.org/152513007/diff/1/gyp/freetype.gyp#newcode8
gyp/freetype.gyp:8: [ 'not skia_freetype_static and skia_os in ["linux",
"freebsd", "openbsd", "solaris", "chromeos"]', {
On 2014/02/06 16:22:13, scroggo wrote:
> nit: over 100 chars.

Done.

https://codereview.chromium.org/152513007/diff/1/gyp/freetype.gyp#newcode25
gyp/freetype.gyp:25: [ 'skia_freetype_static or skia_os in ["android", "nacl"]',
{
On 2014/02/06 16:22:13, scroggo wrote:
> Would it make more sense to define skia_freetype_static based on skia_os? Then
> this condition will be a lot simpler.

Done.

https://codereview.chromium.org/152513007/diff/1/gyp/freetype.gyp#newcode64
gyp/freetype.gyp:64: 'libpng.gyp:libpng',
On 2014/02/06 16:24:34, djsollen wrote:
> I don't think you need this if your pulling in the images.gyp:images target as
> well.

Not the way I am doing it.  The libpng target puts png.h into the include dirs. 
So if you have `#include "png.h"` in your code, you need this target. 
images.gyp doesn't export it.  Plus, I can imagine an executable or library that
needs knows how to render color emoji text, but never has to decode or encode
any other kind of image.

https://codereview.chromium.org/152513007/diff/1/gyp/giflib.gyp
File gyp/giflib.gyp (right):

https://codereview.chromium.org/152513007/diff/1/gyp/giflib.gyp#newcode7
gyp/giflib.gyp:7: 'conditions': [
On 2014/02/06 16:24:34, djsollen wrote:
> I'm not a big fan of defining each target 2x.  Why not have one target with
the
> condition that contains all differences?  Same goes for all the other files
you
> added.

Done.

https://codereview.chromium.org/152513007/diff/1/gyp/giflib.gyp#newcode35
gyp/giflib.gyp:35: 'libraries': [
On 2014/02/06 16:22:13, scroggo wrote:
> If this is correctly empty, it can be removed.

Done.

https://codereview.chromium.org/152513007/diff/1/gyp/giflib.gyp#newcode45
gyp/giflib.gyp:45: }, {
On 2014/02/06 16:22:13, scroggo wrote:
> I assume this is if 'skia_giflib_static' is false? Can you add a comment here
> reflecting that? It can be hard to visually grasp a gyp file, with its several
> layers of brackets and braces.

Done.

https://codereview.chromium.org/152513007/diff/1/gyp/images.gyp
File gyp/images.gyp (right):

https://codereview.chromium.org/152513007/diff/1/gyp/images.gyp#newcode20
gyp/images.gyp:20: 'libpng.gyp:libpng',
On 2014/02/06 16:22:13, scroggo wrote:
> nit: should line up with libjpeg.

Done.

https://codereview.chromium.org/152513007/diff/1/gyp/images.gyp#newcode20
gyp/images.gyp:20: 'libpng.gyp:libpng',
On 2014/02/06 16:24:34, djsollen wrote:
> spacing

Done.

https://codereview.chromium.org/152513007/diff/1/gyp/images.gyp#newcode124
gyp/images.gyp:124: # Any targets that depend on this target should link in
libpng, libgif, and
On 2014/02/06 16:24:34, djsollen wrote:
> this comment is out of date and should be removed.

Done.

https://codereview.chromium.org/152513007/diff/1/gyp/images.gyp#newcode124
gyp/images.gyp:124: # Any targets that depend on this target should link in
libpng, libgif, and
On 2014/02/06 16:22:13, scroggo wrote:
> This comment is out of date.

Done.

https://codereview.chromium.org/152513007/diff/1/gyp/images.gyp#newcode128
gyp/images.gyp:128: 'giflib.gyp:giflib'
On 2014/02/06 16:22:13, scroggo wrote:
> It appears that without this change, the images project will have these
> libraries as link_settings, but with it, giflib will have -lgif as a
> link_setting, and images will depend on giflib. Does that accomplish the same
> thing?

Exactly.  giflib.gyp uses direct_dependent_settings.

https://codereview.chromium.org/152513007/diff/1/gyp/libzlib.gyp
File gyp/libzlib.gyp (right):

https://codereview.chromium.org/152513007/diff/1/gyp/libzlib.gyp#newcode59
gyp/libzlib.gyp:59: 'export_dependent_settings': [
On 2014/02/06 16:22:13, scroggo wrote:
> This file has many empty lists. I don't think they're necessary.

Done.

https://codereview.chromium.org/152513007/diff/1/gyp/skflate.gyp
File gyp/skflate.gyp (right):

https://codereview.chromium.org/152513007/diff/1/gyp/skflate.gyp#newcode1
gyp/skflate.gyp:1: # Target for including zlib.
On 2014/02/06 16:24:34, djsollen wrote:
> fix this comment

Done.

[scroggo, 2014-02-06 18:14:50]

From my perspective this lgtm (with small nits).

https://codereview.chromium.org/152513007/diff/1/gyp/giflib.gyp
File gyp/giflib.gyp (right):

https://codereview.chromium.org/152513007/diff/1/gyp/giflib.gyp#newcode45
gyp/giflib.gyp:45: }, {
On 2014/02/06 18:01:57, Hal Canary wrote:
> On 2014/02/06 16:22:13, scroggo wrote:
> > I assume this is if 'skia_giflib_static' is false? Can you add a comment
here
> > reflecting that? It can be hard to visually grasp a gyp file, with its
several
> > layers of brackets and braces.
> 
> Done.

This seems to still be missing

https://codereview.chromium.org/152513007/diff/1/gyp/skflate.gyp
File gyp/skflate.gyp (right):

https://codereview.chromium.org/152513007/diff/1/gyp/skflate.gyp#newcode1
gyp/skflate.gyp:1: # Target for including zlib.
On 2014/02/06 18:01:57, Hal Canary wrote:
> On 2014/02/06 16:24:34, djsollen wrote:
> > fix this comment
> 
> Done.

I think a better fix is to change it to:

# Target for including SkFlate.

(Personally I like the comment at the top of the file - it tells VI to use
colors rather than show me a solid color.)

[djsollen, 2014-02-06 18:22:40]

https://codereview.chromium.org/152513007/diff/120001/DEPS
File DEPS (right):

https://codereview.chromium.org/152513007/diff/120001/DEPS#newcode45
DEPS:45: "third_party/externals/zlib" :
"https://github.com/madler/zlib.git@5089329",  # zlib.git@v1.2.8
is there a googlesource.com repo that has this?

https://codereview.chromium.org/152513007/diff/120001/gyp/freetype.gyp
File gyp/freetype.gyp (right):

https://codereview.chromium.org/152513007/diff/120001/gyp/freetype.gyp#newcode64
gyp/freetype.gyp:64: 'libpng.gyp:libpng',
Are you sure we need this?  Doesn't images.gyp:20 export all that we need?

https://codereview.chromium.org/152513007/diff/120001/gyp/zlib.gyp
File gyp/zlib.gyp (left):

https://codereview.chromium.org/152513007/diff/120001/gyp/zlib.gyp#oldcode1
gyp/zlib.gyp:1: # Target for including zlib.
just curious but why not repurpose this file instead of creating a new
libzlib.gyp file?

[hal.canary, 2014-02-06 21:01:48]

On 2014/02/06 18:14:50, scroggo wrote:
> On 2014/02/06 18:01:57, Hal Canary wrote:
> > On 2014/02/06 16:22:13, scroggo wrote:
> > > I assume this is if 'skia_giflib_static' is false? Can you add a comment
> here
> > > reflecting that? It can be hard to visually grasp a gyp file, with its
> several
> > > layers of brackets and braces.
> > 
> > Done.
> 
> This seems to still be missing

Fixed that.

[hal.canary, 2014-02-06 21:01:54]

https://codereview.chromium.org/152513007/diff/120001/DEPS
File DEPS (right):

https://codereview.chromium.org/152513007/diff/120001/DEPS#newcode45
DEPS:45: "third_party/externals/zlib" :
"https://github.com/madler/zlib.git@5089329",  # zlib.git@v1.2.8
On 2014/02/06 18:22:41, djsollen wrote:
> is there a http://googlesource.com repo that has this?

Done.

https://codereview.chromium.org/152513007/diff/120001/gyp/freetype.gyp
File gyp/freetype.gyp (right):

https://codereview.chromium.org/152513007/diff/120001/gyp/freetype.gyp#newcode64
gyp/freetype.gyp:64: 'libpng.gyp:libpng',
On 2014/02/06 18:22:41, djsollen wrote:
> Are you sure we need this?  Doesn't images.gyp:20 export all that we need?

Done.

https://codereview.chromium.org/152513007/diff/120001/gyp/zlib.gyp
File gyp/zlib.gyp (left):

https://codereview.chromium.org/152513007/diff/120001/gyp/zlib.gyp#oldcode1
gyp/zlib.gyp:1: # Target for including zlib.
On 2014/02/06 18:22:41, djsollen wrote:
> just curious but why not repurpose this file instead of creating a new
> libzlib.gyp file?

Done.

[hal.canary, 2014-02-06 21:05:13]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2014-02-06 21:05:19]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/halcanary@google.com/152513007/260001

[hal.canary, 2014-02-06 21:55:41]

The CQ bit was unchecked by halcanary@google.com

[hal.canary, 2014-02-18 20:58:01]

Leon, I made some minor changes to the gyp files to make sure it continues to
work on Android and Windows.  

I also added three scripts to the platform tools directory.  Eric, can you take
a look at those?

[scroggo, 2014-02-18 21:46:23]

https://codereview.chromium.org/152513007/diff/740001/gyp/common_variables.gypi
File gyp/common_variables.gypi (right):

https://codereview.chromium.org/152513007/diff/740001/gyp/common_variables.gy...
gyp/common_variables.gypi:100: 'skia_freetype_static%': 'skia_os in ["android",
"nacl"]',
It seems like these aren't really "common". Aren't they only used by
freetype.gyp? Similar for the others.

https://codereview.chromium.org/152513007/diff/740001/gyp/lua.gyp
File gyp/lua.gyp (right):

https://codereview.chromium.org/152513007/diff/740001/gyp/lua.gyp#newcode67
gyp/lua.gyp:67: "getlocaledecpoint()='.'",
Since this is common to both, what if we instead had the following:

'defines': [
  "getlocaledecpoint()='.'"
],
'conditions': [
  ['skia_os != "win", {
    'LUA_USE_POSIX',
  }],
],

https://codereview.chromium.org/152513007/diff/740001/gyp/most.gyp
File gyp/most.gyp (right):

https://codereview.chromium.org/152513007/diff/740001/gyp/most.gyp#newcode32
gyp/most.gyp:32: 'skia_lib.gyp:skia_lib',
skia_lib is not really a test. Better name for all_tests?

[borenet, 2014-02-18 22:15:35]

Scripts look okay, but I'm not a shell scripting expert.

https://codereview.chromium.org/152513007/diff/740001/gyp/common_variables.gypi
File gyp/common_variables.gypi (right):

https://codereview.chromium.org/152513007/diff/740001/gyp/common_variables.gy...
gyp/common_variables.gypi:100: 'skia_freetype_static%': 'skia_os in ["android",
"nacl"]',
On 2014/02/18 21:46:24, scroggo wrote:
> It seems like these aren't really "common". Aren't they only used by
> freetype.gyp? Similar for the others.

I could go either way, but I think it makes sense for any variables we might
want to modify through GYP_DEFINES to be in this file. It kind of documents
which options are available.

https://codereview.chromium.org/152513007/diff/740001/gyp/common_variables.gy...
gyp/common_variables.gypi:105: #     precedence.
I don't like these conflicting variables. "not skia_freetype_static" should be
sufficient.

https://codereview.chromium.org/152513007/diff/740001/gyp/freetype.gyp
File gyp/freetype.gyp (right):

https://codereview.chromium.org/152513007/diff/740001/gyp/freetype.gyp#newcode8
gyp/freetype.gyp:8: [ 'skia_freetype_dynamic and not skia_freetype_static',
This stuff is all specific to OS in that list.  I'd keep it guarded and nest
another conditional for skia_freetype_*.

https://codereview.chromium.org/152513007/diff/740001/gyp/most.gyp
File gyp/most.gyp (right):

https://codereview.chromium.org/152513007/diff/740001/gyp/most.gyp#newcode32
gyp/most.gyp:32: 'skia_lib.gyp:skia_lib',
On 2014/02/18 21:46:24, scroggo wrote:
> skia_lib is not really a test. Better name for all_tests?

I'm not sure why we need this target anyway, and I don't like having two copies
of almost the same list.  Why not modify the above list based on whether the
platform can run SampleApp?

[hal.canary, 2014-02-19 15:21:03]

https://codereview.chromium.org/152513007/diff/740001/gyp/common_variables.gypi
File gyp/common_variables.gypi (right):

https://codereview.chromium.org/152513007/diff/740001/gyp/common_variables.gy...
gyp/common_variables.gypi:100: 'skia_freetype_static%': 'skia_os in ["android",
"nacl"]',
On 2014/02/18 21:46:24, scroggo wrote:
> It seems like these aren't really "common". Aren't they only used by
> freetype.gyp? Similar for the others.

It looks like many of the variables are only used by a single other .gyp file.

https://codereview.chromium.org/152513007/diff/740001/gyp/common_variables.gy...
gyp/common_variables.gypi:105: #     precedence.
On 2014/02/18 22:15:36, borenet wrote:
> I don't like these conflicting variables. "not skia_freetype_static"
> should be sufficient.

More inspection indicated that if we got to freetype.gyp:freetype,
skia_freetype_dynamic was already defined.  So I can safely remove it.

https://codereview.chromium.org/152513007/diff/740001/gyp/freetype.gyp
File gyp/freetype.gyp (right):

https://codereview.chromium.org/152513007/diff/740001/gyp/freetype.gyp#newcode8
gyp/freetype.gyp:8: [ 'skia_freetype_dynamic and not skia_freetype_static',
On 2014/02/18 22:15:36, borenet wrote:
> This stuff is all specific to OS in that list.  I'd keep it guarded and nest
> another conditional for skia_freetype_*.

done

https://codereview.chromium.org/152513007/diff/740001/gyp/lua.gyp
File gyp/lua.gyp (right):

https://codereview.chromium.org/152513007/diff/740001/gyp/lua.gyp#newcode67
gyp/lua.gyp:67: "getlocaledecpoint()='.'",
On 2014/02/18 21:46:24, scroggo wrote:
> Since this is common to both, what if we instead had the following:
> 
> 'defines': [
>   "getlocaledecpoint()='.'"
> ],
> 'conditions': [
>   ['skia_os != "win", {
>     'LUA_USE_POSIX',
>   }],
> ],

Done.

https://codereview.chromium.org/152513007/diff/740001/gyp/most.gyp
File gyp/most.gyp (right):

https://codereview.chromium.org/152513007/diff/740001/gyp/most.gyp#newcode32
gyp/most.gyp:32: 'skia_lib.gyp:skia_lib',
On 2014/02/18 21:46:24, scroggo wrote:
> skia_lib is not really a test. Better name for all_tests?

True.  I don't need it here since all of these dependencies already depend on
it.

https://codereview.chromium.org/152513007/diff/740001/gyp/most.gyp#newcode32
gyp/most.gyp:32: 'skia_lib.gyp:skia_lib',
On 2014/02/18 22:15:36, borenet wrote:
> On 2014/02/18 21:46:24, scroggo wrote:
> > skia_lib is not really a test. Better name for all_tests?
> 
> I'm not sure why we need this target anyway, and I don't like having two
copies
> of almost the same list.  Why not modify the above list based on whether the
> platform can run SampleApp?

I added a variable to disable by default building executables that depend on GUI
libraries instead of having a new build target.

[djsollen, 2014-02-19 15:40:00]

https://codereview.chromium.org/152513007/diff/780002/platform_tools/barelinu...
File platform_tools/barelinux/bin/arm64_download (right):

https://codereview.chromium.org/152513007/diff/780002/platform_tools/barelinu...
platform_tools/barelinux/bin/arm64_download:61: try gsutil_check_get
"gs://${location}/${toolchain}.tar.xz" \
you can do this without requiring gsutil.

http://chromium-skia-gm.commondatastorage.googleapis.com/[bucket_name]/[file_...

[hal.canary, 2014-02-19 15:43:55]

https://codereview.chromium.org/152513007/diff/780002/platform_tools/barelinu...
File platform_tools/barelinux/bin/arm64_download (right):

https://codereview.chromium.org/152513007/diff/780002/platform_tools/barelinu...
platform_tools/barelinux/bin/arm64_download:61: try gsutil_check_get
"gs://${location}/${toolchain}.tar.xz" \
On 2014/02/19 15:40:00, djsollen wrote:
> you can do this without requiring gsutil.
> 
>
http://chromium-skia-gm.commondatastorage.googleapis.com/%5Bbucket_name%5D/%5...

These files have access control.

[scroggo, 2014-02-19 18:33:57]

https://codereview.chromium.org/152513007/diff/780002/gyp/freetype.gyp
File gyp/freetype.gyp (right):

https://codereview.chromium.org/152513007/diff/780002/gyp/freetype.gyp#newcode19
gyp/freetype.gyp:19: 'SK_FONTHOST_FREETYPE_RUNTIME_VERSION=0x020400',
If I understand Eric's comment in
https://codereview.chromium.org/152513007/diff/740001/gyp/freetype.gyp#newcode8,
these should perhaps depend on the OS, not on skia_freetype_static. I could
imagine a situation where we added a new OS which links freetype statically, and
got the side effect of setting the freetype runtime version to 2.4 and
preventing Skia from using dlopen.

[borenet, 2014-02-21 15:24:34]

https://codereview.chromium.org/152513007/diff/780002/gyp/common_variables.gypi
File gyp/common_variables.gypi (right):

https://codereview.chromium.org/152513007/diff/780002/gyp/common_variables.gy...
gyp/common_variables.gypi:100: 'skia_freetype_static%': 'skia_os in ["android",
"nacl"]',
Not sure if it's just convention or what, but this type of thing is typically
done in a 'conditions' block.

https://codereview.chromium.org/152513007/diff/780002/gyp/freetype.gyp
File gyp/freetype.gyp (right):

https://codereview.chromium.org/152513007/diff/780002/gyp/freetype.gyp#newcode19
gyp/freetype.gyp:19: 'SK_FONTHOST_FREETYPE_RUNTIME_VERSION=0x020400',
On 2014/02/19 18:33:57, scroggo wrote:
> If I understand Eric's comment in
>
https://codereview.chromium.org/152513007/diff/740001/gyp/freetype.gyp#newcode8,
> these should perhaps depend on the OS, not on skia_freetype_static. I could
> imagine a situation where we added a new OS which links freetype statically,
and
> got the side effect of setting the freetype runtime version to 2.4 and
> preventing Skia from using dlopen.

Yes, everything in this block only makes sense on the OS which were listed
before.  I think we should keep these under the original conditional.

[hal.canary, 2014-02-24 17:42:35]

https://codereview.chromium.org/152513007/diff/780002/gyp/common_variables.gypi
File gyp/common_variables.gypi (right):

https://codereview.chromium.org/152513007/diff/780002/gyp/common_variables.gy...
gyp/common_variables.gypi:100: 'skia_freetype_static%': 'skia_os in ["android",
"nacl"]',
On 2014/02/21 15:24:34, borenet wrote:
> Not sure if it's just convention or what, but this type of thing is typically
> done in a 'conditions' block.

Done.

https://codereview.chromium.org/152513007/diff/780002/gyp/freetype.gyp
File gyp/freetype.gyp (right):

https://codereview.chromium.org/152513007/diff/780002/gyp/freetype.gyp#newcode19
gyp/freetype.gyp:19: 'SK_FONTHOST_FREETYPE_RUNTIME_VERSION=0x020400',
On 2014/02/19 18:33:57, scroggo wrote:
> If I understand Eric's comment in
>
https://codereview.chromium.org/152513007/diff/740001/gyp/freetype.gyp#newcode8,
> these should perhaps depend on the OS, not on skia_freetype_static. I could
> imagine a situation where we added a new OS which links freetype statically,
and
> got the side effect of setting the freetype runtime version to 2.4 and
> preventing Skia from using dlopen.

I think I've got it right now.

[scroggo, 2014-02-24 17:51:39]

https://codereview.chromium.org/152513007/diff/1000001/gyp/freetype.gyp
File gyp/freetype.gyp (right):

https://codereview.chromium.org/152513007/diff/1000001/gyp/freetype.gyp#newco...
gyp/freetype.gyp:18: # Both Android and NaCl provide at least FreeType 2.4.0
Like you've done with the rest of the linux defines, these should be dependent
on skia_os in ["android", "nacl"]

https://codereview.chromium.org/152513007/diff/1000001/gyp/freetype.gyp#newco...
gyp/freetype.gyp:62: # Remove these, and use the system's freetype instead.
This comment is no longer needed.

[hal.canary, 2014-02-24 18:57:50]

https://codereview.chromium.org/152513007/diff/1000001/gyp/freetype.gyp
File gyp/freetype.gyp (right):

https://codereview.chromium.org/152513007/diff/1000001/gyp/freetype.gyp#newco...
gyp/freetype.gyp:18: # Both Android and NaCl provide at least FreeType 2.4.0
On 2014/02/24 17:51:40, scroggo wrote:
> Like you've done with the rest of the linux defines, these should be dependent
> on skia_os in ["android", "nacl"]

Done.

https://codereview.chromium.org/152513007/diff/1000001/gyp/freetype.gyp#newco...
gyp/freetype.gyp:62: # Remove these, and use the system's freetype instead.
On 2014/02/24 17:51:40, scroggo wrote:
> This comment is no longer needed.

Done.

[scroggo, 2014-02-24 19:19:47]

On 2014/02/24 18:57:50, Hal Canary wrote:
> https://codereview.chromium.org/152513007/diff/1000001/gyp/freetype.gyp
> File gyp/freetype.gyp (right):
> 
>
https://codereview.chromium.org/152513007/diff/1000001/gyp/freetype.gyp#newco...
> gyp/freetype.gyp:18: # Both Android and NaCl provide at least FreeType 2.4.0
> On 2014/02/24 17:51:40, scroggo wrote:
> > Like you've done with the rest of the linux defines, these should be
dependent
> > on skia_os in ["android", "nacl"]
> 
> Done.
> 
>
https://codereview.chromium.org/152513007/diff/1000001/gyp/freetype.gyp#newco...
> gyp/freetype.gyp:62: # Remove these, and use the system's freetype instead.
> On 2014/02/24 17:51:40, scroggo wrote:
> > This comment is no longer needed.
> 
> Done.

The gyp changes lgtm. I can't really speak to the shell scripts.

[hal.canary, 2014-02-24 19:26:47]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2014-02-24 19:26:57]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/halcanary@google.com/152513007/1100002

[hal.canary, 2014-02-24 19:27:22]

On 2014/02/24 19:19:47, scroggo wrote:
> The gyp changes lgtm. I can't really speak to the shell scripts.

The scripts won't break anything, so I'll go ahead and commit this.

[commit-bot: I haz the power, 2014-02-24 20:21:52]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/halcanary@google.com/152513007/1100002

[commit-bot: I haz the power, 2014-02-24 20:22:43]

Change committed as 13570

[borenet, 2014-02-24 20:46:57]

LGTM as well.  Also not a shell script expert.

https://codereview.chromium.org/152513007/diff/1100002/gyp/common_variables.gypi
File gyp/common_variables.gypi (right):

https://codereview.chromium.org/152513007/diff/1100002/gyp/common_variables.g...
gyp/common_variables.gypi:121: #     don't do that; instead compile and
staticlly link the version of
Nit: spelling

https://codereview.chromium.org/152513007/diff/1100002/platform_tools/barelin...
File platform_tools/barelinux/bin/barelinux_make (right):

https://codereview.chromium.org/152513007/diff/1100002/platform_tools/barelin...
platform_tools/barelinux/bin/barelinux_make:62: return $ret
Why not exit here, since you do " || exit" everywhere this is used?