Yup, it should probably be a syntax error to have a space in a label.

GN should be fine with spaces. And I think it should be possible to express Mac-style names-with-spaces-in-program-names naturally.

The main reason spaces don't work are transformations done with file names in the toolchain definition. On Windows I tried it and got it to work with a trivial addition of quotes in the generated command line. https://codereview.chromium.org/1846873002/

Posix should be similar. Interestingly, spaces in source names are more work but will be similar.

I actually tried to make $ work one time. Theoretically GN's escaper should be able to quote things properly. I forget what the exact problem was. I'm not opposed to disallowing these but this seems low priority.

> GN should be fine with spaces. And I think it should be possible to 
> express Mac-style names-with-spaces-in-program-names naturally.

I think there's two different issues here.

1) Should we support spaces in output_names?
2) Should we support spaces in labels?

I'm fine w/ spaces in output_names. I'm not at all sure I'm okay with 2. I can see how the desire to support 1) plus the desire to have the executable target_names match the executable name might imply 2), but I'm not sure that that's the right tradeoff.

However, I am willing to be outvoted if need be.

Adding thakis@ to this since is the sort of thing he tends to have opinions about :).

I think clearly we need to support spaces in output names, and you're right, I do like being able to match things.

What problem are you trying to solve by disallowing them? We need to support spaces in the path part anyway since one can make directories with spaces in them on every system and GN should be able to have BUILD files in such directories. Disallowing spaces in the name part but not the path part seems weird.

Ah, I sat next to rsesek and didn't realize that this is a filename vs output name issue. It'd be cool if gn said "can't have space in target name, but just set output_name to what you want" instead of silently doing something wrong.

We do need support for spaces in output names.

I'd personally not allow them in labels since it's not needed and if they're supported in labels then people will use them and I'll have to weirdly escape target names on the shell when I want to build targets (`ninja foo\ bar`). But I don't feel strongly about this and the consistency argument is a good one too.

I suppose I can live with having GN support spaces in labels, but I'm not sure we should actually use that in Chrome's build files.

The only executables (bundles) that I know of in chromium that actually have spaces in the names are "Content Shell.app" and "Google Chrome.app". In both cases, in GYP, the target_names do not have spaces (content_shell and chrome, respectively), and I don't think we should change that because it's more important that the label be the same across platforms.

I could maybe live with labels having spaces in the names if they did so across platforms or were platform-specific, but even then it still feels weird to me.

Which target was rsesek was hitting issues with?

It was the "ui_unittests Framework" target. I fixed it by changing the target name to ui_unittests_framework and setting the output_name.

I don't see a ui_unittests[_ ]Framework" target. Is this checked in yet?

I renamed "base unittests" on Linux and it worked fine, so something specific muct be going on for this case.

No, it's a new target I am bringing up (the GYP target has the same name) in https://codereview.chromium.org/1842563006/.

If you can paste the error you got (or at least ping me when this lands with a repro case) that would be helpful.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7508242a626ddf83422d3224f6bc14f553b88a3f

commit 7508242a626ddf83422d3224f6bc14f553b88a3f
Author: brettw <brettw@chromium.org>
Date: Fri Apr 01 00:24:54 2016

Allow spaces in GN target names on Windows.

This allows executables with spaces in their names to work on Windows. Posix probably needs a separate patch.

BUG= 599326 

Review URL: https://codereview.chromium.org/1846873002

Cr-Commit-Position: refs/heads/master@{#384445}

[modify] https://crrev.com/7508242a626ddf83422d3224f6bc14f553b88a3f/build/toolchain/win/BUILD.gn

Patch set 6 of this draft CL https://codereview.chromium.org/1842563006 will produce the following error. Changing the mac_framework target name to "content_shell_framework" resolves the error.

rsesek@hotwire:/Volumes/Build/src(gn) % ninja -C out/gn "Content Shell Framework" -v                              
ninja: Entering directory `out/gn'
[1/1] /Volumes/Build/src/out/Release/gn --root=/Volumes/Build/src -q gen //out/gn/
[1/6] ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF 'obj/content/shell/Content Shell Framework_shared_library/shell_content_main.o'.d -DV8_DEPRECATION_WARNINGS -DCLD_VERSION=2 -DENABLE_NOTIFICATIONS -DENABLE_PEPPER_CDMS -DENABLE_PLUGINS=1 -DENABLE_PDF=1 -DENABLE_PRINTING=1 -DENABLE_BASIC_PRINTING=1 -DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1 -DUSE_BROWSER_SPELLCHECKER=1 -DUSE_OPENSSL=1 -DNO_TCMALLOC -DENABLE_WEBRTC=1 -DDISABLE_NACL -DENABLE_EXTENSIONS=1 -DENABLE_TASK_MANAGER=1 -DENABLE_THEMES=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_PLUGIN_INSTALLATION=1 -DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1 -DENABLE_SUPERVISED_USERS=1 -DENABLE_SERVICE_DISCOVERY=1 -DENABLE_AUTOFILL_DIALOG=1 -DENABLE_TOPCHROME_MD=1 -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DCHROMIUM_BUILD -DENABLE_MEDIA_ROUTER=1 -DFIELDTRIAL_TESTING_ENABLED -DCR_CLANG_REVISION=264334-1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_DEBUG -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -DMOJO_USE_SYSTEM_IMPL -DGTEST_HAS_POSIX_RE=0 -DGTEST_LANG_CXX11=0 -DGTEST_HAS_RTTI=0 -DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS -DSK_SUPPORT_GPU=1 -DSK_BUILD_FOR_MAC -DV8_USE_EXTERNAL_STARTUP_DATA -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DU_NOEXCEPT= -DU_STATIC_IMPLEMENTATION -I../.. -Igen -I../../third_party/khronos -I../../gpu -I../../third_party/libwebp -I../../testing/gtest/include -I../../skia/config -I../../skia/ext -I../../third_party/skia/include/c -I../../third_party/skia/include/config -I../../third_party/skia/include/core -I../../third_party/skia/include/effects -I../../third_party/skia/include/images -I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops -I../../third_party/skia/include/pdf -I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports -I../../third_party/skia/include/utils -I../../third_party/skia/include/gpu -I../../third_party/skia/src/gpu -I../../third_party/WebKit -Igen/third_party/WebKit -I../../v8/include -I../../third_party/icu/source/common -I../../third_party/icu/source/i18n -I../../third_party/npapi -I../../third_party/npapi/bindings -Igen -fno-strict-aliasing -fstack-protector-all -fcolor-diagnostics -fdebug-prefix-map=/Volumes/Build/src=. -arch x86_64 -Wall -Werror -Wextra -Wpartial-availability -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-deprecated-register -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override -Wno-shift-negative-value -O0 -g1 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk -mmacosx-version-min=10.6 -fvisibility=hidden -Xclang -load -Xclang ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib -Xclang -add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs -Xclang check-templates -Xclang -plugin-arg-find-bad-constructs -Xclang follow-macro-expansion -Wheader-hygiene -Wstring-conversion -fno-threadsafe-statics -fvisibility-inlines-hidden -Wno-undefined-bool-conversion -Wno-tautological-undefined-compare -std=c++11 -stdlib=libc++ -fno-rtti -fno-exceptions -c ../../content/shell/app/shell_content_main.cc -o 'obj/content/shell/Content Shell Framework_shared_library/shell_content_main.o'
[2/6] if [ ! -e "./obj/content/shell/libContent Shell Framework.dylib" -o ! -e "./obj/content/shell/libContent Shell Framework.dylib.TOC" ] || otool -l "./obj/content/shell/libContent Shell Framework.dylib" | grep -q LC_REEXPORT_DYLIB ; then ../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared -stdlib=libc++ -L ../../third_party/libc++-static -arch x86_64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk -mmacosx-version-min=10.6 -L/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/usr/lib -o "./obj/content/shell/libContent Shell Framework.dylib" -Wl,-filelist,"./obj/content/shell/libContent Shell Framework.dylib.rsp"  -framework AppKit -framework ApplicationServices -framework Carbon -framework CoreFoundation -framework Foundation -framework IOKit -framework OpenGL -framework Security -framework Cocoa -framework SystemConfiguration -lbsm -framework AVFoundation -framework CoreMedia -framework CoreVideo -framework IOSurface -framework QuartzCore -lsandbox -framework Quartz -framework Accelerate -framework AudioUnit -lresolv -framework AudioToolbox -framework CoreAudio -framework QTKit -framework CoreServices -framework IOBluetooth -framework CoreMIDI -lcups && { otool -l "./obj/content/shell/libContent Shell Framework.dylib" | grep LC_ID_DYLIB -A 5; nm -gP "./obj/content/shell/libContent Shell Framework.dylib" | cut -f1-2 -d' ' | grep -v U$$; true; } > "./obj/content/shell/libContent Shell Framework.dylib.TOC"; else ../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared -stdlib=libc++ -L ../../third_party/libc++-static -arch x86_64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk -mmacosx-version-min=10.6 -L/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/usr/lib -o "./obj/content/shell/libContent Shell Framework.dylib" -Wl,-filelist,"./obj/content/shell/libContent Shell Framework.dylib.rsp"  -framework AppKit -framework ApplicationServices -framework Carbon -framework CoreFoundation -framework Foundation -framework IOKit -framework OpenGL -framework Security -framework Cocoa -framework SystemConfiguration -lbsm -framework AVFoundation -framework CoreMedia -framework CoreVideo -framework IOSurface -framework QuartzCore -lsandbox -framework Quartz -framework Accelerate -framework AudioUnit -lresolv -framework AudioToolbox -framework CoreAudio -framework QTKit -framework CoreServices -framework IOBluetooth -framework CoreMIDI -lcups && { otool -l "./obj/content/shell/libContent Shell Framework.dylib" | grep LC_ID_DYLIB -A 5; nm -gP "./obj/content/shell/libContent Shell Framework.dylib" | cut -f1-2 -d' ' | grep -v U$$; true; } > "./obj/content/shell/libContent Shell Framework.dylib.tmp" && if ! cmp -s "./obj/content/shell/libContent Shell Framework.dylib.tmp" "./obj/content/shell/libContent Shell Framework.dylib.TOC"; then mv "./obj/content/shell/libContent Shell Framework.dylib.tmp" "./obj/content/shell/libContent Shell Framework.dylib.TOC" ; fi; fi
FAILED: if [ ! -e "./obj/content/shell/libContent Shell Framework.dylib" -o ! -e "./obj/content/shell/libContent Shell Framework.dylib.TOC" ] || otool -l "./obj/content/shell/libContent Shell Framework.dylib" | grep -q LC_REEXPORT_DYLIB ; then ../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared -stdlib=libc++ -L ../../third_party/libc++-static -arch x86_64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk -mmacosx-version-min=10.6 -L/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/usr/lib -o "./obj/content/shell/libContent Shell Framework.dylib" -Wl,-filelist,"./obj/content/shell/libContent Shell Framework.dylib.rsp"  -framework AppKit -framework ApplicationServices -framework Carbon -framework CoreFoundation -framework Foundation -framework IOKit -framework OpenGL -framework Security -framework Cocoa -framework SystemConfiguration -lbsm -framework AVFoundation -framework CoreMedia -framework CoreVideo -framework IOSurface -framework QuartzCore -lsandbox -framework Quartz -framework Accelerate -framework AudioUnit -lresolv -framework AudioToolbox -framework CoreAudio -framework QTKit -framework CoreServices -framework IOBluetooth -framework CoreMIDI -lcups && { otool -l "./obj/content/shell/libContent Shell Framework.dylib" | grep LC_ID_DYLIB -A 5; nm -gP "./obj/content/shell/libContent Shell Framework.dylib" | cut -f1-2 -d' ' | grep -v U$$; true; } > "./obj/content/shell/libContent Shell Framework.dylib.TOC"; else ../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared -stdlib=libc++ -L ../../third_party/libc++-static -arch x86_64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk -mmacosx-version-min=10.6 -L/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/usr/lib -o "./obj/content/shell/libContent Shell Framework.dylib" -Wl,-filelist,"./obj/content/shell/libContent Shell Framework.dylib.rsp"  -framework AppKit -framework ApplicationServices -framework Carbon -framework CoreFoundation -framework Foundation -framework IOKit -framework OpenGL -framework Security -framework Cocoa -framework SystemConfiguration -lbsm -framework AVFoundation -framework CoreMedia -framework CoreVideo -framework IOSurface -framework QuartzCore -lsandbox -framework Quartz -framework Accelerate -framework AudioUnit -lresolv -framework AudioToolbox -framework CoreAudio -framework QTKit -framework CoreServices -framework IOBluetooth -framework CoreMIDI -lcups && { otool -l "./obj/content/shell/libContent Shell Framework.dylib" | grep LC_ID_DYLIB -A 5; nm -gP "./obj/content/shell/libContent Shell Framework.dylib" | cut -f1-2 -d' ' | grep -v U$$; true; } > "./obj/content/shell/libContent Shell Framework.dylib.tmp" && if ! cmp -s "./obj/content/shell/libContent Shell Framework.dylib.tmp" "./obj/content/shell/libContent Shell Framework.dylib.TOC"; then mv "./obj/content/shell/libContent Shell Framework.dylib.tmp" "./obj/content/shell/libContent Shell Framework.dylib.TOC" ; fi; fi
ld: file not found: 'obj/content/shell/Content Shell Framework_shared_library/shell_content_main.o'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.

This reminds me of some problems I ran into with Mac response file processing in the linker on Mac. I forgot the exact issue, but they were doing some stupid custom parsing instead of parsing it like a command line which limited the things you could do.

If we are indeed limited in these response files, we should maybe introduce new GN patterns for "sanitized" versions of the label name, etc. that won't cause such parsing issues.

Hm, if support for spaces in label names should even require additional gn language features, that'd be a pretty strong argument against supporting them, imho.

Not a language feature, but a pattern that you can use in a toolchain definition to specify where you want stuff. See "gn help tool" (everything in "{{ }}")

Given the bugs I remember, we might need something similar anyway to support file names with spaces in them (which I believe should be possible even if Chrome doesn't do this in practice).

Re: #13, I think you're right. From `man 1 ld`:

     -filelist file[,dirname]
                 Specifies that the linker should link the files listed in file.  This is an alter-
                 native to listing the files on the command line.  The file names are listed one
                 per line separated only by newlines. (Spaces and tabs are assumed to be part of
                 the file name.)  If the optional directory name, dirname is specified, it is
                 prepended to each name in the list file.

Key part is that spaces and tabs are taken literally. The .rsp file generated uses single quotes to escape strings with spaces:

$ head -1 "out/gn/libContent Shell Framework_shared_library.dylib.rsp"
'obj/content/shell/Content Shell Framework_shared_library/shell_content_main.o'

The GYP build doesn't seem to use rsp files for the link step, since the link command line is smaller due to the lack of source_sets. And as far as I can tell, the rspfile is written by Ninja, not GN, and I think the line that performs the quoting is here: https://github.com/ninja-build/ninja/blob/b231274de33a6ebaa5ed284eb233fa360d84ad8f/src/graph.cc#L283

I spoke to thakis@ and he suggested that it was not correct to use the rspfile as the argument to -filelist, since it doesn't expect an escaped rspfile but rather the linker wants filelist in the format it specifies (above). His suggestion was to not use spaces in the target_name and to just set output_name for that. However that doesn't jive with #4 where brettw@ would like to have target_name match output_name. Alternatively, gn could write the filelist out itself with its own quoting rules, rather than relying on ninja to generate the rspfile.

IMO, having spaces in target names is a bit clunky (e.g. `ninja -C out/gn 'Content Shell Framework'` vs ninja -C out/gn content_shell_framework). And I don't have a good solution for the rspfile issue. But happy to defer to the consensus reached here.

When the thing you're producing has spaces, it doesn't seem at all weird to me that one would need to use spaces when compiling that project in Ninja.

This problem seems more general than just spaces in target names, and I don't think we can solve it by making one restriction for one thing in GN (or maybe we can).

Let's say you had a source file or source directory name with a space in it, or any other character that might need shell escaping. GN generates object file names and directories with the names matching the input files and directories, which would also contain spaces.

It would seem that given these rules and Ninja's behavior it's impossible to link those files. Is that understanding correct? I feel that this should be possible to express (whether it's a good idea and whether Chrome actually does it are different issues). 

The solution I'm leaning toward is introduce "sanitized" versions of the current expansions for file names and directory names such that the toolchain can specify transformations for the object file names that never have spaces. In your understanding, would that be sufficient?

Ninja writes response files. Mac's ld's -filelist does not take a response file. I believe it is correct that it is not possible to use a response file as -filelist parameter if any .o file contains spaces. On other platforms, where linkers do accept response files, this will probably work.

Since gn knows all inputs for the linker, it could write the file passed to filelist itself, if you feel that supporting spaces in .cc files is important.

(I think spaces in product names must work, but people who use spaces in .cc filenames set themselves up for lots of pain in general. I agree that it's nice in principle if gn doesn't make their pain worse.)

Note that the code rsesek linked to only runs for the expansion of $out and $in. If rspfile_contents does not contain $out or $in, iirc it's passed through verbatim.

OS X ld does support -filelist with spaces, you just can't have them quoted. ld will treat each line as the literal characters, up to the \n (just verified this locally, and it's what the man page quoted above says).

Having gn sanitize all the filenames would probably work, but it seems like a better solution would be to not use ninja's rspfile as the input to the filelist if it's going to escape things in a way the linker doesn't want. thakis@ thinks that having gn write the filelist explicitly, with the correct (none) escaping, would work. If that's the case and it's a simpler solution, that's probably better. This is the only issue I've hit so far with spaces in file/target names, so it seems like global sanitization may be a bit overboard.

This is probably done enough to be closed?

Yeah, done enough.