Add support for substring replacement in gypi_to_gn

Add support for substring replacement in gypi_to_gn

Use substring replacement in skia source file list, and update the skia BUILD.gn file accordingly. The .gn file gets a bit simpler now: we can do the substring replacement to make absolute paths rather than doing rebase for all the files.

R=djsollen@google.com, dpranke@chromium.org

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

[brettw, 2014-04-09 21:21:33]

Dirk: Python code
Derek: BUILD.gn and skia file list.

[Dirk Pranke, 2014-04-10 01:18:20]

lgtm w/ a couple nits assuming I'm just missing something for the two new skia
variables; otherwise, you need to fix that :).

https://codereview.chromium.org/231983002/diff/1/build/gypi_to_gn.py
File build/gypi_to_gn.py (right):

https://codereview.chromium.org/231983002/diff/1/build/gypi_to_gn.py#newcode97
build/gypi_to_gn.py:97: return result
Nit: one line: "return [ReplaceSubstrings(v, search_for, replace_with) for v in
values]"

https://codereview.chromium.org/231983002/diff/1/build/gypi_to_gn.py#newcode104
build/gypi_to_gn.py:104: ReplaceSubstrings(values[v], search_for, replace_with)
Nit: I'd probably rewrite this to:

result = {}
for key, value in values.items():
  new_key = ReplaceSubstrings(key, search_for, replace_with)
  new_value = ReplaceSubstrings(value, search_for, replace_with)
  result[new_key] = new_value

Perhaps a little more verbose, but clearer and a bit more idiomatic, I think.

https://codereview.chromium.org/231983002/diff/1/skia/skia_gn_files.gypi
File skia/skia_gn_files.gypi (right):

https://codereview.chromium.org/231983002/diff/1/skia/skia_gn_files.gypi#newc...
skia/skia_gn_files.gypi:27: '<(skia_include_path)/gpu/GrBackendEffectFactory.h',
where are skia_include_path and skia_src_path defined ? I don't actually see
these two variables in the existing skia files ...

[djsollen, 2014-04-10 12:47:31]

lgtm with a suggestion to get back to one list of source files.

https://codereview.chromium.org/231983002/diff/1/skia/skia_gn_files.gypi
File skia/skia_gn_files.gypi (right):

https://codereview.chromium.org/231983002/diff/1/skia/skia_gn_files.gypi#newc...
skia/skia_gn_files.gypi:26: 'skia_gpu_sources': [
Why not just delete these lists entirely and have the script read the *.gypi
files from ../third_party/skia/gyp/gpu.gypi?  If we do that we should be able to
delete this file if we move the skia_feature_defines into BUILD.gn.

Would you prefer to do this in a follow-up CL?

https://codereview.chromium.org/231983002/diff/1/skia/skia_gn_files.gypi#newc...
skia/skia_gn_files.gypi:27: '<(skia_include_path)/gpu/GrBackendEffectFactory.h',
On 2014/04/10 01:18:20, Dirk Pranke wrote:
> where are skia_include_path and skia_src_path defined ? I don't actually see
> these two variables in the existing skia files ...

They are defined in skia_common.gypi

[brettw, 2014-04-10 19:24:52]

GN doesn't really understand nested dictionaries, and the variables are in a
"variables" nested dictionary. I was assume that the variables section would be
moved from the .gypi to the including .gyp files.

However, this just came up in a different patch, so it's maybe a common enough
pattern to handle. I think gypi_to_gn can probably check for a root "variables"
dictionary and just collapse it away which would allow the existing gpu.gypi
file to be re-used.

I'mm make this change in the script and then you guys can work on hooking it up
the way you want to the existing .gypi file lists. I really think the Skia team
needs to learn how this works so you can maintain it, and it isn't scalable for
me to convert the entire build by myself. Does this sound like a good plan?

[djsollen, 2014-04-10 19:45:52]

On 2014/04/10 19:24:52, brettw wrote:
> GN doesn't really understand nested dictionaries, and the variables are in a
> "variables" nested dictionary. I was assume that the variables section would
be
> moved from the .gypi to the including .gyp files.
> 
> However, this just came up in a different patch, so it's maybe a common enough
> pattern to handle. I think gypi_to_gn can probably check for a root
"variables"
> dictionary and just collapse it away which would allow the existing gpu.gypi
> file to be re-used.
> 
> I'mm make this change in the script and then you guys can work on hooking it
up
> the way you want to the existing .gypi file lists. I really think the Skia
team
> needs to learn how this works so you can maintain it, and it isn't scalable
for
> me to convert the entire build by myself. Does this sound like a good plan?

Yes, I agree that we need to maintain this in the long term and I appreciate you
taking the time to help make the transition happen.  I forgot that our gpu.gypi
is different than others like core.gypi which don't nest dictionaries.  After
this lands I'll take a stab at making this use the Skia gypi files.

[brettw, 2014-04-11 17:24:04]

Committed patchset #4 manually as r263272 (presubmit successful).