Support more configurability in GN toolchains

Support more configurability in GN toolchains

This uses substitution patterns in toolchains to allow the toolchain to specify more flexibly how files are to be named and generated at each step. The toolchain now has control over the naming of object and executable files, for example, where before these were hardcoded.

This removes most of the OS-specific logic hardcoded into the GN tool. There is still a bunch in action invocation; this will be done in a followup.

R=jamesr@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290685

[brettw, 2014-08-19 16:44:51]

HOW TO REVIEW:

1. Read the documentation in function_toolchain.cc. and look at the example
toolchain in tools/gn/example/build/toolchain/BUILD.gn. It's important this
makes sense and it will also help you understand the rest of the patch.

2. Look at substitution_*. These are pretty mechanical and separable and you can
cross them off the list.

3. Look at tool.* toolchain.* and function_toolchain.cc. This should also be
pretty boring, they just read and store the new values.

4. Look at target.cc, and ninja_*_target_writer.cc. This is the meat of the
patch.

5. Look at the rest. Most of the rest are just minor tweaks the helper functions
and test setup that changed.

[jamesr, 2014-08-19 19:30:42]

lgtm

When are you using CHECK vs DCHECK?  It doesn't seem super obvious to me why a
particular instance is one or the other

https://codereview.chromium.org/440333002/diff/160001/tools/gn/escape.cc
File tools/gn/escape.cc (right):

https://codereview.chromium.org/440333002/diff/160001/tools/gn/escape.cc#newc...
tools/gn/escape.cc:57: dest->push_back('$');
what is this accomplishing? it looks like this function is just a string copy...

https://codereview.chromium.org/440333002/diff/160001/tools/gn/function_rebas...
File tools/gn/function_rebase_path.cc (left):

https://codereview.chromium.org/440333002/diff/160001/tools/gn/function_rebas...
tools/gn/function_rebase_path.cc:257: printf("foo\n");
lol

https://codereview.chromium.org/440333002/diff/160001/tools/gn/function_toolc...
File tools/gn/function_toolchain.cc (right):

https://codereview.chromium.org/440333002/diff/160001/tools/gn/function_toolc...
tools/gn/function_toolchain.cc:161: /*
?

https://codereview.chromium.org/440333002/diff/160001/tools/gn/function_toolc...
tools/gn/function_toolchain.cc:353: "        Valid for: linker tools
(required)\n"
this says 'required' but the code allows it to be missing. seems like optional
makes more sense since you don't have to use it

https://codereview.chromium.org/440333002/diff/160001/tools/gn/function_toolc...
tools/gn/function_toolchain.cc:471: "        TODO(brettw) write this. Probably
doesn't work yet.\n"
should we just leave this out?

https://codereview.chromium.org/440333002/diff/160001/tools/gn/function_toolc...
tools/gn/function_toolchain.cc:551: "    {{defines}}\n"
does this have /Dxxx or -Dxxx or is it just xxx? example might be useful

https://codereview.chromium.org/440333002/diff/160001/tools/gn/function_toolc...
tools/gn/function_toolchain.cc:609: "        Extra libraries from shared library
dependncies not specified\n"
typo 'dependncies' -> 'dependencies'

https://codereview.chromium.org/440333002/diff/160001/tools/gn/function_toolc...
tools/gn/function_toolchain.cc:614: "        These should basically be treated
by sources by your tool.\n"
dunno what 'by sources' means - do you mean 'treated as sources ' ... ?

https://codereview.chromium.org/440333002/diff/160001/tools/gn/ninja_target_w...
File tools/gn/ninja_target_writer.cc (right):

https://codereview.chromium.org/440333002/diff/160001/tools/gn/ninja_target_w...
tools/gn/ninja_target_writer.cc:216: i != hard_deps.end(); ++i)
think this gets {} since the for loop is multiline

https://codereview.chromium.org/440333002/diff/160001/tools/gn/ninja_target_w...
File tools/gn/ninja_target_writer.h (right):

https://codereview.chromium.org/440333002/diff/160001/tools/gn/ninja_target_w...
tools/gn/ninja_target_writer.h:12: #include "tools/gn/substitution_type.h"
could just forward declare SubstitutionBits, but this is fine too

https://codereview.chromium.org/440333002/diff/160001/tools/gn/ninja_utils.h
File tools/gn/ninja_utils.h (right):

https://codereview.chromium.org/440333002/diff/160001/tools/gn/ninja_utils.h#...
tools/gn/ninja_utils.h:11: class SourceFile;
does a forward declaration of a return type work?

https://codereview.chromium.org/440333002/diff/160001/tools/gn/substitution_p...
File tools/gn/substitution_pattern.h (right):

https://codereview.chromium.org/440333002/diff/160001/tools/gn/substitution_p...
tools/gn/substitution_pattern.h:8: #include <functional>
doesn't appear to be used

https://codereview.chromium.org/440333002/diff/160001/tools/gn/target.cc
File tools/gn/target.cc (right):

https://codereview.chromium.org/440333002/diff/160001/tools/gn/target.cc#newc...
tools/gn/target.cc:159: DCHECK(output_type_ != UNKNOWN);
DCHECK_NE will give better output on failure

https://codereview.chromium.org/440333002/diff/160001/tools/gn/target.cc#newc...
tools/gn/target.cc:169: "The target " + label().GetUserVisibleName(false) +
this could use some stringprintf, imo. it's really hard to read the mix of
literals and expressions

https://codereview.chromium.org/440333002/diff/160001/tools/gn/target.cc#newc...
tools/gn/target.cc:265: CHECK(tool->outputs().list().size() >= 1);
CHECK_GE produces slightly better output on failure (i guess if it fails you
know it's 0 anyway)

https://codereview.chromium.org/440333002/diff/160001/tools/gn/tool.cc
File tools/gn/tool.cc (right):

https://codereview.chromium.org/440333002/diff/160001/tools/gn/tool.cc#newcode7
tools/gn/tool.cc:7: #include <string.h>
not used

https://codereview.chromium.org/440333002/diff/160001/tools/gn/tool.h
File tools/gn/tool.h (right):

https://codereview.chromium.org/440333002/diff/160001/tools/gn/tool.h#newcode25
tools/gn/tool.h:25: // Getters/setters
----------------------------------------------------------
seems like this should just be a struct with public members

https://codereview.chromium.org/440333002/diff/160001/tools/gn/tool.h#newcode81
tools/gn/tool.h:81: const std::string& lib_dir_switch() const {
either put a blank line between different getter/setter pairs or don't, having a
mix in this file is jarring to read

https://codereview.chromium.org/440333002/diff/160001/tools/gn/toolchain.h
File tools/gn/toolchain.h (right):

https://codereview.chromium.org/440333002/diff/160001/tools/gn/toolchain.h#ne...
tools/gn/toolchain.h:29: // safetly be accessed from any thread at any time (we
do this when asking for
typo 'safetly' -> 'safely'

[brettw, 2014-08-19 21:29:57]

New snap up if you care.

https://codereview.chromium.org/440333002/diff/160001/tools/gn/escape.cc
File tools/gn/escape.cc (right):

https://codereview.chromium.org/440333002/diff/160001/tools/gn/escape.cc#newc...
tools/gn/escape.cc:57: dest->push_back('$');
On 2014/08/19 19:30:41, jamesr wrote:
> what is this accomplishing? it looks like this function is just a string
copy...

It's a string copy that converts "$" -> "$$" (the char is still copied in the
case below)

https://codereview.chromium.org/440333002/diff/160001/tools/gn/function_toolc...
File tools/gn/function_toolchain.cc (right):

https://codereview.chromium.org/440333002/diff/160001/tools/gn/function_toolc...
tools/gn/function_toolchain.cc:161: /*
On 2014/08/19 19:30:41, jamesr wrote:
> ?

Deleted

https://codereview.chromium.org/440333002/diff/160001/tools/gn/ninja_utils.h
File tools/gn/ninja_utils.h (right):

https://codereview.chromium.org/440333002/diff/160001/tools/gn/ninja_utils.h#...
tools/gn/ninja_utils.h:11: class SourceFile;
On 2014/08/19 19:30:41, jamesr wrote:
> does a forward declaration of a return type work?

Yeah, returning an object basically ends up looking like an implicit output
pointer parameter, so it doesn't care.

https://codereview.chromium.org/440333002/diff/160001/tools/gn/tool.h
File tools/gn/tool.h (right):

https://codereview.chromium.org/440333002/diff/160001/tools/gn/tool.h#newcode25
tools/gn/tool.h:25: // Getters/setters
----------------------------------------------------------
On 2014/08/19 19:30:42, jamesr wrote:
> seems like this should just be a struct with public members

I wanted to have the DCHECKs for verifying that it is not modiofied at the wrong
time.

https://codereview.chromium.org/440333002/diff/160001/tools/gn/tool.h#newcode81
tools/gn/tool.h:81: const std::string& lib_dir_switch() const {
On 2014/08/19 19:30:42, jamesr wrote:
> either put a blank line between different getter/setter pairs or don't, having
a
> mix in this file is jarring to read

Done.

[brettw, 2014-08-19 22:52:23]

Committed patchset #12 manually as 290685 (presubmit successful).