Support for source_target_relative expansion in GN

Support for source_target_relative expansion in GN

This can be used to get a path to the source file relative to the target's
directory.

Committed: https://crrev.com/7d88638248b10cec98364cf561f268cbed62587e
Cr-Commit-Position: refs/heads/master@{#439541}

[Petr Hosek, 2016-09-30 19:06:24]

Description was changed from

==========
Support for source_literal expansion in GN

This can be used to copy source file name as is without any
processing into the output.
==========

to

==========
Support for source_literal expansion in GN

This can be used to copy source file name as is without any
processing into the output.
==========

[Petr Hosek, 2016-09-30 19:06:25]

phosek@chromium.org changed reviewers:
+ brettw@chromium.org, dpranke@chromium.org

[Petr Hosek, 2016-09-30 19:10:20]

We already discussed this briefly with Brett over IM. The primary use case is
the following case:

copy("headers") {
  sources = [
    "include/foo.h",
    "include/bits/bar.h",
  ]
  outputs = [
    "${sysroot}/{{source_literal}}",
  ]
}

Currently there is no way to do this, you need have one copy target for each
subdirectory of include which is a little inconvenient. Regarding the
source_literal name, I'm open to suggestions.

[brettw, 2016-09-30 21:06:18]

Does this work? In looking at the structure everything has been converted to
SourceFile's by the time we expand these patterns. This means that all sources
should be absolute, and the only difference between this mode and
source_root_relative_dir is that the new one will include "//" at the beginning.

What about calling it something like {{source_target_relative}} which would
rebase the directory to be relative to the directory of the target, which I
think will be what you want.

[Petr Hosek, 2016-10-05 16:49:45]

Description was changed from

==========
Support for source_literal expansion in GN

This can be used to copy source file name as is without any
processing into the output.
==========

to

==========
Support for source_target_relative expansion in GN

This can be used to copy source file name as is without any
processing into the output.
==========

[Petr Hosek, 2016-10-05 16:50:33]

Description was changed from

==========
Support for source_target_relative expansion in GN

This can be used to copy source file name as is without any
processing into the output.
==========

to

==========
Support for source_target_relative expansion in GN

This can be used to get a path to the source file relative to the target's
directory.
==========

[Petr Hosek, 2016-10-05 16:51:23]

On 2016/09/30 21:06:18, brettw (ping on IM after 24h) wrote:
> Does this work? In looking at the structure everything has been converted to
> SourceFile's by the time we expand these patterns. This means that all sources
> should be absolute, and the only difference between this mode and
> source_root_relative_dir is that the new one will include "//" at the
beginning.
> 
> What about calling it something like {{source_target_relative}} which would
> rebase the directory to be relative to the directory of the target, which I
> think will be what you want.

Done, it's a bit more complicated and not as simple as I thought it'd be but it
seems to be working.

[brettw, 2016-10-05 19:47:50]

This looks great, just some minor tweaks.

https://codereview.chromium.org/2387763002/diff/20001/tools/gn/function_proce...
File tools/gn/function_process_file_template.cc (right):

https://codereview.chromium.org/2387763002/diff/20001/tools/gn/function_proce...
tools/gn/function_process_file_template.cc:94: 
Here can you check that subst.required_types() doesn't contain
SOURCE_TARGET_RELATIVE and throw an error if it does? I think it's better to be
really obvious that this doesn't work.

https://codereview.chromium.org/2387763002/diff/20001/tools/gn/substitution_w...
File tools/gn/substitution_writer.cc (right):

https://codereview.chromium.org/2387763002/diff/20001/tools/gn/substitution_w...
tools/gn/substitution_writer.cc:112: "        \"//foo/bar/baz.txt\" =>
\"baz.txt\"\n"
Can this comment mention this only works in actions, and it's an error o use in
process_file_template (where there is no "target").

https://codereview.chromium.org/2387763002/diff/20001/tools/gn/substitution_w...
tools/gn/substitution_writer.cc:398: if (target)
Chrome-style says this condition needs {}

https://codereview.chromium.org/2387763002/diff/20001/tools/gn/substitution_w...
tools/gn/substitution_writer.cc:401: to_rebase = source.value();
In the "else" case here, I think it would be better to return
"_UNSUPPORTED_SUBSTITUTION_" or something so it's obvious if somebody encounters
this that they messed up, rather than the paths just being wonky.

https://codereview.chromium.org/2387763002/diff/20001/tools/gn/substitution_w...
File tools/gn/substitution_writer.h (right):

https://codereview.chromium.org/2387763002/diff/20001/tools/gn/substitution_w...
tools/gn/substitution_writer.h:48: // writer since they depend on traversing the
dependency tree.
Can you append a note here that the functions that take a target can accept a
null target pointer if there is no target context, and then the substitutions
requiring target context won't work.

https://codereview.chromium.org/2387763002/diff/20001/tools/gn/substitution_w...
tools/gn/substitution_writer.h:83: // first (see for example IsFileInOuputDir).
GN almost never supports random pointers being null, so I think it's worthwhile
to call out that target can be null really obviously. In each of these blocks
with comments above the function, can you append "The target can be null (see
class comment above)."

[Petr Hosek, 2016-10-11 00:59:38]

The CQ bit was checked by phosek@chromium.org to run a CQ dry run

[Petr Hosek, 2016-10-11 00:59:50]

https://codereview.chromium.org/2387763002/diff/20001/tools/gn/function_proce...
File tools/gn/function_process_file_template.cc (right):

https://codereview.chromium.org/2387763002/diff/20001/tools/gn/function_proce...
tools/gn/function_process_file_template.cc:94: 
On 2016/10/05 19:47:50, brettw (ping on IM after 24h) wrote:
> Here can you check that subst.required_types() doesn't contain
> SOURCE_TARGET_RELATIVE and throw an error if it does? I think it's better to
be
> really obvious that this doesn't work.

Done.

https://codereview.chromium.org/2387763002/diff/20001/tools/gn/substitution_w...
File tools/gn/substitution_writer.cc (right):

https://codereview.chromium.org/2387763002/diff/20001/tools/gn/substitution_w...
tools/gn/substitution_writer.cc:112: "        \"//foo/bar/baz.txt\" =>
\"baz.txt\"\n"
On 2016/10/05 19:47:50, brettw (ping on IM after 24h) wrote:
> Can this comment mention this only works in actions, and it's an error o use
in
> process_file_template (where there is no "target").

Done.

https://codereview.chromium.org/2387763002/diff/20001/tools/gn/substitution_w...
tools/gn/substitution_writer.cc:398: if (target)
On 2016/10/05 19:47:50, brettw (ping on IM after 24h) wrote:
> Chrome-style says this condition needs {}

Done.

https://codereview.chromium.org/2387763002/diff/20001/tools/gn/substitution_w...
tools/gn/substitution_writer.cc:401: to_rebase = source.value();
On 2016/10/05 19:47:50, brettw (ping on IM after 24h) wrote:
> In the "else" case here, I think it would be better to return
> "_UNSUPPORTED_SUBSTITUTION_" or something so it's obvious if somebody
encounters
> this that they messed up, rather than the paths just being wonky.

Done.

https://codereview.chromium.org/2387763002/diff/20001/tools/gn/substitution_w...
File tools/gn/substitution_writer.h (right):

https://codereview.chromium.org/2387763002/diff/20001/tools/gn/substitution_w...
tools/gn/substitution_writer.h:48: // writer since they depend on traversing the
dependency tree.
On 2016/10/05 19:47:50, brettw (ping on IM after 24h) wrote:
> Can you append a note here that the functions that take a target can accept a
> null target pointer if there is no target context, and then the substitutions
> requiring target context won't work.

Done.

https://codereview.chromium.org/2387763002/diff/20001/tools/gn/substitution_w...
tools/gn/substitution_writer.h:83: // first (see for example IsFileInOuputDir).
On 2016/10/05 19:47:50, brettw (ping on IM after 24h) wrote:
> GN almost never supports random pointers being null, so I think it's
worthwhile
> to call out that target can be null really obviously. In each of these blocks
> with comments above the function, can you append "The target can be null (see
> class comment above)."

Done.

[commit-bot: I haz the power, 2016-10-11 01:00:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 01:14:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-11 01:14:42]

Dry run: This issue passed the CQ dry run.

[brettw, 2016-10-11 22:35:24]

lgtm

[Petr Hosek, 2016-12-19 20:25:04]

The CQ bit was checked by phosek@chromium.org

[Petr Hosek, 2016-12-19 20:25:04]

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/2387763002/#ps60001
(title: "Rebased")

[commit-bot: I haz the power, 2016-12-19 20:25:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-19 20:38:44]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482179104327570,
"parent_rev": "6d66169b4559a1049d289d25881a5115477d7626", "commit_rev":
"ca14af0288dabc3760a23661fff98c2307031a51"}

[commit-bot: I haz the power, 2016-12-19 20:39:19]

Description was changed from

==========
Support for source_target_relative expansion in GN

This can be used to get a path to the source file relative to the target's
directory.
==========

to

==========
Support for source_target_relative expansion in GN

This can be used to get a path to the source file relative to the target's
directory.

Review-Url: https://codereview.chromium.org/2387763002
==========

[commit-bot: I haz the power, 2016-12-19 20:39:20]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-19 20:41:08]

Description was changed from

==========
Support for source_target_relative expansion in GN

This can be used to get a path to the source file relative to the target's
directory.

Review-Url: https://codereview.chromium.org/2387763002
==========

to

==========
Support for source_target_relative expansion in GN

This can be used to get a path to the source file relative to the target's
directory.

Committed: https://crrev.com/7d88638248b10cec98364cf561f268cbed62587e
Cr-Commit-Position: refs/heads/master@{#439541}
==========

[commit-bot: I haz the power, 2016-12-19 20:41:09]

Patchset 4 (id:??) landed as
https://crrev.com/7d88638248b10cec98364cf561f268cbed62587e
Cr-Commit-Position: refs/heads/master@{#439541}