Split off 'Google' category into separate ADMX/ADML files

Split off 'Google' category into separate ADMX/ADML files

Allows sharing of the category. The category shows up as tree item
in the GPO editor. If multiple ADMX files use a 'Google' category,
there is one 'Google' tree item for each ADMX file. This CL allows
the category to be shared. ADMX files can reference the 'Google'
category by pointing into the google.admx file.

BUG=chromium:665400
TEST=Tested policy_templates and pack_policy_templates, both branded and unbranded, made sure ADMX loads in editor.

Committed: https://crrev.com/76942473295e92c2f34a6ed40a47a806dec38072
Cr-Commit-Position: refs/heads/master@{#433597}

[ljusten (tachyonic), 2016-11-17 11:23:21]

ljusten@chromium.org changed reviewers:
+ flackr@chromium.org, thakis@chromium.org

[ljusten (tachyonic), 2016-11-17 11:23:22]

flackr@chromium.org: Please review changes in components.

thakis@chromium.org: Please review changes in tools.

[ljusten (tachyonic), 2016-11-17 11:24:49]

ljusten@chromium.org changed reviewers:
+ pastarmovj@chromium.org
- flackr@chromium.org

[ljusten (tachyonic), 2016-11-17 11:24:50]

Oops, wrong reviewer.

thakis@: Please review changes in tools.
pastarmovj: Please review changes in components.

[ljusten (tachyonic), 2016-11-17 11:25:28]

The CQ bit was checked by ljusten@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 11:25:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 12:15:26]

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

[commit-bot: I haz the power, 2016-11-17 12:15:27]

Dry run: This issue passed the CQ dry run.

[pastarmovj, 2016-11-17 13:30:20]

lgtm

[Nico, 2016-11-18 17:27:17]

We don't really have anyone left who really groks grit, as far as I know. The
grit change in here looks hacky to me, but I don't know grit well.

The build bits look incomplete to me.

https://codereview.chromium.org/2509243003/diff/20001/tools/grit/grit/format/...
File tools/grit/grit/format/policy_templates/writers/admx_writer.py (right):

https://codereview.chromium.org/2509243003/diff/20001/tools/grit/grit/format/...
tools/grit/grit/format/policy_templates/writers/admx_writer.py:72: if
self.config['build'] == 'chrome':
Is there precedent for grit checking for build vs chrome? grit is supposed to be
fairly standalone from what I understand; this feels wrong to me.

https://codereview.chromium.org/2509243003/diff/40001/components/policy/BUILD.gn
File components/policy/BUILD.gn (right):

https://codereview.chromium.org/2509243003/diff/40001/components/policy/BUILD...
components/policy/BUILD.gn:126: rebase_path(policy_templates_base_dir,
root_build_dir),
Are the files in this directory used as input? If so, if you don't list them in
inputs, the action won't re-run if the inputs change. (Look at gn's depfile
thingy for how to make the tool write a depfile if you can't pass all the inputs
on the command line)

https://codereview.chromium.org/2509243003/diff/40001/components/policy/BUILD...
components/policy/BUILD.gn:130: rebase_path(policy_templates_grd_file,
root_build_dir),
this should probably be listed as an input then?

https://codereview.chromium.org/2509243003/diff/40001/components/policy/tools...
File components/policy/tools/create_google_admx.py (right):

https://codereview.chromium.org/2509243003/diff/40001/components/policy/tools...
components/policy/tools/create_google_admx.py:68: file_list =
grd_helper.get_grd_outputs(options)
since all those are inputs too they too need to go in the depfile

[ljusten (tachyonic), 2016-11-19 17:12:35]

Nico, PTAL. You were right about the missing inputs. There's precedence for
accessing the build type in admx code, although I agree it's not very clean.

https://codereview.chromium.org/2509243003/diff/20001/tools/grit/grit/format/...
File tools/grit/grit/format/policy_templates/writers/admx_writer.py (right):

https://codereview.chromium.org/2509243003/diff/20001/tools/grit/grit/format/...
tools/grit/grit/format/policy_templates/writers/admx_writer.py:72: if
self.config['build'] == 'chrome':
On 2016/11/18 17:27:16, Nico wrote:
> Is there precedent for grit checking for build vs chrome? grit is supposed to
be
> fairly standalone from what I understand; this feels wrong to me.

There is precedence in adml_writer (_AddBaseStrings), where we assign different
translations based on the build type. We also query the build and the Chromium
version for a comment. Moreover, writer_configuration.py (part of grit) is full
of Chrome-specific definitions, so I guess at this point it's not really
standalone.

https://codereview.chromium.org/2509243003/diff/40001/components/policy/BUILD.gn
File components/policy/BUILD.gn (right):

https://codereview.chromium.org/2509243003/diff/40001/components/policy/BUILD...
components/policy/BUILD.gn:126: rebase_path(policy_templates_base_dir,
root_build_dir),
On 2016/11/18 17:27:16, Nico wrote:
> Are the files in this directory used as input? If so, if you don't list them
in
> inputs, the action won't re-run if the inputs change. (Look at gn's depfile
> thingy for how to make the tool write a depfile if you can't pass all the
inputs
> on the command line)

It defines the output path. Thanks for the tip with the depfile, that's good to
know!

https://codereview.chromium.org/2509243003/diff/40001/components/policy/BUILD...
components/policy/BUILD.gn:130: rebase_path(policy_templates_grd_file,
root_build_dir),
On 2016/11/18 17:27:16, Nico wrote:
> this should probably be listed as an input then?

Yes, you're right! Thanks.

https://codereview.chromium.org/2509243003/diff/40001/components/policy/tools...
File components/policy/tools/create_google_admx.py (right):

https://codereview.chromium.org/2509243003/diff/40001/components/policy/tools...
components/policy/tools/create_google_admx.py:68: file_list =
grd_helper.get_grd_outputs(options)
On 2016/11/18 17:27:16, Nico wrote:
> since all those are inputs too they too need to go in the depfile

This should be fine. The files are defined in the grd file, which I'll list as
input file. Thus, file_list cannot change unless the grd file changes, which
will retrigger this script.

But I just noticed that pack_policy_templates also lists them as input, so I'll
do the same.

[Nico, 2016-11-19 17:50:57]

Thanks, good replies. One more question:

https://codereview.chromium.org/2509243003/diff/40001/components/policy/tools...
File components/policy/tools/create_google_admx.py (right):

https://codereview.chromium.org/2509243003/diff/40001/components/policy/tools...
components/policy/tools/create_google_admx.py:68: file_list =
grd_helper.get_grd_outputs(options)
On 2016/11/19 17:12:35, ljusten wrote:
> On 2016/11/18 17:27:16, Nico wrote:
> > since all those are inputs too they too need to go in the depfile
> 
> This should be fine. The files are defined in the grd file, which I'll list as
> input file. Thus, file_list cannot change unless the grd file changes, which
> will retrigger this script.

A grd change will trigger a rebuild, but if the grd file doesn't change but the
grd file gets reprocessed (due to its inputs changing) and hence the grd outputs
change, you want the build to run your script afterwards. I think this part is
still Missing from the build graph atm (?)

> 
> But I just noticed that pack_policy_templates also lists them as input, so
I'll
> do the same.

[ljusten (tachyonic), 2016-11-19 23:41:23]

https://codereview.chromium.org/2509243003/diff/40001/components/policy/tools...
File components/policy/tools/create_google_admx.py (right):

https://codereview.chromium.org/2509243003/diff/40001/components/policy/tools...
components/policy/tools/create_google_admx.py:68: file_list =
grd_helper.get_grd_outputs(options)
On 2016/11/19 17:50:57, Nico wrote:
> On 2016/11/19 17:12:35, ljusten wrote:
> > On 2016/11/18 17:27:16, Nico wrote:
> > > since all those are inputs too they too need to go in the depfile
> > 
> > This should be fine. The files are defined in the grd file, which I'll list
as
> > input file. Thus, file_list cannot change unless the grd file changes, which
> > will retrigger this script.
> 
> A grd change will trigger a rebuild, but if the grd file doesn't change but
the
> grd file gets reprocessed (due to its inputs changing) and hence the grd
outputs
> change, you want the build to run your script afterwards. I think this part is
> still Missing from the build graph atm (?)

What kind of inputs could change the output of the grd file? To my
understanding, the outputs are all listed in the grd file and there are no
inputs that could change the outputs (except maybe for grit defines, but those
shouldn't cause issues).

Also note that I added 'policy_templates_windows_outputs' to the inputs. These
*are* the outputs of the grd file. They're defined in the gni file (not the
grd), but there's some magic in the policy_templates grit target that makes sure
that the outputs defined in the gni file actually match the grd outputs. That
should be enough to guarantee that we don't miss any inputs.

[ljusten (tachyonic), 2016-11-21 13:58:16]

Cleaned up the build dependencies. Turned out policy_templates_windows_outputs
couldn't be an input of create_google_admx since policy_templates created that,
but policy_templates depended on create_google_admx (circular dependency). Now
it works properly.

Also moved the build-specific code to writer_configuration to address your
concern about accessing the build type in grit, so at least it's all in one
place now.

Tested both Chromium and Chrome templates on Win Server 2012.

[Nico, 2016-11-21 16:00:39]

lgtm as far as I understand :-) Thanks for explaining!

https://codereview.chromium.org/2509243003/diff/40001/components/policy/tools...
File components/policy/tools/create_google_admx.py (right):

https://codereview.chromium.org/2509243003/diff/40001/components/policy/tools...
components/policy/tools/create_google_admx.py:68: file_list =
grd_helper.get_grd_outputs(options)
On 2016/11/19 23:41:23, ljusten wrote:
> On 2016/11/19 17:50:57, Nico wrote:
> > On 2016/11/19 17:12:35, ljusten wrote:
> > > On 2016/11/18 17:27:16, Nico wrote:
> > > > since all those are inputs too they too need to go in the depfile
> > > 
> > > This should be fine. The files are defined in the grd file, which I'll
list
> as
> > > input file. Thus, file_list cannot change unless the grd file changes,
which
> > > will retrigger this script.
> > 
> > A grd change will trigger a rebuild, but if the grd file doesn't change but
> the
> > grd file gets reprocessed (due to its inputs changing) and hence the grd
> outputs
> > change, you want the build to run your script afterwards. I think this part
is
> > still Missing from the build graph atm (?)
> 
> What kind of inputs could change the output of the grd file? To my
> understanding, the outputs are all listed in the grd file and there are no
> inputs that could change the outputs (except maybe for grit defines, but those
> shouldn't cause issues).
> 
> Also note that I added 'policy_templates_windows_outputs' to the inputs. These
> *are* the outputs of the grd file. They're defined in the gni file (not the
> grd), but there's some magic in the policy_templates grit target that makes
sure
> that the outputs defined in the gni file actually match the grd outputs. That
> should be enough to guarantee that we don't miss any inputs.

So only the list of output filenames is important, not the contents of those
files? I meant something could write an output file with the same name again.

[ljusten (tachyonic), 2016-11-21 17:24:38]

On 2016/11/21 16:00:39, Nico wrote:
> lgtm as far as I understand :-) Thanks for explaining!
> 
>
https://codereview.chromium.org/2509243003/diff/40001/components/policy/tools...
> File components/policy/tools/create_google_admx.py (right):
> 
>
https://codereview.chromium.org/2509243003/diff/40001/components/policy/tools...
> components/policy/tools/create_google_admx.py:68: file_list =
> grd_helper.get_grd_outputs(options)
> On 2016/11/19 23:41:23, ljusten wrote:
> > On 2016/11/19 17:50:57, Nico wrote:
> > > On 2016/11/19 17:12:35, ljusten wrote:
> > > > On 2016/11/18 17:27:16, Nico wrote:
> > > > > since all those are inputs too they too need to go in the depfile
> > > > 
> > > > This should be fine. The files are defined in the grd file, which I'll
> list
> > as
> > > > input file. Thus, file_list cannot change unless the grd file changes,
> which
> > > > will retrigger this script.
> > > 
> > > A grd change will trigger a rebuild, but if the grd file doesn't change
but
> > the
> > > grd file gets reprocessed (due to its inputs changing) and hence the grd
> > outputs
> > > change, you want the build to run your script afterwards. I think this
part
> is
> > > still Missing from the build graph atm (?)
> > 
> > What kind of inputs could change the output of the grd file? To my
> > understanding, the outputs are all listed in the grd file and there are no
> > inputs that could change the outputs (except maybe for grit defines, but
those
> > shouldn't cause issues).
> > 
> > Also note that I added 'policy_templates_windows_outputs' to the inputs.
These
> > *are* the outputs of the grd file. They're defined in the gni file (not the
> > grd), but there's some magic in the policy_templates grit target that makes
> sure
> > that the outputs defined in the gni file actually match the grd outputs.
That
> > should be enough to guarantee that we don't miss any inputs.
> 
> So only the list of output filenames is important, not the contents of those
> files? I meant something could write an output file with the same name again.

Yes, only the list. The script just iterates the files and writes
path/google.adm[x|l]
for every path/something.adm[x|l].

[ljusten (tachyonic), 2016-11-21 17:24:54]

The CQ bit was checked by ljusten@chromium.org

[ljusten (tachyonic), 2016-11-21 17:24:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
pastarmovj@chromium.org
Link to the patchset: https://codereview.chromium.org/2509243003/#ps80001
(title: "Cleanup gn deps; move cat adm* strings to config")

[commit-bot: I haz the power, 2016-11-21 17:25:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 17:30:24]

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

[commit-bot: I haz the power, 2016-11-21 17:30:25]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[ljusten (tachyonic), 2016-11-21 17:40:35]

The CQ bit was checked by ljusten@chromium.org

[ljusten (tachyonic), 2016-11-21 17:40:35]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
pastarmovj@chromium.org
Link to the patchset: https://codereview.chromium.org/2509243003/#ps100001
(title: "Android build fix")

[commit-bot: I haz the power, 2016-11-21 17:40:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 19:05:28]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1479750035122930,
"parent_rev": "2e9cc2378da9fa784256ec135069904c4581fbc6", "commit_rev":
"bc955c03360fba2762dc362aaf985146af9c1607"}

[commit-bot: I haz the power, 2016-11-21 19:05:58]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-21 19:09:32]

Description was changed from

==========
Split off 'Google' category into separate ADMX/ADML files

Allows sharing of the category. The category shows up as tree item
in the GPO editor. If multiple ADMX files use a 'Google' category,
there is one 'Google' tree item for each ADMX file. This CL allows
the category to be shared. ADMX files can reference the 'Google'
category by pointing into the google.admx file.

BUG=chromium:665400
TEST=Tested policy_templates and pack_policy_templates, both branded and
unbranded, made sure ADMX loads in editor.
==========

to

==========
Split off 'Google' category into separate ADMX/ADML files

Allows sharing of the category. The category shows up as tree item
in the GPO editor. If multiple ADMX files use a 'Google' category,
there is one 'Google' tree item for each ADMX file. This CL allows
the category to be shared. ADMX files can reference the 'Google'
category by pointing into the google.admx file.

BUG=chromium:665400
TEST=Tested policy_templates and pack_policy_templates, both branded and
unbranded, made sure ADMX loads in editor.

Committed: https://crrev.com/76942473295e92c2f34a6ed40a47a806dec38072
Cr-Commit-Position: refs/heads/master@{#433597}
==========

[commit-bot: I haz the power, 2016-11-21 19:09:34]

Patchset 6 (id:??) landed as
https://crrev.com/76942473295e92c2f34a6ed40a47a806dec38072
Cr-Commit-Position: refs/heads/master@{#433597}