Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1721883002:
gn/win: Don't strip trailing = on exec_script arguments. (Closed)
Descriptiongn/win: Don't strip trailing = on exec_script arguments.
BUG=588799
Committed: https://crrev.com/baf2794a6e1adbd276deecd16637d4829a358ed8
Cr-Commit-Position: refs/heads/master@{#376983}
Patch Set 1 #
Total comments: 5
Patch Set 2 : comments #Patch Set 3 : tweak comment #Messages
Total messages: 16 (7 generated)
thakis@chromium.org changed reviewers: + brettw@chromium.org
https://codereview.chromium.org/1721883002/diff/1/tools/gn/function_exec_scri... File tools/gn/function_exec_script.cc (right): https://codereview.chromium.org/1721883002/diff/1/tools/gn/function_exec_scri... tools/gn/function_exec_script.cc:178: cmdline.AppendArg("--"); I'm confused, won't this pass "--" to Python as well. Or is that OK? If we're doing that on purpose I'd mention it explicitly, since it's not "exactly whats in the .gn file". https://codereview.chromium.org/1721883002/diff/1/tools/gn/function_exec_scri... tools/gn/function_exec_script.cc:182: Looks like you got an extra line in here.
https://codereview.chromium.org/1721883002/diff/1/tools/gn/function_exec_scri... File tools/gn/function_exec_script.cc (right): https://codereview.chromium.org/1721883002/diff/1/tools/gn/function_exec_scri... tools/gn/function_exec_script.cc:178: cmdline.AppendArg("--"); On 2016/02/22 22:32:45, brettw wrote: > I'm confused, won't this pass "--" to Python as well. Or is that OK? If we're > doing that on purpose I'd mention it explicitly, since it's not "exactly whats > in the .gn file". base/command_kind.cc filters out the first -- parameter...it has a somewhat magic API here. https://codereview.chromium.org/1721883002/diff/1/tools/gn/function_exec_scri... tools/gn/function_exec_script.cc:182: On 2016/02/22 22:32:45, brettw wrote: > Looks like you got an extra line in here. Will remove
lgtm https://codereview.chromium.org/1721883002/diff/1/tools/gn/function_exec_scri... File tools/gn/function_exec_script.cc (right): https://codereview.chromium.org/1721883002/diff/1/tools/gn/function_exec_scri... tools/gn/function_exec_script.cc:178: cmdline.AppendArg("--"); On 2016/02/22 22:58:05, Nico wrote: > On 2016/02/22 22:32:45, brettw wrote: > > I'm confused, won't this pass "--" to Python as well. Or is that OK? If we're > > doing that on purpose I'd mention it explicitly, since it's not "exactly whats > > in the .gn file". > > base/command_kind.cc filters out the first -- parameter...it has a somewhat > magic API here. Weird, base::CommandLine is a disaster. Can you mention this in the comment.
The CQ bit was checked by thakis@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/1721883002/#ps20001 (title: "comments")
The CQ bit was unchecked by thakis@chromium.org
On 2016/02/22 23:00:35, brettw wrote: > lgtm > > https://codereview.chromium.org/1721883002/diff/1/tools/gn/function_exec_scri... > File tools/gn/function_exec_script.cc (right): > > https://codereview.chromium.org/1721883002/diff/1/tools/gn/function_exec_scri... > tools/gn/function_exec_script.cc:178: cmdline.AppendArg("--"); > On 2016/02/22 22:58:05, Nico wrote: > > On 2016/02/22 22:32:45, brettw wrote: > > > I'm confused, won't this pass "--" to Python as well. Or is that OK? If > we're > > > doing that on purpose I'd mention it explicitly, since it's not "exactly > whats > > > in the .gn file". > > > > base/command_kind.cc filters out the first -- parameter...it has a somewhat > > magic API here. > > Weird, base::CommandLine is a disaster. Can you mention this in the comment. Actually at least on OS X this seems to not be true. Need to check on Windows again tomorrow. (But the -- is passed to python, not the .py script, so it's at least harmless. In any case, unchecking cq for now.)
I guess I didn't test this carefully enough, on Windows it also includes the -- in the command like so: C:/python_27_amd64/files/python.exe -- C:/src/chrome/src/build/gypi_to_gn.py C:/src/chrome/src/net/net.gypi (CommandLine::GetArgs() does remove one --, but that isn't called here.) However, this does the right thing. -- tells python that everything following it isn't a parameter for python, and since we pass no parameters to python that's ok. So I think I'll update the comment a bit and hit cq again.
The CQ bit was checked by thakis@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/1721883002/#ps40001 (title: "tweak comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1721883002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1721883002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== gn/win: Don't strip trailing = on exec_script arguments. BUG=588799 ========== to ========== gn/win: Don't strip trailing = on exec_script arguments. BUG=588799 Committed: https://crrev.com/baf2794a6e1adbd276deecd16637d4829a358ed8 Cr-Commit-Position: refs/heads/master@{#376983} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/baf2794a6e1adbd276deecd16637d4829a358ed8 Cr-Commit-Position: refs/heads/master@{#376983} |
