Ninja gyp_skia.

Ninja gyp_skia.

[bungeman, 2012-08-02 10:19:44]

This is a quick and dirty hack I've been using to build skia with ninja.
Basically use this instead of gyp_skia and then 'ninja -C out/Release' or 'ninja
-C out/Debug'. (Or of course you could just 'cd out/Release && ninja' just as
well.) The one issue with sharing with gyp_skia is that make needs
--generator-output and gyp doesn't support that, and gyp treats -Goutput
differently than make.

In any event there is probably a better way to do this, but I'm throwing this
one out there because it exists.

[bungeman, 2012-08-30 22:48:05]

Another idea that crossed my mind. May want to be able to specify which
generator to run and the ninja_output_dir'.

[epoger, 2012-09-04 16:19:13]

https://codereview.appspot.com/6455075/diff/3001/gyp_skia
File gyp_skia (right):

https://codereview.appspot.com/6455075/diff/3001/gyp_skia#newcode1
gyp_skia:1: #!/usr/bin/python
Briefly, what is the advantage of using ninja?  Is it available on all
platforms?  Should we use it by default?

https://codereview.appspot.com/6455075/diff/3001/gyp_skia#newcode28
gyp_skia:28: ninja_output_dir = 'out'
Is this the same 'out' directory as referred to by default_output_dir?

If so, why do we specify it as a relative path in one case, but an absolute path
in the other?

https://codereview.appspot.com/6455075/diff/3001/gyp_skia#newcode93
gyp_skia:93: # make, visual studio, xcode depending on platform.
"make, visual studio, xcode depending on platform." 

->

Gyp arguments for the default build system for each platform:
- "make" on Linux
- XCode on Mac
- Visual Studio on Windows

https://codereview.appspot.com/6455075/diff/3001/gyp_skia#newcode97
gyp_skia:97: default_args.extend(['--generator-output',
os.path.abspath(default_output_dir)])
Please group all the default_args settings in one stanza, and all the ninja_args
settings in a separate stanza.

[bungeman, 2012-09-04 18:23:37]

https://codereview.appspot.com/6455075/diff/3001/gyp_skia
File gyp_skia (right):

https://codereview.appspot.com/6455075/diff/3001/gyp_skia#newcode1
gyp_skia:1: #!/usr/bin/python
On 2012/09/04 16:19:14, epoger wrote:
> Briefly, what is the advantage of using ninja?  Is it available on all
> platforms?  Should we use it by default?

It is available and builds on all platforms. It is always command line and
produces minimal output, allowing for easy view of warnings and errors in the
terminal scroll-back. It's also really fast, which isn't that big a deal for
Skia, but it is nice.

The downside is that ninja is build only. As such it is a great replacement for
make on Linux. On Windows and Mac it's still very nice to have the vc++ and
xcode projects.

https://codereview.appspot.com/6455075/diff/3001/gyp_skia#newcode28
gyp_skia:28: ninja_output_dir = 'out'
On 2012/09/04 16:19:14, epoger wrote:
> Is this the same 'out' directory as referred to by default_output_dir?
> 
> If so, why do we specify it as a relative path in one case, but an absolute
path
> in the other?

I think we force make to the absolute path to make things saner? Ninja does not
like the absolute path; just using the absolute path here means nothing builds.
This may or may not be fixable with mucking about with depth maybe, but I'm not
sure how that would be done.

What this boils down to is a very fundamental difference between make and ninja
and how they are generated by gyp. The make generator needs a place to put all
of it's make files (--generator-output) and somewhere else to build
(-Goutput_dir=). The ninja generator just needs somewhere to build
(-Goutput_dir=) because there will be separate ninja files generated for each
configuration and the ninja files are considered to be part of the build and
must be where the build will take place.

The ninja generator will error if passed --generator-output. The make generator
sees -Goutput_dir= as where to put build output, but ninja also sees
-Goutput_dir= as the relative location to the build output from the current
directory when calculating paths to the sources from the build directory.

TL;DR: if you can figure out a better means of making this work, I'm all for
that.

https://codereview.appspot.com/6455075/diff/3001/gyp_skia#newcode93
gyp_skia:93: # make, visual studio, xcode depending on platform.
On 2012/09/04 16:19:14, epoger wrote:
> "make, visual studio, xcode depending on platform." 
> 
> ->
> 
> Gyp arguments for the default build system for each platform:
> - "make" on Linux
> - XCode on Mac
> - Visual Studio on Windows

Sure, that seems like a more direct comment.

https://codereview.appspot.com/6455075/diff/3001/gyp_skia#newcode97
gyp_skia:97: default_args.extend(['--generator-output',
os.path.abspath(default_output_dir)])
On 2012/09/04 16:19:14, epoger wrote:
> Please group all the default_args settings in one stanza, and all the
ninja_args
> settings in a separate stanza.

Yes, splitting them out completely is a good idea.

[thakis, 2012-09-08 01:35:13]

On 2012/09/04 18:23:37, bungeman wrote:
> https://codereview.appspot.com/6455075/diff/3001/gyp_skia
> File gyp_skia (right):
> 
> https://codereview.appspot.com/6455075/diff/3001/gyp_skia#newcode1
> gyp_skia:1: #!/usr/bin/python
> On 2012/09/04 16:19:14, epoger wrote:
> > Briefly, what is the advantage of using ninja?  Is it available on all
> > platforms?  Should we use it by default?
> 
> It is available and builds on all platforms. It is always command line and
> produces minimal output, allowing for easy view of warnings and errors in the
> terminal scroll-back. It's also really fast, which isn't that big a deal for
> Skia, but it is nice.
> 
> The downside is that ninja is build only. As such it is a great replacement
for
> make on Linux. On Windows and Mac it's still very nice to have the vc++ and
> xcode projects.
> 
> https://codereview.appspot.com/6455075/diff/3001/gyp_skia#newcode28
> gyp_skia:28: ninja_output_dir = 'out'
> On 2012/09/04 16:19:14, epoger wrote:
> > Is this the same 'out' directory as referred to by default_output_dir?
> > 
> > If so, why do we specify it as a relative path in one case, but an absolute
> path
> > in the other?
> 
> I think we force make to the absolute path to make things saner? Ninja does
not
> like the absolute path; just using the absolute path here means nothing
builds.
> This may or may not be fixable with mucking about with depth maybe, but I'm
not
> sure how that would be done.
> 
> What this boils down to is a very fundamental difference between make and
ninja
> and how they are generated by gyp. The make generator needs a place to put all
> of it's make files (--generator-output) and somewhere else to build
> (-Goutput_dir=). The ninja generator just needs somewhere to build
> (-Goutput_dir=) because there will be separate ninja files generated for each
> configuration and the ninja files are considered to be part of the build and
> must be where the build will take place.
> 
> The ninja generator will error if passed --generator-output. The make
generator
> sees -Goutput_dir= as where to put build output, but ninja also sees
> -Goutput_dir= as the relative location to the build output from the current
> directory when calculating paths to the sources from the build directory.
> 
> TL;DR: if you can figure out a better means of making this work, I'm all for
> that.

Here's what I wrote to epoger in private email when ninja on mac was new: """
More interestingly, if you remove the -Goutput_dir and
--generator-output parameters from gyp_skia, ninja works as well – on
both mac and linux:

./gyp_skia --format=ninja
ninja -C out/Debug tests

(A ninja binary is in depot_tools and hence probably already in your
path.) Ninja is a lot faster than make, and it has a much nicer UI too
(defaults to a sane -j, produces much less output). Since the ninja
generator always writes its generated "makefiles" to the output
directory, it doesn't support --generator-output (which controls where
generated makefiles go). I'll see if I can tweak it so that
--generator-output=path -Goutput_dir=. is transparently treated as
-Goutput_dir=path. It also looks like the generator has trouble with
absolute paths for -Goutput_dir, I'll see if I can fix that too. Once
that's done, ninja should work without any changes to gyp_skia on your
end.
"""

I haven't had time to look into this, but the above tweak to gyp's ninja
generator should be enough to let skia build without needing the change in this
CL.

> 
> https://codereview.appspot.com/6455075/diff/3001/gyp_skia#newcode93
> gyp_skia:93: # make, visual studio, xcode depending on platform.
> On 2012/09/04 16:19:14, epoger wrote:
> > "make, visual studio, xcode depending on platform." 
> > 
> > ->
> > 
> > Gyp arguments for the default build system for each platform:
> > - "make" on Linux
> > - XCode on Mac
> > - Visual Studio on Windows
> 
> Sure, that seems like a more direct comment.
> 
> https://codereview.appspot.com/6455075/diff/3001/gyp_skia#newcode97
> gyp_skia:97: default_args.extend(['--generator-output',
> os.path.abspath(default_output_dir)])
> On 2012/09/04 16:19:14, epoger wrote:
> > Please group all the default_args settings in one stanza, and all the
> ninja_args
> > settings in a separate stanza.
> 
> Yes, splitting them out completely is a good idea.

[tfarina1, 2012-09-08 01:43:04]

On 2012/09/08 01:35:13, thakis wrote:
> if you remove the -Goutput_dir and --generator-output parameters from
gyp_skia, ninja works as well
Indeed! Thanks a lot!

> – on both mac and linux:
> 
> ./gyp_skia --format=ninja
> ninja -C out/Debug tests
> 
works like a charm. ;)

[bungeman, 2012-09-08 01:57:19]

On 2012/09/08 01:35:13, thakis wrote:
> I'll see if I can tweak it so that
> --generator-output=path -Goutput_dir=. is transparently treated as
> -Goutput_dir=path. It also looks like the generator has trouble with
> absolute paths for -Goutput_dir, I'll see if I can fix that too. Once
> that's done, ninja should work without any changes to gyp_skia on your
> end.

This would be a most welcome set of changes.

I was also debating a change to gyp so that a special generator name 'default'
which would just turn into the default generator for the platform. This way it
would be possible to '-f default,ninja'. Currently this CL just runs gyp twice,
once to get the default and once to get ninja, but this means that everything
needs to be parsed twice which takes time. However, I'm somewhat dubious about
this since it seems that the selection of the default should be up to Skia's
gyp_skia script and not rely on gyp's auto selection. Ideally, it would be nice
to run multiple generators at a time, each with their own independent set of -G*
or --generator* parameters, but mostly to avoid the multiple parsing of gyps.

[bungeman, 2012-09-26 20:02:18]

So here is something I'd feel more comfortable checking in. Basically it parses
the GYP_GENERATORS environment variable the same way gyp would, sees if ninja is
in the list, and treats it specially if it is. This should be fairly transparent
to users, as opposed to previous proposals.

[tfarina1, 2012-09-26 21:14:26]

I've patched and tested the latest patch set.

It works for me.

Thanks for doing this.

PS: Should we file a bug to fix the following warning?

"ninja: warning: multiple rules generate all. build will not be correct;
continuing anyway"

[bungeman, 2012-09-26 21:21:25]

On 2012/09/26 21:14:26, tfarina1 wrote:
> PS: Should we file a bug to fix the following warning?
> 
> "ninja: warning: multiple rules generate all. build will not be correct;
> continuing anyway"

Probably a good idea. It hasn't bitten us yet, but it probably will some day
when there's something else going on. I think it's just an artifact of having
multiple disjoint top level targets which and all the 'all's should be the same.
On the other hand, I haven't taken a good look.

[tfarina1, 2012-09-26 21:26:50]

On Wed, Sep 26, 2012 at 6:21 PM,  <bungeman@google.com> wrote:
> On 2012/09/26 21:14:26, tfarina1 wrote:
>>
>> PS: Should we file a bug to fix the following warning?
>
>
>> "ninja: warning: multiple rules generate all. build will not be
>
> correct;
>>
>> continuing anyway"
>
>
> Probably a good idea. It hasn't bitten us yet, but it probably will some
> day when there's something else going on. I think it's just an artifact
> of having multiple disjoint top level targets which and all the 'all's
> should be the same. On the other hand, I haven't taken a good look.
>
Looks like it's because we have gyp/all.gyp and skia.gyp with both
defining a 'all' target. The former is deprecated.

-- 
Thiago

[bungeman, 2012-09-26 22:19:28]

> Looks like it's because we have gyp/all.gyp and skia.gyp with both
> defining a 'all' target. The former is deprecated.
> 
> -- 
> Thiago

I removed the gyp/all.gyp locally and still had the issue. Turns out the ninja
gyp generator is automatically creating an 'all' target. I'll see if this can be
suppressed. If I locally change the name of the 'all' target in skia.gyp to
'skia', then I still get an 'all' target which builds everything.

[tfarina1, 2012-09-26 22:29:02]

On Wed, Sep 26, 2012 at 7:19 PM,  <bungeman@google.com> wrote:
> I removed the gyp/all.gyp locally and still had the issue. Turns out the
> ninja gyp generator is automatically creating an 'all' target. I'll see
> if this can be suppressed. If I locally change the name of the 'all'
> target in skia.gyp to 'skia', then I still get an 'all' target which
> builds everything.
>
If you rename the target from 'all' to 'All' then the warning goes away.

-- 
Thiago

[thakis, 2012-09-27 02:18:08]

https://codereview.appspot.com/6455075/diff/12001/gyp_skia
File gyp_skia (right):

https://codereview.appspot.com/6455075/diff/12001/gyp_skia#newcode82
gyp_skia:82: default_args.extend(['-Goutput_dir=.'])
You don't think it's better to special-case |--generator-output=foo
-Goutput_dir=.| as an alias for |-Goutput_dir=foo| in gyp's ninja generator,
instead of doing this here?

[bungeman, 2012-09-27 03:13:32]

Some thoughts on non-special case ways to make a change in the ninja generator.

https://codereview.appspot.com/6455075/diff/12001/gyp_skia
File gyp_skia (right):

https://codereview.appspot.com/6455075/diff/12001/gyp_skia#newcode82
gyp_skia:82: default_args.extend(['-Goutput_dir=.'])
On 2012/09/27 02:18:09, thakis wrote:
> You don't think it's better to special-case |--generator-output=foo
> -Goutput_dir=.| as an alias for |-Goutput_dir=foo| in gyp's ninja generator,
> instead of doing this here?

I would prefer this to be

--build-output=buildDir
-Ggenerator_output=generatorDir

where buildDir is where the build should happen and generatorDir is where the
gyp generator places its generated build scripts, if this is supported and the
generator recognizes it (ignored otherwise). Relative paths for both are
resolved against pwd.

To avoid any magic special casing, the way to do this in ninja currently would
be to take pwd/${--generator-output}/${-Goutput_dir} instead of just erroring
out when --generator-output is present. If any of the expansions is an absolute
path it overwrites whatever came to the left. Maybe I'll write that up.

[thakis, 2012-09-27 03:50:59]

On 2012/09/27 03:13:32, bungeman wrote:
> Some thoughts on non-special case ways to make a change in the ninja
generator.
> 
> https://codereview.appspot.com/6455075/diff/12001/gyp_skia
> File gyp_skia (right):
> 
> https://codereview.appspot.com/6455075/diff/12001/gyp_skia#newcode82
> gyp_skia:82: default_args.extend(['-Goutput_dir=.'])
> On 2012/09/27 02:18:09, thakis wrote:
> > You don't think it's better to special-case |--generator-output=foo
> > -Goutput_dir=.| as an alias for |-Goutput_dir=foo| in gyp's ninja generator,
> > instead of doing this here?
> 
> I would prefer this to be
> 
> --build-output=buildDir
> -Ggenerator_output=generatorDir
> 
> where buildDir is where the build should happen and generatorDir is where the
> gyp generator places its generated build scripts, if this is supported and the
> generator recognizes it (ignored otherwise). Relative paths for both are
> resolved against pwd.
> 
> To avoid any magic special casing, the way to do this in ninja currently would
> be to take pwd/${--generator-output}/${-Goutput_dir} instead of just erroring
> out when --generator-output is present. If any of the expansions is an
absolute
> path it overwrites whatever came to the left. Maybe I'll write that up.

That's not the same though. For output_dir=. and generator-output=foo, then
output_dir=foo is the same thing in ninja.

If you want to change output_dir to be unrelated to generator_output in this
script, I agree that you need a special case here.

[bungeman, 2012-09-27 06:07:02]

Created https://codereview.appspot.com/6566059/ .

[bungeman, 2012-10-02 06:27:12]

Closing in favor of https://codereview.appspot.com/6566059/ .