|
|
DescriptionAdd policy_templates target for GN.
BUG=462362
Patch Set 1 #
Total comments: 2
Patch Set 2 : Scope outputs by {policy_templates} as Gn variables are global. #
Total comments: 2
Patch Set 3 : s/templates_dir/policy_templates_base_dir #Patch Set 4 : Mac fix #Messages
Total messages: 36 (14 generated)
knn@chromium.org changed reviewers: + bauerb@chromium.org, pneubeck@chromium.org
Please Review.
On 2015/02/27 10:45:45, knn wrote: > Please Review. Hi Philipp, Can you review my change today if you find time? Thanks, Khannan
https://codereview.chromium.org/957283003/diff/1/components/policy/BUILD.gn File components/policy/BUILD.gn (right): https://codereview.chromium.org/957283003/diff/1/components/policy/BUILD.gn#n... components/policy/BUILD.gn:124: output_dir = "$root_gen_dir/chrome" if you set this, shouldn't all outputs be relative to this dir?
TLDR: Should I use relative paths in policy_templates.gni? https://codereview.chromium.org/957283003/diff/1/components/policy/BUILD.gn File components/policy/BUILD.gn (right): https://codereview.chromium.org/957283003/diff/1/components/policy/BUILD.gn#n... components/policy/BUILD.gn:124: output_dir = "$root_gen_dir/chrome" On 2015/03/03 17:32:03, pneubeck wrote: > if you set this, shouldn't all outputs be relative to this dir? The output_dir variable is used only for relative paths. The path in the grd file is relative to the directory of grd file itself. Since the BUILD.gn file is in a different location, output_dir is necessary. As for paths in the policy_templates.gni file, they are absolute so there is no base directory. I can change the paths to relative if that is what you want?
thanks for the explanation. As discussed, I'm fine with both relative and absolute paths for the output file list. We should use whatever is more common in the code base for consistency. lgtm
On 2015/03/03 19:33:20, pneubeck wrote: > thanks for the explanation. > As discussed, I'm fine with both relative and absolute paths for the output file > list. We should use whatever is more common in the code base for consistency. > > lgtm FYI Philipp, scoping the names (ie prefixing the names with the target name) of outputs. This will improve readability in other places where these variables are used.
The CQ bit was checked by knn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pneubeck@chromium.org Link to the patchset: https://codereview.chromium.org/957283003/revert#ps20001 (title: "Scope outputs by {policy_templates} as Gn variables are global.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/957283003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) Timed out jobs are not retried to avoid causing additional load on the builders with large pending queues.
https://codereview.chromium.org/957283003/diff/20001/components/policy/resour... File components/policy/resources/policy_templates.gni (right): https://codereview.chromium.org/957283003/diff/20001/components/policy/resour... components/policy/resources/policy_templates.gni:8: templates_dir = "$root_gen_dir/chrome/app/policy" AFAIU, this variable is also exported by this file. Consider prefixing this as well to prevent name clashes (if that can be an issue in GN).
Done. https://codereview.chromium.org/957283003/diff/20001/components/policy/resour... File components/policy/resources/policy_templates.gni (right): https://codereview.chromium.org/957283003/diff/20001/components/policy/resour... components/policy/resources/policy_templates.gni:8: templates_dir = "$root_gen_dir/chrome/app/policy" On 2015/03/07 02:05:45, pneubeck wrote: > AFAIU, this variable is also exported by this file. > Consider prefixing this as well to prevent name clashes (if that can be an issue > in GN). Thanks for catching this!
The CQ bit was checked by knn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pneubeck@chromium.org Link to the patchset: https://codereview.chromium.org/957283003/#ps40001 (title: "s/templates_dir/policy_templates_base_dir")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/957283003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch.
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by knn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/957283003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch.
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by knn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/957283003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f24e2a468f0fffe65f626ba5d7c4c0089a78a65e Cr-Commit-Position: refs/heads/master@{#319616}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/994533003/ by treib@chromium.org. The reason for reverting is: Seems to have broken (at least) the Mac GN build: http://build.chromium.org/p/chromium.mac/builders/Mac%20GN http://build.chromium.org/p/chromium.mac/builders/Mac%20GN%20(dbg).
The CQ bit was checked by knn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pneubeck@chromium.org Link to the patchset: https://codereview.chromium.org/957283003/#ps60001 (title: "Mac fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/957283003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/982eb90bf74535cbe5e21c4eacb759c42ff424d4 Cr-Commit-Position: refs/heads/master@{#319759}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/992863002/ by dgrogan@chromium.org. The reason for reverting is: Possibly broke some GN builds (reverted with https://codereview.chromium.org/865573002), e.g.: http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/24483/s... http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2... ninja: Entering directory `/mnt/data/b/build/slave/Linux_GN/build/src/out/Release' ninja:error: expected depfile 'gen/chrome/policy_templates.d' to mention 'gen/chrome/app/policy/common/html/am/chrome_policy_list.html', got 'gen/chrome/app/policy/linux/examples/chrome.json' . |