Add an implementation path option to json_schema_compiler

Add an implementation path option to json_schema_compiler

This replaces the hard-coded chrome/browser/extensions/api path with
a command-line option (impl_dir).

BUG=349019
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=254995

[Ken Rockot(use gerrit already), 2014-03-04 17:19:02]

Please take a look.

[Yoyo Zhou, 2014-03-04 21:49:05]

LGTM
What does it mean for the targets with impl_dir = ''?

https://codereview.chromium.org/183763032/diff/1/build/json_schema_bundle_com...
File build/json_schema_bundle_compile.gypi (right):

https://codereview.chromium.org/183763032/diff/1/build/json_schema_bundle_com...
build/json_schema_bundle_compile.gypi:11: #   impl_dir: the root path of api
implementations
I'd call this the "default root path" because it can be overridden by individual
APIs.

[Ken Rockot(use gerrit already), 2014-03-04 22:20:42]

On 2014/03/04 21:49:05, Yoyo Zhou wrote:
> LGTM
> What does it mean for the targets with impl_dir = ''?

I guess what it really means is that the bundle generator will look for a
non-existent header at src/apiname/apiname.h, whereas the behavior before had it
looking for a non-existent header at
src/chrome/browser/extensions/api/apiname/apiname.h.

An empty string seemed like a reasonable choice, but I would consider two
alternatives:

1. Change these to 'chrome/browser/extensions/api' so that the behavior is
equivalent to before.
2. Change these to some invalid path string so that the expectation of
non-existence is explicit

[Ken Rockot(use gerrit already), 2014-03-04 22:20:49]

https://codereview.chromium.org/183763032/diff/1/build/json_schema_bundle_com...
File build/json_schema_bundle_compile.gypi (right):

https://codereview.chromium.org/183763032/diff/1/build/json_schema_bundle_com...
build/json_schema_bundle_compile.gypi:11: #   impl_dir: the root path of api
implementations
On 2014/03/04 21:49:06, Yoyo Zhou wrote:
> I'd call this the "default root path" because it can be overridden by
individual
> APIs.

Done.

[Yoyo Zhou, 2014-03-04 22:24:37]

On 2014/03/04 22:20:42, Ken Rockot wrote:
> On 2014/03/04 21:49:05, Yoyo Zhou wrote:
> > LGTM
> > What does it mean for the targets with impl_dir = ''?
> 
> I guess what it really means is that the bundle generator will look for a
> non-existent header at src/apiname/apiname.h, whereas the behavior before had
it
> looking for a non-existent header at
> src/chrome/browser/extensions/api/apiname/apiname.h.
> 
> An empty string seemed like a reasonable choice, but I would consider two
> alternatives:
> 
> 1. Change these to 'chrome/browser/extensions/api' so that the behavior is
> equivalent to before.
> 2. Change these to some invalid path string so that the expectation of
> non-existence is explicit

Another option is to make an empty string an explicit error, i.e. raise an error
in cpp_bundle_generator if you would be about to use an empty impl_dir.

[Ken Rockot(use gerrit already), 2014-03-04 22:32:19]

On 2014/03/04 22:24:37, Yoyo Zhou wrote:
> On 2014/03/04 22:20:42, Ken Rockot wrote:
> > On 2014/03/04 21:49:05, Yoyo Zhou wrote:
> > > LGTM
> > > What does it mean for the targets with impl_dir = ''?
> > 
> > I guess what it really means is that the bundle generator will look for a
> > non-existent header at src/apiname/apiname.h, whereas the behavior before
had
> it
> > looking for a non-existent header at
> > src/chrome/browser/extensions/api/apiname/apiname.h.
> > 
> > An empty string seemed like a reasonable choice, but I would consider two
> > alternatives:
> > 
> > 1. Change these to 'chrome/browser/extensions/api' so that the behavior is
> > equivalent to before.
> > 2. Change these to some invalid path string so that the expectation of
> > non-existence is explicit
> 
> Another option is to make an empty string an explicit error, i.e. raise an
error
> in cpp_bundle_generator if you would be about to use an empty impl_dir.

And what do targets specify for impl_dir when they don't care / don't have any
relevant path to speak of? They could omit it completely, but then is there a
clean gyp way of saying "Add an --impl-dir=<(impl_dir) arg if and only if
impl_dir is defined?"

[Yoyo Zhou, 2014-03-04 22:36:10]

On 2014/03/04 22:32:19, Ken Rockot wrote:
> On 2014/03/04 22:24:37, Yoyo Zhou wrote:
> > On 2014/03/04 22:20:42, Ken Rockot wrote:
> > > On 2014/03/04 21:49:05, Yoyo Zhou wrote:
> > > > LGTM
> > > > What does it mean for the targets with impl_dir = ''?
> > > 
> > > I guess what it really means is that the bundle generator will look for a
> > > non-existent header at src/apiname/apiname.h, whereas the behavior before
> had
> > it
> > > looking for a non-existent header at
> > > src/chrome/browser/extensions/api/apiname/apiname.h.
> > > 
> > > An empty string seemed like a reasonable choice, but I would consider two
> > > alternatives:
> > > 
> > > 1. Change these to 'chrome/browser/extensions/api' so that the behavior is
> > > equivalent to before.
> > > 2. Change these to some invalid path string so that the expectation of
> > > non-existence is explicit
> > 
> > Another option is to make an empty string an explicit error, i.e. raise an
> error
> > in cpp_bundle_generator if you would be about to use an empty impl_dir.
> 
> And what do targets specify for impl_dir when they don't care / don't have any
> relevant path to speak of? They could omit it completely, but then is there a
> clean gyp way of saying "Add an --impl-dir=<(impl_dir) arg if and only if
> impl_dir is defined?"

I don't know if GYP is that smart. I wonder if it's possible to define a default
impl_dir of '' and override that. (Because it'd be best for those who don't care
about impl_dir to not have to specify it.)

[Ken Rockot(use gerrit already), 2014-03-04 22:53:47]

> I don't know if GYP is that smart. I wonder if it's possible to define a
default
> impl_dir of '' and override that. (Because it'd be best for those who don't
care
> about impl_dir to not have to specify it.)

Yeah, there we go. This feels like a better approach.

The schema compiler gypis will now set impl_dir to chrome/browser/extensions/api
if wasn't already set.

Existing gyp files remain unchanged but the c/b/e/api path can be overridden by
setting impl_dir.

Of course nothing currently needs this override, but I've tested it manually.

[Ken Rockot(use gerrit already), 2014-03-05 00:11:37]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-03-05 00:14:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/183763032/40001

[commit-bot: I haz the power, 2014-03-05 01:24:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/183763032/40001

[commit-bot: I haz the power, 2014-03-05 06:09:12]

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

[commit-bot: I haz the power, 2014-03-05 06:09:12]

Retried try job too often on win for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[Ken Rockot(use gerrit already), 2014-03-05 07:08:15]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-03-05 07:09:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/183763032/40001

[commit-bot: I haz the power, 2014-03-05 10:11:24]

Change committed as 254995