Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2083903005:
Adds gn config for policy_templates.zip (Closed)
DescriptionAdds gn config for policy_templates.zip
BUG=621575
Committed: https://crrev.com/6211e7d67d57db54d8c5b9a56b9be6de99618ef4
Cr-Commit-Position: refs/heads/master@{#401497}
Patch Set 1 #Patch Set 2 #
Total comments: 11
Patch Set 3 : cr #
Total comments: 1
Patch Set 4 #Patch Set 5 : fix trybot failure #Messages
Total messages: 21 (10 generated)
Description was changed from ========== Adds gn config for policy_templates.zip BUG=621575 ========== to ========== Adds gn config for policy_templates.zip BUG=621575 ==========
zmin@chromium.org changed reviewers: + brettw@chromium.org, pastarmovj@chromium.org
Hi Julian, Brett, Please review the GN config for policy_template.zip Related GYP config file: https://cs.chromium.org/chromium/src/chrome/policy_templates.gypi?q=policy_te... Thanks, Owen Min
Thanks! https://codereview.chromium.org/2083903005/diff/20001/components/policy/BUILD.gn File components/policy/BUILD.gn (right): https://codereview.chromium.org/2083903005/diff/20001/components/policy/BUILD... components/policy/BUILD.gn:136: grit_grd_file = "resources/policy_templates.grd" This variable name seems too generic, maybe something like policy_templates_grd_file https://codereview.chromium.org/2083903005/diff/20001/components/policy/BUILD... components/policy/BUILD.gn:399: version_file = "VERSION" You should put these add_version and pack_templates targets in an is_win block since you only reference them on Windows. Otherwise they'll still show up in "all" on other platforms. https://codereview.chromium.org/2083903005/diff/20001/components/policy/BUILD... components/policy/BUILD.gn:400: version_path = "$policy_templates_base_dir/" + version_file This can be written: version_path = "$policy_templates_base_dir/$version_file" https://codereview.chromium.org/2083903005/diff/20001/components/policy/BUILD... components/policy/BUILD.gn:404: "../../chrome/VERSION", This would be clearer as "//chrome/VERSION" https://codereview.chromium.org/2083903005/diff/20001/components/policy/BUILD... components/policy/BUILD.gn:412: output_zip_file = "$root_out_dir/policy_templates.zip" _Don't_ change this line when doing the below root_out_dir change. This line is correct. https://codereview.chromium.org/2083903005/diff/20001/components/policy/BUILD... components/policy/BUILD.gn:424: rebase_path(output_zip_file, root_out_dir), All of these root_out_dir calls in args should be root_build_dir instead (they will be the same for the main toolchain but not for cross-compiles).
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2083903005/diff/20001/components/policy/BUILD.gn File components/policy/BUILD.gn (right): https://codereview.chromium.org/2083903005/diff/20001/components/policy/BUILD... components/policy/BUILD.gn:136: grit_grd_file = "resources/policy_templates.grd" On 2016/06/21 22:50:42, brettw wrote: > This variable name seems too generic, maybe something like > policy_templates_grd_file Done. https://codereview.chromium.org/2083903005/diff/20001/components/policy/BUILD... components/policy/BUILD.gn:399: version_file = "VERSION" On 2016/06/21 22:50:42, brettw wrote: > You should put these add_version and pack_templates targets in an is_win block > since you only reference them on Windows. Otherwise they'll still show up in > "all" on other platforms. Done. https://codereview.chromium.org/2083903005/diff/20001/components/policy/BUILD... components/policy/BUILD.gn:400: version_path = "$policy_templates_base_dir/" + version_file On 2016/06/21 22:50:42, brettw wrote: > This can be written: > version_path = "$policy_templates_base_dir/$version_file" Done. https://codereview.chromium.org/2083903005/diff/20001/components/policy/BUILD... components/policy/BUILD.gn:404: "../../chrome/VERSION", On 2016/06/21 22:50:42, brettw wrote: > This would be clearer as "//chrome/VERSION" Done. https://codereview.chromium.org/2083903005/diff/20001/components/policy/BUILD... components/policy/BUILD.gn:424: rebase_path(output_zip_file, root_out_dir), On 2016/06/21 22:50:42, brettw wrote: > All of these root_out_dir calls in args should be root_build_dir instead (they > will be the same for the main toolchain but not for cross-compiles). Done.
lgtm https://codereview.chromium.org/2083903005/diff/60001/components/policy/BUILD.gn File components/policy/BUILD.gn (right): https://codereview.chromium.org/2083903005/diff/60001/components/policy/BUILD... components/policy/BUILD.gn:444: group("pack_policy_templates") { Unless you're going to add more stuff, this group isn't doing anything for you. Just call the action above ":pack_policy_templates" and delete this.
On 2016/06/22 19:08:45, brettw wrote: > lgtm > > https://codereview.chromium.org/2083903005/diff/60001/components/policy/BUILD.gn > File components/policy/BUILD.gn (right): > > https://codereview.chromium.org/2083903005/diff/60001/components/policy/BUILD... > components/policy/BUILD.gn:444: group("pack_policy_templates") { > Unless you're going to add more stuff, this group isn't doing anything for you. > Just call the action above ":pack_policy_templates" and delete this. Done.
Lgtm. Thanks for taking care of this!
The CQ bit was checked by zmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org Link to the patchset: https://codereview.chromium.org/2083903005/#ps80001 (title: "")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2083903005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_clang on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by zmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org, pastarmovj@chromium.org Link to the patchset: https://codereview.chromium.org/2083903005/#ps100001 (title: "fix trybot failure")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2083903005/100001
Message was sent while issue was closed.
Description was changed from ========== Adds gn config for policy_templates.zip BUG=621575 ========== to ========== Adds gn config for policy_templates.zip BUG=621575 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Adds gn config for policy_templates.zip BUG=621575 ========== to ========== Adds gn config for policy_templates.zip BUG=621575 Committed: https://crrev.com/6211e7d67d57db54d8c5b9a56b9be6de99618ef4 Cr-Commit-Position: refs/heads/master@{#401497} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6211e7d67d57db54d8c5b9a56b9be6de99618ef4 Cr-Commit-Position: refs/heads/master@{#401497} |
