Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2512043002:
Move the GN exec_script whitelist for //build into //build. (Closed)
|
Created:
4 years, 1 month ago by Dirk Pranke Modified:
4 years, 1 month ago Reviewers:
jochen (gone - plz use gerrit), mtklein_C, Michael Achenbach, kjellander_chromium, ehmaldonado_chromium, brettw CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove the GN exec_script whitelist for //build into //build.
This change moves the list of files that are allowed to call
exec_script() into a variable in a file in the //build directory,
so that the list can be shared more easily by other repos that
are pulling in //build via DEPS but have their own custom dotfile.
R=brettw@chromium.org, machenbach@chromium.org
BUG=617873
Committed: https://crrev.com/1cfa53187039597e5cdaa302cab1921a57060dce
Cr-Commit-Position: refs/heads/master@{#433764}
Patch Set 1 #
Total comments: 5
Patch Set 2 : actually add the //build file #Patch Set 3 : tweak comment, remove skia file #Messages
Total messages: 23 (7 generated)
dpranke@chromium.org changed reviewers: + ehmaldonado@chromium.org, jochen@chromium.org, kjellander@chromium.org, mtklein@chromium.org
thoughts? https://codereview.chromium.org/2512043002/diff/1/.gn File .gn (right): https://codereview.chromium.org/2512043002/diff/1/.gn#newcode223 .gn:223: build_dot_gn.exec_script_whitelist + [ We could generalize this pattern and provide build_dot_gn values for buildconfig, check_targets, and secondary_source as well.
Ok by me on behalf of Skia. We have a custom .gn but don't pull //build (only //buildtools to get at gn.sha1 files). https://codereview.chromium.org/2512043002/diff/1/.gn File .gn (right): https://codereview.chromium.org/2512043002/diff/1/.gn#newcode243 .gn:243: "//third_party/skia/gn/shared_sources.gni", FWIW, this no longer execs any scripts. We can probably remove it from the whitelist whenever you like.
I defer to machenbach for v8
I like this approach! Many times we've had to change our .gn file due to whitelist changes in the //build tree. lgtm (given the //build/dot_gn.gni is landed in another CL first of course).
https://codereview.chromium.org/2512043002/diff/1/.gn File .gn (right): https://codereview.chromium.org/2512043002/diff/1/.gn#newcode220 .gn:220: import("//build/dot_gn.gni") I'd prefer this goes at the top. I actually don't think you need the comment. https://codereview.chromium.org/2512043002/diff/1/.gn#newcode222 .gn:222: exec_script_whitelist = I'd like a comment here instead saying that whitelist values for //build go in that file for sharing with other projects, and these are the non-build ones for the Chrome repo.
Did you forget to "git add" the .gni file?
On 2016/11/18 22:12:07, brettw (ping on IM after 24h) wrote: > Did you forget to "git add" the .gni file? Weird, I guess so. I could've sworn I added it.
Okay, I didn't even have a copy of the file in my local repo. Very strange. Anyway, I've recreated it (and changed the name). I am open to other suggestions for the name also.
https://codereview.chromium.org/2512043002/diff/1/.gn File .gn (right): https://codereview.chromium.org/2512043002/diff/1/.gn#newcode243 .gn:243: "//third_party/skia/gn/shared_sources.gni", On 2016/11/18 00:19:43, mtklein_C wrote: > FWIW, this no longer execs any scripts. We can probably remove it from the > whitelist whenever you like. Acknowledged.
LGTM, I like it!
lgtm
The CQ bit was checked by dpranke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@chromium.org Link to the patchset: https://codereview.chromium.org/2512043002/#ps40001 (title: "tweak comment, remove skia file")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dpranke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1479778556406110,
"parent_rev": "dc7cf744322462eb85ca6f83b2de4bca6be14579", "commit_rev":
"7811a564615fce1a594137c0ef932c7399d520a3"}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Move the GN exec_script whitelist for //build into //build. This change moves the list of files that are allowed to call exec_script() into a variable in a file in the //build directory, so that the list can be shared more easily by other repos that are pulling in //build via DEPS but have their own custom dotfile. R=brettw@chromium.org, machenbach@chromium.org BUG=617873 ========== to ========== Move the GN exec_script whitelist for //build into //build. This change moves the list of files that are allowed to call exec_script() into a variable in a file in the //build directory, so that the list can be shared more easily by other repos that are pulling in //build via DEPS but have their own custom dotfile. R=brettw@chromium.org, machenbach@chromium.org BUG=617873 Committed: https://crrev.com/1cfa53187039597e5cdaa302cab1921a57060dce Cr-Commit-Position: refs/heads/master@{#433764} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/1cfa53187039597e5cdaa302cab1921a57060dce Cr-Commit-Position: refs/heads/master@{#433764} |
