Standardize grit invokes

Standardize grit invokes

- Add two gypi files to src/build for the boilerplate added to targets and actions
- Update grit_info to also support the -E flag so it can share more of the grit build commandline
- switch over all but the webkit and webkit glue grd files to use the common support


BUG=22247
TEST=everything still localized correctly
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=79548

[TVL, 2011-03-25 17:30:31]

I'm not sure if owners will make me get stamps from other folks, but I figure
you two are the right starting points for the guts of this.  I haven't done the
grd in third_party/WebKit or webkit/glue, but I believe this covers the rest of
them.  Looking at patch set 9, you'll see I got all green, those were all
clobber tries to confirm everything is using newly generated paks, headers, etc.

[Mark Mentovai, 2011-03-25 17:48:55]

I really like this idea.

http://codereview.chromium.org/6705030/diff/12001/app/app.gyp
File app/app.gyp (right):

http://codereview.chromium.org/6705030/diff/12001/app/app.gyp#newcode1
app/app.gyp:1: # Copyright (c) 2010 The Chromium Authors. All rights reserved.
I’d love to see some trybot results.

http://codereview.chromium.org/6705030/diff/12001/build/grit_action.gypi
File build/grit_action.gypi (right):

http://codereview.chromium.org/6705030/diff/12001/build/grit_action.gypi#newc...
build/grit_action.gypi:25: '<!@(<(grit_info_cmd) --inputs <(grit_grd_file))',
Does this list grit.py itself (and any in-tree Python libraries) too?

http://codereview.chromium.org/6705030/diff/12001/build/grit_target.gypi
File build/grit_target.gypi (right):

http://codereview.chromium.org/6705030/diff/12001/build/grit_target.gypi#newc...
build/grit_target.gypi:14: 'direct_dependent_settings': {
Really? I guess this means that you’re expecting all users of this to be
none-type targets and to have all of the real compiling done in dependents?

This is wrong if the action winds up living in a non-none-type target, like a
static_library.

http://codereview.chromium.org/6705030/diff/12001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

http://codereview.chromium.org/6705030/diff/12001/chrome/chrome.gyp#newcode157
chrome/chrome.gyp:157: # Grit action boiler plate.
These comments all over the place are getting redundant.

[tony, 2011-03-25 18:07:04]

http://codereview.chromium.org/6705030/diff/12001/build/grit_action.gypi
File build/grit_action.gypi (right):

http://codereview.chromium.org/6705030/diff/12001/build/grit_action.gypi#newc...
build/grit_action.gypi:1: # Copyright (c) 2011 The Chromium Authors. All rights
reserved.
Should these files be in tools/grit/ near grit_info.py?  This kind of feels like
a vsprops file, which we used to keep near the code it applied too.

http://codereview.chromium.org/6705030/diff/12001/build/grit_action.gypi#newc...
build/grit_action.gypi:25: '<!@(<(grit_info_cmd) --inputs <(grit_grd_file))',
On 2011/03/25 17:48:55, Mark Mentovai wrote:
> Does this list grit.py itself (and any in-tree Python libraries) too?

Yes, it should.

http://codereview.chromium.org/6705030/diff/12001/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.gypi (right):

http://codereview.chromium.org/6705030/diff/12001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.gypi:20: 'includes': [
'../../../build/grit_target.gypi' ],
Can we use <(DEPTH)?

http://codereview.chromium.org/6705030/diff/12001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

http://codereview.chromium.org/6705030/diff/12001/chrome/chrome.gyp#newcode53
chrome/chrome.gyp:53: # Clean this up when Windows moves to gyp.
I don't think this TODO applies to this code.  It applies to code that already
got moved to build/common.gypi.

http://codereview.chromium.org/6705030/diff/12001/chrome/chrome.gyp#newcode56
chrome/chrome.gyp:56: 'grit_defines': ['-E', 'CHROMIUM_BUILD=chromium'],
I would just move this into build/common.gypi (near where the TODO got moved
to).  I didn't do it in the last patch because I was considering renaming
grit_defines to grit_flags, but it doesn't seem like a big deal either way.

http://codereview.chromium.org/6705030/diff/12001/chrome/chrome.gyp#newcode153
chrome/chrome.gyp:153: 'action_name': 'autofile_resources',
Nit: autofile -> autofill

http://codereview.chromium.org/6705030/diff/12001/chrome/chrome.gyp#newcode326
chrome/chrome.gyp:326: # the real inputs/outputs at GYP time isn't possible.
Good job figuring this out :)
Sorry this target is weird and complicated :(

Is it possible to make this action depend on
third_party/WebKit/Source/WebKit/chromium/WebKit.gyp:generate_devtools_grd?

In a follow up, we can probably modify grit_info.py -inputs to take no .grd file
and just return a list of all the grit .py files so changes to grit will also
trigger a rebuild.

http://codereview.chromium.org/6705030/diff/12001/ui/base/strings/ui_strings.gyp
File ui/base/strings/ui_strings.gyp (right):

http://codereview.chromium.org/6705030/diff/12001/ui/base/strings/ui_strings....
ui/base/strings/ui_strings.gyp:19: 'includes': [
'../../../build/grit_action.gypi' ],
<(DEPTH)?

[Mark Mentovai, 2011-03-25 18:10:00]

Variable expansions don’t work in GYP includes.

[TVL, 2011-03-25 18:15:51]

http://codereview.chromium.org/6705030/diff/12001/app/app.gyp
File app/app.gyp (right):

http://codereview.chromium.org/6705030/diff/12001/app/app.gyp#newcode1
app/app.gyp:1: # Copyright (c) 2010 The Chromium Authors. All rights reserved.
On 2011/03/25 17:48:55, Mark Mentovai wrote:
> I’d love to see some trybot results.

From review request:

Looking at patch set 9, you'll see I got all green, those were all
clobber tries to confirm everything is using newly generated paks, headers, etc.

:)

This set is just a sync to trunk since atleast one of these files seems to
change every hour...

http://codereview.chromium.org/6705030/diff/12001/build/grit_action.gypi
File build/grit_action.gypi (right):

http://codereview.chromium.org/6705030/diff/12001/build/grit_action.gypi#newc...
build/grit_action.gypi:1: # Copyright (c) 2011 The Chromium Authors. All rights
reserved.
On 2011/03/25 18:07:04, tony wrote:
> Should these files be in tools/grit/ near grit_info.py?  This kind of feels
like
> a vsprops file, which we used to keep near the code it applied too.

That was my first thought also, but they actually depend on the flags in
common.gypi, so it seemed like it might be a little better to hang them here.

Mark - Any thoughts?

http://codereview.chromium.org/6705030/diff/12001/build/grit_target.gypi
File build/grit_target.gypi (right):

http://codereview.chromium.org/6705030/diff/12001/build/grit_target.gypi#newc...
build/grit_target.gypi:14: 'direct_dependent_settings': {
On 2011/03/25 17:48:55, Mark Mentovai wrote:
> Really? I guess this means that you’re expecting all users of this to be
> none-type targets and to have all of the real compiling done in dependents?
> 
> This is wrong if the action winds up living in a non-none-type target, like a
> static_library.

Yea, I'm torn on this one.  It was in every place we've used this because we
always made a target for these and didn't directly hang them on other targets. 
So in the effort to cut down common code I did this.

http://codereview.chromium.org/6705030/diff/12001/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.gypi (right):

http://codereview.chromium.org/6705030/diff/12001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.gypi:20: 'includes': [
'../../../build/grit_target.gypi' ],
On 2011/03/25 18:07:04, tony wrote:
> Can we use <(DEPTH)?

Nope, includes happen before variable expansion.

http://codereview.chromium.org/6705030/diff/12001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

http://codereview.chromium.org/6705030/diff/12001/chrome/chrome.gyp#newcode53
chrome/chrome.gyp:53: # Clean this up when Windows moves to gyp.
On 2011/03/25 18:07:04, tony wrote:
> I don't think this TODO applies to this code.  It applies to code that already
> got moved to build/common.gypi.

oops, right.

http://codereview.chromium.org/6705030/diff/12001/chrome/chrome.gyp#newcode56
chrome/chrome.gyp:56: 'grit_defines': ['-E', 'CHROMIUM_BUILD=chromium'],
On 2011/03/25 18:07:04, tony wrote:
> I would just move this into build/common.gypi (near where the TODO got moved
> to).  I didn't do it in the last patch because I was considering renaming
> grit_defines to grit_flags, but it doesn't seem like a big deal either way.

ok

http://codereview.chromium.org/6705030/diff/12001/chrome/chrome.gyp#newcode153
chrome/chrome.gyp:153: 'action_name': 'autofile_resources',
On 2011/03/25 18:07:04, tony wrote:
> Nit: autofile -> autofill

oops

http://codereview.chromium.org/6705030/diff/12001/chrome/chrome.gyp#newcode157
chrome/chrome.gyp:157: # Grit action boiler plate.
On 2011/03/25 17:48:55, Mark Mentovai wrote:
> These comments all over the place are getting redundant.

Yea, when you review it like this they are, but I was figuring if someone opens
one of these at random, it might make sense to still have the comments.  i'll
drop them if that's the better idea.

http://codereview.chromium.org/6705030/diff/12001/chrome/chrome.gyp#newcode326
chrome/chrome.gyp:326: # the real inputs/outputs at GYP time isn't possible.
On 2011/03/25 18:07:04, tony wrote:
> Good job figuring this out :)
> Sorry this target is weird and complicated :(
> 
> Is it possible to make this action depend on
> third_party/WebKit/Source/WebKit/chromium/WebKit.gyp:generate_devtools_grd?

Actions can't depend on anything, but the target depends on it, line 284
> 
> In a follow up, we can probably modify grit_info.py -inputs to take no .grd
file
> and just return a list of all the grit .py files so changes to grit will also
> trigger a rebuild.

[Mark Mentovai, 2011-03-25 18:22:07]

http://codereview.chromium.org/6705030/diff/12001/build/grit_action.gypi
File build/grit_action.gypi (right):

http://codereview.chromium.org/6705030/diff/12001/build/grit_action.gypi#newc...
build/grit_action.gypi:1: # Copyright (c) 2011 The Chromium Authors. All rights
reserved.
TVL wrote:
> On 2011/03/25 18:07:04, tony wrote:
> > Should these files be in tools/grit/ near grit_info.py?  This kind of feels
> like
> > a vsprops file, which we used to keep near the code it applied too.
> 
> That was my first thought also, but they actually depend on the flags in
> common.gypi, so it seemed like it might be a little better to hang them here.
> 
> Mark - Any thoughts?

I’m fine with either location. I can see arguments for placing it in both spots.
I don’t have a preference between the two.

http://codereview.chromium.org/6705030/diff/12001/build/grit_action.gypi#newc...
build/grit_action.gypi:10: #
Remove this line.

http://codereview.chromium.org/6705030/diff/12001/build/grit_action.gypi#newc...
build/grit_action.gypi:14: # basis causes the problem.  GYP Rules could be
extended to do this, but
“causes the problem”—which problem, you haven’t introduced a problem yet. Causes
a problem?

http://codereview.chromium.org/6705030/diff/12001/build/grit_action.gypi#newc...
build/grit_action.gypi:14: # basis causes the problem.  GYP Rules could be
extended to do this, but
Rules use a lowercase R.

http://codereview.chromium.org/6705030/diff/12001/build/grit_action.gypi#newc...
build/grit_action.gypi:15: # then every generator would have to be able to do
it, because that is when
but then every generator would need to be updated to handle this.

http://codereview.chromium.org/6705030/diff/12001/build/grit_target.gypi
File build/grit_target.gypi (right):

http://codereview.chromium.org/6705030/diff/12001/build/grit_target.gypi#newc...
build/grit_target.gypi:5: # This file is meant to be included into an target
that will be one or
an target -> a target

http://codereview.chromium.org/6705030/diff/12001/build/grit_target.gypi#newc...
build/grit_target.gypi:5: # This file is meant to be included into an target
that will be one or
will be is wrong, what did you mean?

http://codereview.chromium.org/6705030/diff/12001/build/grit_target.gypi#newc...
build/grit_target.gypi:9: #
Remove this line.

http://codereview.chromium.org/6705030/diff/12001/build/grit_target.gypi#newc...
build/grit_target.gypi:14: 'direct_dependent_settings': {
TVL wrote:
> On 2011/03/25 17:48:55, Mark Mentovai wrote:
> > Really? I guess this means that you’re expecting all users of this to be
> > none-type targets and to have all of the real compiling done in dependents?
> > 
> > This is wrong if the action winds up living in a non-none-type target, like
a
> > static_library.
> 
> Yea, I'm torn on this one.  It was in every place we've used this because we
> always made a target for these and didn't directly hang them on other targets.

> So in the effort to cut down common code I did this.

Have you considered putting this inside a target_conditions? If _type is none,
do what you’ve got here. Otherwise, bring include_dirs directly into the target,
no direct_dependent_settings. Then we can start transitioning these to happen
directly in targets where that makes sense.

http://codereview.chromium.org/6705030/diff/12001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

http://codereview.chromium.org/6705030/diff/12001/chrome/chrome.gyp#newcode157
chrome/chrome.gyp:157: # Grit action boiler plate.
TVL wrote:
> On 2011/03/25 17:48:55, Mark Mentovai wrote:
> > These comments all over the place are getting redundant.
> 
> Yea, when you review it like this they are, but I was figuring if someone
opens
> one of these at random, it might make sense to still have the comments.  i'll
> drop them if that's the better idea.

The comments don’t really say anything more than the includes do, except for the
word “boilerplate,” which incidentally shouldn’t have a space in it.

I don’t think the word “boilerplate” is significant enough to warrant the
comment. I don’t even think it’s really the best word for the job.

[TVL, 2011-03-25 18:33:30]

http://codereview.chromium.org/6705030/diff/12001/build/grit_action.gypi
File build/grit_action.gypi (right):

http://codereview.chromium.org/6705030/diff/12001/build/grit_action.gypi#newc...
build/grit_action.gypi:10: #
On 2011/03/25 18:22:07, Mark Mentovai wrote:
> Remove this line.

Done.

http://codereview.chromium.org/6705030/diff/12001/build/grit_action.gypi#newc...
build/grit_action.gypi:14: # basis causes the problem.  GYP Rules could be
extended to do this, but
On 2011/03/25 18:22:07, Mark Mentovai wrote:
> “causes the problem”—which problem, you haven’t introduced a problem yet.
Causes
> a problem?

Done.

http://codereview.chromium.org/6705030/diff/12001/build/grit_action.gypi#newc...
build/grit_action.gypi:15: # then every generator would have to be able to do
it, because that is when
On 2011/03/25 18:22:07, Mark Mentovai wrote:
> but then every generator would need to be updated to handle this.

Done.

http://codereview.chromium.org/6705030/diff/12001/build/grit_target.gypi
File build/grit_target.gypi (right):

http://codereview.chromium.org/6705030/diff/12001/build/grit_target.gypi#newc...
build/grit_target.gypi:5: # This file is meant to be included into an target
that will be one or
On 2011/03/25 18:22:07, Mark Mentovai wrote:
> an target -> a target

Done.

http://codereview.chromium.org/6705030/diff/12001/build/grit_target.gypi#newc...
build/grit_target.gypi:5: # This file is meant to be included into an target
that will be one or
On 2011/03/25 18:22:07, Mark Mentovai wrote:
> will be is wrong, what did you mean?

Done.

http://codereview.chromium.org/6705030/diff/12001/build/grit_target.gypi#newc...
build/grit_target.gypi:9: #
On 2011/03/25 18:22:07, Mark Mentovai wrote:
> Remove this line.

Done.

http://codereview.chromium.org/6705030/diff/12001/build/grit_target.gypi#newc...
build/grit_target.gypi:14: 'direct_dependent_settings': {
On 2011/03/25 18:22:07, Mark Mentovai wrote:
> TVL wrote:
> > On 2011/03/25 17:48:55, Mark Mentovai wrote:
> > > Really? I guess this means that you’re expecting all users of this to be
> > > none-type targets and to have all of the real compiling done in
dependents?
> > > 
> > > This is wrong if the action winds up living in a non-none-type target,
like
> a
> > > static_library.
> > 
> > Yea, I'm torn on this one.  It was in every place we've used this because we
> > always made a target for these and didn't directly hang them on other
targets.
> 
> > So in the effort to cut down common code I did this.
> 
> Have you considered putting this inside a target_conditions? If _type is none,
> do what you’ve got here. Otherwise, bring include_dirs directly into the
target,
> no direct_dependent_settings. Then we can start transitioning these to happen
> directly in targets where that makes sense.

makes sense, i'll take a shot at that next.

http://codereview.chromium.org/6705030/diff/12001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

http://codereview.chromium.org/6705030/diff/12001/chrome/chrome.gyp#newcode157
chrome/chrome.gyp:157: # Grit action boiler plate.
On 2011/03/25 18:22:07, Mark Mentovai wrote:
> TVL wrote:
> > On 2011/03/25 17:48:55, Mark Mentovai wrote:
> > > These comments all over the place are getting redundant.
> > 
> > Yea, when you review it like this they are, but I was figuring if someone
> opens
> > one of these at random, it might make sense to still have the comments. 
i'll
> > drop them if that's the better idea.
> 
> The comments don’t really say anything more than the includes do, except for
the
> word “boilerplate,” which incidentally shouldn’t have a space in it.
> 
> I don’t think the word “boilerplate” is significant enough to warrant the
> comment. I don’t even think it’s really the best word for the job.

ok, going away.

[TVL, 2011-03-25 18:43:53]

fyi - patchset 11 is all comments except the target_conditions in
grit_target.gypi if you want to scan those (and a sync to trunk).

I'll try to get the target_condition working now.

[tony, 2011-03-25 18:45:34]

LGTM

http://codereview.chromium.org/6705030/diff/12001/build/grit_action.gypi
File build/grit_action.gypi (right):

http://codereview.chromium.org/6705030/diff/12001/build/grit_action.gypi#newc...
build/grit_action.gypi:1: # Copyright (c) 2011 The Chromium Authors. All rights
reserved.
On 2011/03/25 18:22:07, Mark Mentovai wrote:
> TVL wrote:
> > On 2011/03/25 18:07:04, tony wrote:
> > > Should these files be in tools/grit/ near grit_info.py?  This kind of
feels
> > like
> > > a vsprops file, which we used to keep near the code it applied too.
> > 
> > That was my first thought also, but they actually depend on the flags in
> > common.gypi, so it seemed like it might be a little better to hang them
here.
> > 
> > Mark - Any thoughts?
> 
> I’m fine with either location. I can see arguments for placing it in both
spots.
> I don’t have a preference between the two.

My main concern was src/build/ getting too crowded, but this is OK for now.  We
can move the files later if we want.

http://codereview.chromium.org/6705030/diff/12001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

http://codereview.chromium.org/6705030/diff/12001/chrome/chrome.gyp#newcode326
chrome/chrome.gyp:326: # the real inputs/outputs at GYP time isn't possible.
On 2011/03/25 18:15:53, TVL wrote:
> On 2011/03/25 18:07:04, tony wrote:
> > Is it possible to make this action depend on
> > third_party/WebKit/Source/WebKit/chromium/WebKit.gyp:generate_devtools_grd?
> 
> Actions can't depend on anything, but the target depends on it, line 284

Ah, I see.  I guess that means any changes to generate_devtools_grd causes all
the actions to re-run, but I guess it was like that before.

[tony, 2011-03-25 18:47:09]

Oh, did you mean to include build/common.gypi?  I don't see the branded_env
variable anymore.

[TVL, 2011-03-25 18:48:43]

http://codereview.chromium.org/6705030/diff/12001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

http://codereview.chromium.org/6705030/diff/12001/chrome/chrome.gyp#newcode326
chrome/chrome.gyp:326: # the real inputs/outputs at GYP time isn't possible.
On 2011/03/25 18:45:34, tony wrote:
> On 2011/03/25 18:15:53, TVL wrote:
> > On 2011/03/25 18:07:04, tony wrote:
> > > Is it possible to make this action depend on
> > >
third_party/WebKit/Source/WebKit/chromium/WebKit.gyp:generate_devtools_grd?
> > 
> > Actions can't depend on anything, but the target depends on it, line 284
> 
> Ah, I see.  I guess that means any changes to generate_devtools_grd causes all
> the actions to re-run, but I guess it was like that before.

Not completely, the actions only run according to the inputs/outputs.  The
dependency only means the traget is built first.

[TVL, 2011-03-25 18:51:20]

On 2011/03/25 18:47:09, tony wrote:
> Oh, did you mean to include build/common.gypi?  I don't see the branded_env
> variable anymore.

missed it.  added in patchset 12.

[tony, 2011-03-25 19:01:20]

LGTM

[TVL, 2011-03-25 20:08:15]

ok, last upload done, this is a sync to trunk again.

i can't do the conditions for includes for direct_dependents/includes because
chrome.gyp:chrome_strings target uses a conditional to set the type of the
target, so type isn't defined when the condition is evaluated.  I'll see if i
can get the history on that hack and remove it and revisit the condition at a
later date.

just sent to all the trybots one last time.  i got a lot of flake before that
took a few tries to clear, so i might get some red on these tries, but given we
didn't really change anything since the last all green, i think this is ready to
go.  So final comments please so I can try to land this weekend or monday
morning.

thanks.

[Mark Mentovai, 2011-03-25 20:16:53]

LGTM. As discussed, it’s OK to fix up chrome_strings and put the condition
switching on type into grit_target.gypi in a future change.

http://codereview.chromium.org/6705030/diff/14002/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/6705030/diff/14002/build/common.gypi#newcode491
build/common.gypi:491: # Setup -D and -E flags passed into grit.
Set up as a verb is two words.

http://codereview.chromium.org/6705030/diff/14002/build/grit_action.gypi
File build/grit_action.gypi (right):

http://codereview.chromium.org/6705030/diff/14002/build/grit_action.gypi#newc...
build/grit_action.gypi:13: # basis which can't currently be done.  GYP rules
could be extended to do
I think you meant

“but it would need to determine inputs and outputs via grit_info on a per-file
basis. GYP rules don’t currently support that. They could be extended to do
this, but then…”

and drop the “, because that is when the rules are expanded/applied” thing. By
that point, the comment will already be 100% clear.