DevTools: Support debug_devtools=true arg in GN builds

DevTools: Support debug_devtools=true arg in GN builds

R=dgozman@chromium.org, lushnikov, pfeldman

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183061

[apavlov, 2014-09-25 15:02:11]

apavlov@chromium.org changed reviewers:
+ dgozman@chromium.org

[apavlov, 2014-09-25 15:02:12]

+dgozman

[dgozman, 2014-09-25 16:43:15]

I don't like that debug_devtools is all over the place. Can we group
debug/release rules together?

https://codereview.chromium.org/608523002/diff/1/Source/devtools/BUILD.gn
File Source/devtools/BUILD.gn (right):

https://codereview.chromium.org/608523002/diff/1/Source/devtools/BUILD.gn#new...
Source/devtools/BUILD.gn:269: ":build_common_module",
alphabetize?

https://codereview.chromium.org/608523002/diff/1/Source/devtools/BUILD.gn#new...
Source/devtools/BUILD.gn:365: print(target_name)
remove?

[apavlov, 2014-09-26 06:44:45]

apavlov@chromium.org changed reviewers:
+ brettw@chromium.org

[apavlov, 2014-09-26 06:49:08]

On 2014/09/25 16:43:15, dgozman wrote:
> I don't like that debug_devtools is all over the place. Can we group
> debug/release rules together?

I have retained only two conditionals at the top level, and merging them seems
wrong. One of them is for the variable declarations (since GN does not allow
unused vars), and the other one is for the rules proper.

[apavlov, 2014-09-26 07:00:24]

https://codereview.chromium.org/608523002/diff/1/Source/devtools/BUILD.gn
File Source/devtools/BUILD.gn (right):

https://codereview.chromium.org/608523002/diff/1/Source/devtools/BUILD.gn#new...
Source/devtools/BUILD.gn:269: ":build_common_module",
On 2014/09/25 16:43:15, dgozman wrote:
> alphabetize?

Done.

https://codereview.chromium.org/608523002/diff/1/Source/devtools/BUILD.gn#new...
Source/devtools/BUILD.gn:365: print(target_name)
On 2014/09/25 16:43:15, dgozman wrote:
> remove?

Done.

[apavlov, 2014-09-26 14:58:34]

On 2014/09/25 16:43:15, dgozman wrote:
> I don't like that debug_devtools is all over the place. Can we group
> debug/release rules together?

I have hit the situation where unrelated targets for debug and release modes
generate the same sets of resources (e.g. app.js and app.css), which breaks
things heavily. I have removed and commented out some [seemingly] harmless
conditionals.

[apavlov, 2014-09-26 15:02:43]

On 2014/09/25 16:43:15, dgozman wrote:
> I don't like that debug_devtools is all over the place. Can we group
> debug/release rules together?


@brettw: Brett, I've tried minimizing the number of checks but still, there's
almost a dozen of those sprinkled around BUILD.gn. The culprit is that our build
procedure concatenates a number of resource sets (or otherwise processes them)
in the !debug_devtools mode or just copies them to the deployment location in
the debug_devtools mode (and we have many more of similar conditionals in
devtools.gyp). Have you encountered similar situations during the gyp-gn
conversion?

[dgozman, 2014-09-26 15:35:37]

https://codereview.chromium.org/608523002/diff/40001/Source/devtools/BUILD.gn
File Source/devtools/BUILD.gn (right):

https://codereview.chromium.org/608523002/diff/40001/Source/devtools/BUILD.gn...
Source/devtools/BUILD.gn:33: gypi_values.devtools_sdk_js_files +
sdk duplicated

[brettw, 2014-09-26 18:24:12]

https://codereview.chromium.org/608523002/diff/40001/Source/devtools/BUILD.gn
File Source/devtools/BUILD.gn (right):

https://codereview.chromium.org/608523002/diff/40001/Source/devtools/BUILD.gn...
Source/devtools/BUILD.gn:7: app_names = [
Are we anticipating adding lots more apps here? If this is going to be static or
only get one more, I'd actually prefer to list all the things out manually.

[apavlov, 2014-09-26 19:47:07]

https://codereview.chromium.org/608523002/diff/40001/Source/devtools/BUILD.gn
File Source/devtools/BUILD.gn (right):

https://codereview.chromium.org/608523002/diff/40001/Source/devtools/BUILD.gn...
Source/devtools/BUILD.gn:7: app_names = [
On 2014/09/26 18:24:12, brettw wrote:
> Are we anticipating adding lots more apps here? If this is going to be static
or
> only get one more, I'd actually prefer to list all the things out manually.

Another app is actually pending addition (from dgozman). I'm not sure how many
apps can get added as the project evolves, but one or two more are easily
possible.

https://codereview.chromium.org/608523002/diff/40001/Source/devtools/BUILD.gn...
Source/devtools/BUILD.gn:33: gypi_values.devtools_sdk_js_files +
On 2014/09/26 15:35:37, dgozman wrote:
> sdk duplicated

Thanks, it's a bad rebase

[brettw, 2014-09-26 19:52:53]

I prefer no foreach if there's only a couple more apps. I didn't even want to
add this feature since it makes it hard to follow what's happening. For example,
you can't grep for some target names and source files, errors become more
difficult to understand, and I don't think in this case it's much more
duplication.

[apavlov, 2014-09-26 20:00:24]

On 2014/09/26 19:52:53, brettw wrote:
> I prefer no foreach if there's only a couple more apps. I didn't even want to
> add this feature since it makes it hard to follow what's happening. For
example,
> you can't grep for some target names and source files, errors become more
> difficult to understand, and I don't think in this case it's much more
> duplication.

Okay, reverting this should not be too hard.
Do you think it is possible to further reduce the number of "if
(debug_devtools)" conditionals? A few targets have different behavior, depending
on what mode we are building in (and sometimes they should produce different
contents of same-named files in the gen/... directory). Did you have to convert
any gyp-based builds similar to this one?

[brettw, 2014-09-26 20:39:27]

I don't remember specifically. I remember there's some devtools stuff in
chrome/browser. Probably just grep for gyp files with debug_devtools should give
the full set.

[apavlov, 2014-09-29 14:32:40]

On 2014/09/26 20:39:27, brettw wrote:
> I don't remember specifically. I remember there's some devtools stuff in
> chrome/browser. Probably just grep for gyp files with debug_devtools should
give
> the full set.

Yeah, thanks. I'll have to stick with this number of if's then...

[dgozman, 2014-10-01 13:28:24]

lgtm

https://codereview.chromium.org/608523002/diff/80001/Source/devtools/BUILD.gn
File Source/devtools/BUILD.gn (right):

https://codereview.chromium.org/608523002/diff/80001/Source/devtools/BUILD.gn...
Source/devtools/BUILD.gn:238: action("build_applications_release") {
Shouldn't you add "build_applications_release" to the deps of
"build_applications"?

[apavlov, 2014-10-01 13:55:05]

The CQ bit was checked by apavlov@chromium.org

[apavlov, 2014-10-01 13:55:19]

https://codereview.chromium.org/608523002/diff/80001/Source/devtools/BUILD.gn
File Source/devtools/BUILD.gn (right):

https://codereview.chromium.org/608523002/diff/80001/Source/devtools/BUILD.gn...
Source/devtools/BUILD.gn:238: action("build_applications_release") {
On 2014/10/01 13:28:24, dgozman wrote:
> Shouldn't you add "build_applications_release" to the deps of
> "build_applications"?

Done.

[commit-bot: I haz the power, 2014-10-01 13:55:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/608523002/100001

[apavlov, 2014-10-01 14:13:52]

Committed patchset #6 (id:100001) manually as 183061 (presubmit successful).