change default build (in "make" wrapper) to ninja on all platforms

change default build (in "make" wrapper) to ninja on all platforms

This will change the build method used by bots, and also developers who run the toplevel "fake-make" wrapper.

DO NOT COMMIT until Chrome M35 branches, in order to not interfere in that process.

manually committed as https://code.google.com/p/skia/source/detail?r=14003

BUG=skia:2317, skia:2348, skia:2338
NOTRY=True

[epoger, 2014-03-21 19:17:23]

adding "EXCEPT Android" to the description; I'm going to modify the patch to
leave Android's build alone

[epoger, 2014-03-24 17:25:33]

Ben, Eric, and Derek- please review at patchset 4.

As noted in the description, I do not intend to commit this before the Chrome
M35 branch.  But I would appreciate your recommendations on:
- other trybots I should run (see trybots I already ran at various patchsets)
- modifications to the approach as a whole
- anything else you think of

[borenet, 2014-03-24 17:28:09]

I think I'd rather us be honest and just remove the Makefile and run Ninja
directly.

[epoger, 2014-03-24 17:29:54]

On 2014/03/24 17:28:09, borenet wrote:
> I think I'd rather us be honest and just remove the Makefile and run Ninja
> directly.

I'm fine with that, but that will require us to change the buildbot scripts (and
thus is not amenable to running through trybots to test).

My suggestion is:
1. change the wrapper to call ninja
2. make sure it works everywhere
3. stop using the wrapper

[epoger, 2014-03-24 17:32:03]

On 2014/03/24 17:29:54, epoger wrote:
> On 2014/03/24 17:28:09, borenet wrote:
> > I think I'd rather us be honest and just remove the Makefile and run Ninja
> > directly.
> 
> I'm fine with that, but that will require us to change the buildbot scripts
(and
> thus is not amenable to running through trybots to test).
> 
> My suggestion is:
> 1. change the wrapper to call ninja
> 2. make sure it works everywhere
> 3. stop using the wrapper

P.S. One "charm" of the fake-make wrapper script (for some definitions of
"charm") is that individual developers who don't care whether they are using
make, xcode, ninja etc. get the new behavior without changing the commands they
run.  Their build process is automatically kept in sync with that used by the
buildbots.

[borenet, 2014-03-24 17:39:03]

Code LGTM.  I don't feel too strongly, but my preference is:
- Change the bots to run ninja directly (maybe stage the rollout by platform)
- Change the documentation to reflect the new workflow
- Remove the "make" script

To me, this keeps us more in line with the "single blessed workflow" philosophy.

[epoger, 2014-03-24 17:46:07]

On 2014/03/24 17:39:03, borenet wrote:
> Code LGTM.  I don't feel too strongly, but my preference is:
> - Change the bots to run ninja directly (maybe stage the rollout by platform)
> - Change the documentation to reflect the new workflow
> - Remove the "make" script
> 
> To me, this keeps us more in line with the "single blessed workflow"
philosophy.

I am a strong proponent of the "single blessed workflow" philosophy.

Unless there is strong opposition, I advocate this combination of Eric's and my
proposed steps:

1. change the "fake make" wrapper to call ninja
2. if it fails on any platforms, revert, experiment further, and then goto step
1
3. change the bots to run ninja directly
4. change the documentation to reflect the new workflow
5. remove the "fake make" wrapper script

(There is also the issue of Android!  My hope is that we can shift that build to
ninja as well in parallel with steps 1/2 above.)

[epoger, 2014-03-25 17:05:02]

ping bungeman and djsollen

[bungeman-skia, 2014-03-25 17:10:04]

sgtm/lgtm for the change and general plan, at least on the bits that I know
something about (so +1?).

[djsollen, 2014-03-25 17:34:05]

I have the same sentiment as ben.  Also, if we can figure out a way to get the
android_ninja build working on mac we won't need to keep this around for android
either.

[epoger, 2014-03-25 17:45:58]

On 2014/03/25 17:34:05, djsollen wrote:
> I have the same sentiment as ben.  Also, if we can figure out a way to get the
> android_ninja build working on mac we won't need to keep this around for
android
> either.

Filed a separate bug for that one to you, Derek:
https://code.google.com/p/skia/issues/detail?id=2338 ('switch to ninja for
Android builds')

[epoger, 2014-03-27 18:46:36]

As of patchset 6, I think this CL is "ready for prime time".

I plan to commit it on the morning of Tuesday, April 1.  No foolin'...

[epoger, 2014-03-31 15:30:06]

As of patchset 9, uses ninja to build Android too, so now it covers ALL
platforms.  Sending some trybots to test it now...

[mtklein, 2014-03-31 15:41:30]

On 2014/03/31 15:30:06, epoger wrote:
> As of patchset 9, uses ninja to build Android too, so now it covers ALL
> platforms.  Sending some trybots to test it now...

Just curious, do the Android bots use this top-level Makefile?  How does
platform_tools/android/bin/android_setup.sh get sourced?  I always assumed they
were running platform_tools/android/bin/android_make directly.

[epoger, 2014-03-31 15:49:51]

On 2014/03/31 15:41:30, mtklein wrote:
> On 2014/03/31 15:30:06, epoger wrote:
> > As of patchset 9, uses ninja to build Android too, so now it covers ALL
> > platforms.  Sending some trybots to test it now...
> 
> Just curious, do the Android bots use this top-level Makefile?  How does
> platform_tools/android/bin/android_setup.sh get sourced?  I always assumed
they
> were running platform_tools/android/bin/android_make directly.

I had the same question.

Looking at
http://108.170.220.76:10117/builders/Build-Ubuntu12-GCC-Arm7-Release-GalaxyNe...
, we can see that the buildbot calls "platform_tools/android/bin/android_make" .

Looking at
https://skia.googlesource.com/skia/+/master/platform_tools/android/bin/androi...
, we can see that this was in turn calling "GYP_GENERATORS=make-android make
$TARGETS", and thus launching the toplevel wrapper Makefile.

As of patchset 9, platform_tools/android/bin/android_make does not call the
toplevel Makefile wrapper anymore.

[mtklein, 2014-03-31 15:52:28]

On 2014/03/31 15:49:51, epoger wrote:
> On 2014/03/31 15:41:30, mtklein wrote:
> > On 2014/03/31 15:30:06, epoger wrote:
> > > As of patchset 9, uses ninja to build Android too, so now it covers ALL
> > > platforms.  Sending some trybots to test it now...
> > 
> > Just curious, do the Android bots use this top-level Makefile?  How does
> > platform_tools/android/bin/android_setup.sh get sourced?  I always assumed
> they
> > were running platform_tools/android/bin/android_make directly.
> 
> I had the same question.
> 
> Looking at
>
http://108.170.220.76:10117/builders/Build-Ubuntu12-GCC-Arm7-Release-GalaxyNe...
> , we can see that the buildbot calls "platform_tools/android/bin/android_make"
.
> 
> Looking at
>
https://skia.googlesource.com/skia/+/master/platform_tools/android/bin/androi...
> , we can see that this was in turn calling "GYP_GENERATORS=make-android make
> $TARGETS", and thus launching the toplevel wrapper Makefile.
> 
> As of patchset 9, platform_tools/android/bin/android_make does not call the
> toplevel Makefile wrapper anymore.

Ah, I see!  So it went the other way around.  That makes sense.  Thanks!

[djsollen, 2014-03-31 16:00:33]

https://codereview.chromium.org/206463007/diff/200001/platform_tools/android/...
File platform_tools/android/bin/android_make (left):

https://codereview.chromium.org/206463007/diff/200001/platform_tools/android/...
platform_tools/android/bin/android_make:14: if [ $(basename $0) = "android_make"
]; then
can we keep this if statement and instead print a warning (see suggestion below)
and then just run with ninja?

WARNING: The android_make script is unsupported and only serves as a wrapper for
android_ninja. This wrapper will be removed soon so please update your workflow.

[mtklein, 2014-03-31 16:04:18]

https://codereview.chromium.org/206463007/diff/200001/platform_tools/android/...
File platform_tools/android/bin/android_make (left):

https://codereview.chromium.org/206463007/diff/200001/platform_tools/android/...
platform_tools/android/bin/android_make:14: if [ $(basename $0) = "android_make"
]; then
On 2014/03/31 16:00:34, djsollen wrote:
> can we keep this if statement and instead print a warning (see suggestion
below)
> and then just run with ninja?
> 
> WARNING: The android_make script is unsupported and only serves as a wrapper
for
> android_ninja. This wrapper will be removed soon so please update your
workflow.

android_ninja is a symlink to android_make.  I think this is TODO: have the bots
run android_ninja, then rename android_make to android_ninja, removing the
symlink

[epoger, 2014-03-31 16:08:56]

On 2014/03/31 16:04:18, mtklein wrote:
>
https://codereview.chromium.org/206463007/diff/200001/platform_tools/android/...
> File platform_tools/android/bin/android_make (left):
> 
>
https://codereview.chromium.org/206463007/diff/200001/platform_tools/android/...
> platform_tools/android/bin/android_make:14: if [ $(basename $0) =
"android_make"
> ]; then
> On 2014/03/31 16:00:34, djsollen wrote:
> > can we keep this if statement and instead print a warning (see suggestion
> below)
> > and then just run with ninja?
> > 
> > WARNING: The android_make script is unsupported and only serves as a wrapper
> for
> > android_ninja. This wrapper will be removed soon so please update your
> workflow.
> 
> android_ninja is a symlink to android_make.  I think this is TODO: have the
bots
> run android_ninja, then rename android_make to android_ninja, removing the
> symlink

Counter-counter proposal:

As part of https://code.google.com/p/skia/issues/detail?id=2362 ('replace
"fake-make" build wrappers with build.py'):
1. Copy android_make to new android_build file.  Add warning message to
android_make (and hence android_ninja): "you should start running android_build
instead".
2. Change the bots to call android_build instead of android_make
3. Change the warning in android_make (and android_ninja) to an error
4. Delete android_make and android_ninja

[mtklein, 2014-03-31 16:15:45]

On 2014/03/31 16:08:56, epoger wrote:
> On 2014/03/31 16:04:18, mtklein wrote:
> >
>
https://codereview.chromium.org/206463007/diff/200001/platform_tools/android/...
> > File platform_tools/android/bin/android_make (left):
> > 
> >
>
https://codereview.chromium.org/206463007/diff/200001/platform_tools/android/...
> > platform_tools/android/bin/android_make:14: if [ $(basename $0) =
> "android_make"
> > ]; then
> > On 2014/03/31 16:00:34, djsollen wrote:
> > > can we keep this if statement and instead print a warning (see suggestion
> > below)
> > > and then just run with ninja?
> > > 
> > > WARNING: The android_make script is unsupported and only serves as a
wrapper
> > for
> > > android_ninja. This wrapper will be removed soon so please update your
> > workflow.
> > 
> > android_ninja is a symlink to android_make.  I think this is TODO: have the
> bots
> > run android_ninja, then rename android_make to android_ninja, removing the
> > symlink
> 
> Counter-counter proposal:
> 
> As part of https://code.google.com/p/skia/issues/detail?id=2362 ('replace
> "fake-make" build wrappers with build.py'):
> 1. Copy android_make to new android_build file.  Add warning message to
> android_make (and hence android_ninja): "you should start running
android_build
> instead".
> 2. Change the bots to call android_build instead of android_make
> 3. Change the warning in android_make (and android_ninja) to an error
> 4. Delete android_make and android_ninja

I don't follow why android_build needs to exist.  If you just change the bots to
run android_ninja, we can clean up the rest without bothering bots.

[epoger, 2014-03-31 16:20:58]

On 2014/03/31 16:15:45, mtklein wrote:
> On 2014/03/31 16:08:56, epoger wrote:
> > On 2014/03/31 16:04:18, mtklein wrote:
> > >
> >
>
https://codereview.chromium.org/206463007/diff/200001/platform_tools/android/...
> > > File platform_tools/android/bin/android_make (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/206463007/diff/200001/platform_tools/android/...
> > > platform_tools/android/bin/android_make:14: if [ $(basename $0) =
> > "android_make"
> > > ]; then
> > > On 2014/03/31 16:00:34, djsollen wrote:
> > > > can we keep this if statement and instead print a warning (see
suggestion
> > > below)
> > > > and then just run with ninja?
> > > > 
> > > > WARNING: The android_make script is unsupported and only serves as a
> wrapper
> > > for
> > > > android_ninja. This wrapper will be removed soon so please update your
> > > workflow.
> > > 
> > > android_ninja is a symlink to android_make.  I think this is TODO: have
the
> > bots
> > > run android_ninja, then rename android_make to android_ninja, removing the
> > > symlink
> > 
> > Counter-counter proposal:
> > 
> > As part of https://code.google.com/p/skia/issues/detail?id=2362 ('replace
> > "fake-make" build wrappers with build.py'):
> > 1. Copy android_make to new android_build file.  Add warning message to
> > android_make (and hence android_ninja): "you should start running
> android_build
> > instead".
> > 2. Change the bots to call android_build instead of android_make
> > 3. Change the warning in android_make (and android_ninja) to an error
> > 4. Delete android_make and android_ninja
> 
> I don't follow why android_build needs to exist.  If you just change the bots
to
> run android_ninja, we can clean up the rest without bothering bots.

I think the point of android_build is exactly in line with
https://code.google.com/p/skia/issues/detail?id=2362 ('replace "fake-make" build
wrappers with build.py') .  We maintain a script that shows developers how to
build, instead of documentation to do so.

My giving it a generic name ("build" instead of "ninja"), we don't have to worry
about the name being misleading when we change its inner workings.

On a related note, there are some trybot failures for me to deal with, so I'm
going to have to make a change to the buildbots today at any rate:
1.
http://108.170.220.76:10117/builders/Build-Ubuntu12-GCC-Arm7-Debug-GalaxyNexu...
: I will have to make the buildbots stop passing --jobs arguments (and when
doing that, I may as well make them call android_ninja instead of android_make)
2.
http://108.170.220.76:10117/builders/Build-Mac10.7-Clang-Arm7-Release-iOS-Try...
: filed https://code.google.com/p/skia/issues/detail?id=2363 ('ninja build for
iOS fails')

[epoger, 2014-03-31 20:56:41]

Patchset 10: Thanks to
https://skia.googlesource.com/buildbot/+/73a028bfb6a2e5ef1300728dbaf660af6881...
('reland "make bots call android_ninja instead of android_make"'), we don't need
to fuss with android_make anymore.  So I removed that file from the CL; nothing
else changed.

Assuming that the trybots look good at patchset 10, I will commit on Tuesday
morning.

[epoger, 2014-04-01 06:28:29]

The CQ bit was checked by epoger@google.com

[commit-bot: I haz the power, 2014-04-01 06:28:33]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/epoger@google.com/206463007/220001

[commit-bot: I haz the power, 2014-04-01 06:28:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-01 06:28:38]

Failed to apply patch for make.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file make.py
  Hunk #1 FAILED at 42.
  Hunk #2 FAILED at 99.
  Hunk #3 FAILED at 184.
  3 out of 3 hunks FAILED -- saving rejects to file make.py.rej

Patch:       make.py
Index: make.py
diff --git a/make.py b/make.py
index
28f3554a492076fbeeb3a532dccca54fcf9061a4..306a977c4301afa87f355734f279e13f08e01a4c
100644
--- a/make.py
+++ b/make.py
@@ -42,11 +42,6 @@ def rmtree(path):
     print '> rmtree %s' % path
     shutil.rmtree(path, ignore_errors=True)
 
-def mkdirs(path):
-    print '> mkdirs %s' % path
-    if not os.path.isdir(path):
-        os.makedirs(path)
-
 def runcommand(command):
     print '> %s' % command
     if os.system(command):
@@ -99,30 +94,21 @@ def MakeWindows(targets):
     parameters:
         targets: build targets as a list of strings
     """
+    # TODO(epoger): I'm not sure if this is needed for ninja builds.
     CheckWindowsEnvironment()
 
     # Run gyp_skia to prepare Visual Studio projects.
     cd(SCRIPT_DIR)
     runcommand('python gyp_skia')
 
-    # Prepare final output dir into which we will copy all binaries.
-    msbuild_working_dir = os.path.join(SCRIPT_DIR, OUT_SUBDIR, GYP_SUBDIR)
-    msbuild_output_dir = os.path.join(msbuild_working_dir, BUILDTYPE)
-    final_output_dir = os.path.join(SCRIPT_DIR, OUT_SUBDIR, BUILDTYPE)
-    mkdirs(final_output_dir)
+    # We already built the gypfiles...
+    while TARGET_GYP in targets:
+        targets.remove(TARGET_GYP)
 
-    for target in targets:
-        # We already built the gypfiles...
-        if target == TARGET_GYP:
-            continue
-
-        cd(msbuild_working_dir)
-        runcommand(
-            ('msbuild /nologo /property:Configuration=%s'
-            ' /target:%s /verbosity:minimal %s.sln') % (
-                BUILDTYPE, target, target))
-        runcommand('xcopy /y %s\* %s' % (
-            msbuild_output_dir, final_output_dir))
+    # And call ninja to do the work!
+    if targets:
+        runcommand('ninja -C %s %s' % (
+            os.path.join(OUT_SUBDIR, BUILDTYPE), ' '.join(targets)))
 
 
 def Make(args):
@@ -184,5 +170,3 @@ def Make(args):
 
 # main()
 Make(sys.argv[1:])
-
-

[epoger, 2014-04-01 06:41:11]

The CQ bit was checked by epoger@google.com

[commit-bot: I haz the power, 2014-04-01 06:41:16]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/epoger@google.com/206463007/240001

[commit-bot: I haz the power, 2014-04-01 06:41:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-01 06:41:19]

Failed to apply patch for make.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file make.py
  Hunk #1 FAILED at 42.
  Hunk #2 FAILED at 99.
  Hunk #3 FAILED at 184.
  3 out of 3 hunks FAILED -- saving rejects to file make.py.rej

Patch:       make.py
Index: make.py
diff --git a/make.py b/make.py
index
28f3554a492076fbeeb3a532dccca54fcf9061a4..306a977c4301afa87f355734f279e13f08e01a4c
100644
--- a/make.py
+++ b/make.py
@@ -42,11 +42,6 @@ def rmtree(path):
     print '> rmtree %s' % path
     shutil.rmtree(path, ignore_errors=True)
 
-def mkdirs(path):
-    print '> mkdirs %s' % path
-    if not os.path.isdir(path):
-        os.makedirs(path)
-
 def runcommand(command):
     print '> %s' % command
     if os.system(command):
@@ -99,30 +94,21 @@ def MakeWindows(targets):
     parameters:
         targets: build targets as a list of strings
     """
+    # TODO(epoger): I'm not sure if this is needed for ninja builds.
     CheckWindowsEnvironment()
 
     # Run gyp_skia to prepare Visual Studio projects.
     cd(SCRIPT_DIR)
     runcommand('python gyp_skia')
 
-    # Prepare final output dir into which we will copy all binaries.
-    msbuild_working_dir = os.path.join(SCRIPT_DIR, OUT_SUBDIR, GYP_SUBDIR)
-    msbuild_output_dir = os.path.join(msbuild_working_dir, BUILDTYPE)
-    final_output_dir = os.path.join(SCRIPT_DIR, OUT_SUBDIR, BUILDTYPE)
-    mkdirs(final_output_dir)
+    # We already built the gypfiles...
+    while TARGET_GYP in targets:
+        targets.remove(TARGET_GYP)
 
-    for target in targets:
-        # We already built the gypfiles...
-        if target == TARGET_GYP:
-            continue
-
-        cd(msbuild_working_dir)
-        runcommand(
-            ('msbuild /nologo /property:Configuration=%s'
-            ' /target:%s /verbosity:minimal %s.sln') % (
-                BUILDTYPE, target, target))
-        runcommand('xcopy /y %s\* %s' % (
-            msbuild_output_dir, final_output_dir))
+    # And call ninja to do the work!
+    if targets:
+        runcommand('ninja -C %s %s' % (
+            os.path.join(OUT_SUBDIR, BUILDTYPE), ' '.join(targets)))
 
 
 def Make(args):
@@ -184,5 +170,3 @@ def Make(args):
 
 # main()
 Make(sys.argv[1:])
-
-