New issue
Advanced search Search tips

Issue 831613 link

Starred by 1 user

Issue metadata

Status: Untriaged
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

gn writes invalid ninja files if an action has a newline ("$0x0A") in its command

Project Member Reported by thakis@chromium.org, Apr 11 2018

Issue description

$ git diff
diff --git a/base/BUILD.gn b/base/BUILD.gn
index 463fa3e30aae..bf58e7a6e3e4 100644
--- a/base/BUILD.gn
+++ b/base/BUILD.gn
@@ -110,6 +110,12 @@ if (is_android) {
   }
 }
 
+action("foo") {
+  script = "//build/gn_helpers.py"
+  args = [ "-D", "values=a=0$0x0Ab=1" ]
+  outputs = [ "$root_out_dir/output" ]
+}
+
 # Base and everything it depends on should be a static library rather than
 # a source set. Base is more of a "library" in the classic sense in that many
 # small parts of it are used in many different contexts. This combined with a
Nicos-MacBook-Pro:src thakis$ gn gen out/gn
Done. Made 8922 targets from 1524 files in 5922ms
$ ~/src/ninja/ninja -C out/gn base_unittests
ninja: Entering directory `out/gn'
ninja: error: toolchain.ninja:184: unexpected indent

$ sed -n 182,186p out/gn/toolchain.ninja 
  command = python ../../build/gn_helpers.py -D values=a=0\
b=1
  description = ACTION //base:foo(//build/toolchain/mac:clang_x64)
  restat = 1
  pool = build_toolchain_action_pool



Newlines must be escaped by '$' right in front of the newline in ninja. Looks like gn did write a '\' there, so maybe it attempted to do the right thing and failed, or maybe that's just the escaping for bash.

My guess is that the $0xFF notation is newer than gn's escaping code and since newlines are the only thing needing escaping on the rhs of a command line, no ninja-level escaping code was added at that point.
 

Comment 1 by thakis@chromium.org, Apr 11 2018

Cc: agrieve@chromium.org
Components: Build
Labels: Build-Tools-GN
Status: Untriaged (was: Unconfirmed)
Yup, that is how it happened, $0xff is relatively new: https://codereview.chromium.org/1546393002
Labels: -OS-Mac

Comment 3 by thakis@chromium.org, Apr 16 2018

Labels: OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows
I guess it's OS-All, but we no longer have that label. I hope if I add multiple OSs, it won't show up in the mac triage queue again.
Labels: -OS-Linux -OS-Android -OS-Windows -OS-Chrome -OS-Mac -OS-Fuchsia
A bug that is os-independent should have no boxes checked, not all of them.

Comment 5 by thakis@chromium.org, May 10 2018

I took a short look at this today for a bit. It's not fully clear to me what to do about this. Ninja doesn't have a way to include a \n char on a command ($ followed by a newline is replace with nothing, like the backslash line continuation of C's preprocessor), which is imho a reasonable position to take. So gn would have to write something that can go on a single line but functions as a newilne. In bash, $'\n' does that, but it's bash-specific and on Windows, ninja doesn't run commands in a shell so it's not clear if anything could be done there.

So maybe the thing to do is make gn produce an error at gn time instead.
gnnl.diff
3.6 KB Download

Sign in to add a comment