Update gyp and make to allow alternative out directories

Update gyp to allow alternative out directories

If the enviroment variable SKIA_OUT is set the output of both
gyp and make will be redirected to that directory.

Committed: https://code.google.com/p/skia/source/detail?r=6578

[EricB, 2012-11-20 19:41:36]

https://codereview.appspot.com/6782095/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/6782095/diff/1/Makefile#newcode75
Makefile:75: rm -rf out xcodebuild
Why not change this line to read:
rm -rf $(SKIA_OUT) xcodebuild
?

https://codereview.appspot.com/6782095/diff/1/Makefile#newcode104
Makefile:104: $(CWD)/gyp_skia
This file is really inconsistent with its use of tabs and spaces.  My
understanding is that tabs should always be used.  Are we sure that everything
is working as expected?

https://codereview.appspot.com/6782095/diff/1/gyp_skia
File gyp_skia (right):

https://codereview.appspot.com/6782095/diff/1/gyp_skia#newcode60
gyp_skia:60: if output_dir is None:
More pythonic: "if not output_dir:"

https://codereview.appspot.com/6782095/diff/1/gyp_skia#newcode66
gyp_skia:66: not 'make' in os.getenv('GYP_GENERATORS')))):
Suggest rewriting this expression to read:

if os.name != 'posix' or \
   (sys.platform.startswith('darwin') and \
    (not os.getenv('GYP_GENERATORS') or \
     'make' not in os.getenv('GYP_GENERATORS'))):

I think the \'s are necessary to keep python from complaining.

https://codereview.appspot.com/6782095/diff/1/gyp_skia#newcode67
gyp_skia:67: print "ERROR: variable SKIA_OUT is not valid on Mac (using
xcodebuild) or Windows"
 > 80 chars.  You can break strings like this:

print 'Error: variable SKIA_OUT is not valid on Mac ' \
      '(using xcodebuild) or Windows'

https://codereview.appspot.com/6782095/diff/1/gyp_skia#newcode70
gyp_skia:70: if os.path.isabs(output_dir) is True:
"is True" isn't necessary here.

[epoger, 2012-11-20 20:42:36]

https://codereview.appspot.com/6782095/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/6782095/diff/1/Makefile#newcode97
Makefile:97: $(MAKE) out/Makefile
I guess you need to replace this, and any other appearance of "out"?

https://codereview.appspot.com/6782095/diff/1/Makefile#newcode104
Makefile:104: $(CWD)/gyp_skia
On 2012/11/20 19:41:36, EricB wrote:
> This file is really inconsistent with its use of tabs and spaces.  My
> understanding is that tabs should always be used.  Are we sure that everything
> is working as expected?

IIRC 'make' is very particular about it, and wants real tab characters instead
of spaces.  So yes, it would be good to test as many of the changed code paths
as possible.

https://codereview.appspot.com/6782095/diff/1/gyp_skia
File gyp_skia (right):

https://codereview.appspot.com/6782095/diff/1/gyp_skia#newcode58
gyp_skia:58: output_dir = os.getenv('SKIA_OUT')
I don't know whether things will still work if the specified output_dir is at a
different depth than ./out .  You might want to try that out, and add a comment
indicating what the constraints are (if any).

[DerekS, 2012-11-20 21:27:42]

still need to test that makefile change on linux as my previous testing has been
done on the mac.

https://codereview.appspot.com/6782095/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/6782095/diff/1/Makefile#newcode75
Makefile:75: rm -rf out xcodebuild
On 2012/11/20 19:41:36, EricB wrote:
> Why not change this line to read:
> rm -rf $(SKIA_OUT) xcodebuild
> ?

I wanted to preserve the old behavior of always cleaning out and debug in case
you had previously put stuff there.

https://codereview.appspot.com/6782095/diff/1/Makefile#newcode77
Makefile:77: rm -rf $(SKIA_OUT)
this is actually a tab in my source file. I'm not sure why it doesn't show up
here with the red arrow.

https://codereview.appspot.com/6782095/diff/1/Makefile#newcode97
Makefile:97: $(MAKE) out/Makefile
On 2012/11/20 20:42:36, epoger wrote:
> I guess you need to replace this, and any other appearance of "out"?

I do need to replace it here, but not in all cases as I don't support the
standard xcode build so I just leave it and it's symlinks as is.

https://codereview.appspot.com/6782095/diff/1/Makefile#newcode104
Makefile:104: $(CWD)/gyp_skia
On 2012/11/20 19:41:36, EricB wrote:
> This file is really inconsistent with its use of tabs and spaces.  My
> understanding is that tabs should always be used.  Are we sure that everything
> is working as expected?

I agree that it should be tabs and all the edits I made used tabs for
indentation.

https://codereview.appspot.com/6782095/diff/1/gyp_skia
File gyp_skia (right):

https://codereview.appspot.com/6782095/diff/1/gyp_skia#newcode58
gyp_skia:58: output_dir = os.getenv('SKIA_OUT')
On 2012/11/20 20:42:36, epoger wrote:
> I don't know whether things will still work if the specified output_dir is at
a
> different depth than ./out .  You might want to try that out, and add a
comment
> indicating what the constraints are (if any).

Yes, it will work at any depth but requires you to run make -C to point it at
the correct makefile.

https://codereview.appspot.com/6782095/diff/1/gyp_skia#newcode60
gyp_skia:60: if output_dir is None:
On 2012/11/20 19:41:36, EricB wrote:
> More pythonic: "if not output_dir:"

Done.

https://codereview.appspot.com/6782095/diff/1/gyp_skia#newcode67
gyp_skia:67: print "ERROR: variable SKIA_OUT is not valid on Mac (using
xcodebuild) or Windows"
On 2012/11/20 19:41:36, EricB wrote:
>  > 80 chars.  You can break strings like this:
> 
> print 'Error: variable SKIA_OUT is not valid on Mac ' \
>       '(using xcodebuild) or Windows'

Done.

https://codereview.appspot.com/6782095/diff/1/gyp_skia#newcode70
gyp_skia:70: if os.path.isabs(output_dir) is True:
On 2012/11/20 19:41:36, EricB wrote:
> "is True" isn't necessary here.

Done.

[epoger, 2012-11-27 17:11:03]

LGTM

Some minor suggestions, nothing to block submission...

https://codereview.appspot.com/6782095/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/6782095/diff/1/Makefile#newcode75
Makefile:75: rm -rf out xcodebuild
On 2012/11/20 21:27:43, DerekS wrote:
> On 2012/11/20 19:41:36, EricB wrote:
> > Why not change this line to read:
> > rm -rf $(SKIA_OUT) xcodebuild
> > ?
> 
> I wanted to preserve the old behavior of always cleaning out and debug in case
> you had previously put stuff there.

It seems strange to me that we would clean out a directory other than SKIA_OUT. 
It's bad enough that we have to clean out "xcodebuild" specifically... it would
be great if that directory could be created within SKIA_OUT.

Maybe change it to this, which will yield the same results but hopefully be more
enlightening?

    rm -rf $(SKIA_OUT)
    # TODO: During a transition period, clean out the old hardcoded "out/"
    # directory.
    # Once SKIA_OUT has been in place for a while, delete this.
    rm -rf out
    # TODO: Our Xcode build writes into a local "xcodebuild/" directory.
    # Until we can fix that, make sure it gets cleaned up...
    rm -rf xcodebuild

https://codereview.appspot.com/6782095/diff/1/gyp_skia
File gyp_skia (right):

https://codereview.appspot.com/6782095/diff/1/gyp_skia#newcode66
gyp_skia:66: not 'make' in os.getenv('GYP_GENERATORS')))):
On 2012/11/20 19:41:36, EricB wrote:
> Suggest rewriting this expression to read:
> 
> if os.name != 'posix' or \
>    (sys.platform.startswith('darwin') and \
>     (not os.getenv('GYP_GENERATORS') or \
>      'make' not in os.getenv('GYP_GENERATORS'))):
> 
> I think the \'s are necessary to keep python from complaining.

Python doesn't need the backslashes, because the open-parenthesis tells it to
keep going to the next line.

https://codereview.appspot.com/6782095/diff/3002/Makefile
File Makefile (right):

https://codereview.appspot.com/6782095/diff/3002/Makefile#newcode4
Makefile:4: # Some usage examples (tested on both Linux and Mac):
Maybe add a usage example using SKIA_OUT?  (Or just change one of the examples
below to use a value other than "out".)

https://codereview.appspot.com/6782095/diff/3002/Makefile#newcode58
Makefile:58: # recursively invoked Makefile within out/ _is_ allowed to run in
parallel
out -> $SKIA_OUT

https://codereview.appspot.com/6782095/diff/3002/Makefile#newcode119
Makefile:119: rm -f out/$(BUILDTYPE) || if test -d out/$(BUILDTYPE); then echo
"run 'make clean' or otherwise delete out/$(BUILDTYPE)"; exit 1; fi
I think it would be better to use $SKIA_OUT here (so we can get rid of the last
hardcoded references to "out/"), and fail-fast if SKIA_OUT is defined as
something other than "out/" in this case.

Hopefully we can remove this limitation before too long.

https://codereview.appspot.com/6782095/diff/3002/gyp_skia
File gyp_skia (right):

https://codereview.appspot.com/6782095/diff/3002/gyp_skia#newcode58
gyp_skia:58: # SKIA_OUT can be any directory either as a child of the standard
out/
But we delete "out/" as part of "make clean", so if any of your builds are
within "out/" they will be deleted by any call to "make clean"!

I don't think that's bad enough to warrant a change in behavior, but it might be
confusing.