Cleanup chrome/version.h handling

Cleanup chrome/common/chrome_version.h handling

Rename "version.h" to "chrome/common/chrome_version.h"
Update #include and GYP/GN references
Generate on all platforms
Remove redundant generated chrome/common/chrome_version.cc.version
Fix DEPS include_rules

BUG=542807
Committed: https://crrev.com/59647ecac9524d954010d3d10b539ee4a340bf94
Cr-Commit-Position: refs/heads/master@{#355196}

Committed: https://crrev.com/31f086c2013853749cac45522eb950b9d0ab0302
Cr-Commit-Position: refs/heads/master@{#355377}

Committed: https://crrev.com/15a48be892b15ac3acc0eb4b823abfbaf75dfa15
Cr-Commit-Position: refs/heads/master@{#355434}

[mdempsky, 2015-10-13 22:48:30]

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

[mdempsky, 2015-10-13 22:48:31]

brettw: Not ready to submit yet, but thoughts on the overall direction?

I'm confused why the ios_rel_device_ninja and ios_dbg_simulator_ninja bots are
failing though.  They're complaining about a cyclic dependency from chrome.gyp
-> common_constants.gyp -> chrome.gyp, but I can't understand why that's not
triggering on the Windows bots too (e.g., chrome.gyp:crash_service ->
common_constants.gyp:common_constants -> chrome.gyp:chrome_version_header).  If
you have any ideas on what's going on here or how to resolve it, please advise.

[mdempsky, 2015-10-13 22:54:50]

On 2015/10/13 22:48:31, mdempsky wrote:
> I'm confused why the ios_rel_device_ninja and ios_dbg_simulator_ninja bots are
> failing though.

Apparently iOS is the only platform where we currently disallow circular
dependencies between GYP files.  From build/gyp_chromium.py:

  # There shouldn't be a circular dependency relationship between .gyp files,
  # but in Chromium's .gyp files, on non-Mac platforms, circular relationships
  # currently exist.  The check for circular dependencies is currently
  # bypassed on other platforms, but is left enabled on iOS, where a violation
  # of the rule causes Xcode to misbehave badly.
  # TODO(mark): Find and kill remaining circular dependencies, and remove this
  # option.  http://crbug.com/35878.
  # TODO(tc): Fix circular dependencies in ChromiumOS then add linux2 to the
  # list.
  if gyp_vars_dict.get('OS') != 'ios':
    args.append('--no-circular-check')

[brettw, 2015-10-13 23:44:20]

Seems like an improvement, although I do wonder if it should be in
chrome/common. It's not clear to me what the relationship between this and
chrome_version.cc without further study. Do they go together or are they totally
different things?

[mdempsky, 2015-10-14 00:15:27]

On 2015/10/13 23:44:20, brettw wrote:
> Seems like an improvement, although I do wonder if it should be in
> chrome/common. It's not clear to me what the relationship between this and
> chrome_version.cc without further study. Do they go together or are they
totally
> different things?

They compute the same values, just one is a macro string literal while the other
is an external/opaque global char array.  I'd have to dig into their histories
to see why they're separate, but I don't see any reason for them to remain
separate today.

[mdempsky, 2015-10-14 17:17:05]

On 2015/10/13 23:44:20, brettw wrote:
> Seems like an improvement, although I do wonder if it should be in
> chrome/common.

I'm going to split the chrome_version.cc cleanup into a separate CL because the
iOS build is going to require some more involved .gyp file refactoring.

Do you want to make a decision on chrome/version.h vs chrome/common/version.h
for this CL?  If you're uncertain, I'd like to proceed with chrome/version.h for
now (to unblock kerrnel@), and I'm happy to move it again in a followup CL.

[mdempsky, 2015-10-14 18:02:17]

brettw: PTAL.  I think this is ready to submit, except maybe if you'd prefer
renaming to chrome/common/version.h.

[Greg K, 2015-10-16 20:22:44]

On 2015/10/14 18:02:17, mdempsky wrote:
> brettw: PTAL.  I think this is ready to submit, except maybe if you'd prefer
> renaming to chrome/common/version.h.

Matt, thanks for working on this. One observation, when I was building something
with this change, if I just built the chrome target, it didn't build (version.h
was missing). But I build chrome_version_header explicitly, as seen blow, it
works. Is that expected behavior?

ninja -C out/Debug chrome_version_header chrome

[mdempsky, 2015-10-16 20:36:30]

On 2015/10/16 20:22:44, Greg Kerr wrote:
> Matt, thanks for working on this. One observation, when I was building
something
> with this change, if I just built the chrome target, it didn't build
(version.h
> was missing). But I build chrome_version_header explicitly, as seen blow, it
> works. Is that expected behavior?

Unless you're building on Windows, I think that's the expected behavior. 
chrome_version_header used to be a Windows-only target, so the chrome target
couldn't/doesn't yet depend on it on other platforms.

[brettw, 2015-10-16 21:11:48]

Yeah, I would prefer it being in chrome/common. If we can't think of a good
reason why not to do that, can we do that here?

LGTM

[Greg K, 2015-10-16 21:12:41]

On 2015/10/16 20:22:44, Greg Kerr wrote:
> On 2015/10/14 18:02:17, mdempsky wrote:
> > brettw: PTAL.  I think this is ready to submit, except maybe if you'd prefer
> > renaming to chrome/common/version.h.
> 
> Matt, thanks for working on this. One observation, when I was building
something
> with this change, if I just built the chrome target, it didn't build
(version.h
> was missing). But I build chrome_version_header explicitly, as seen blow, it
> works. Is that expected behavior?
> 
> ninja -C out/Debug chrome_version_header chrome

I tracked this down to a missing dependency needed in a target I'm using. Please
proceed with the CL as is and I will add the dependency as part of my CL. Thanks
again for doing this.

[mdempsky, 2015-10-16 21:16:18]

On 2015/10/16 21:11:48, brettw wrote:
> Yeah, I would prefer it being in chrome/common. If we can't think of a good
> reason why not to do that, can we do that here?

I'll take a look.  Do you want me to move/rename GYP's
chrome/chrome.gyp:chrome_version_header and GN's //chrome:version_header build
targets while I'm at it too?

[brettw, 2015-10-16 22:17:25]

I'd say the build rules for both GYP and GN should be in the corresponding
"common" files.

[mdempsky, 2015-10-20 20:59:04]

Description was changed from

==========
Cleanup chrome/version.h handling

Rename "version.h" to "chrome/version.h"
Update #include references
Generate on all platforms
Fix DEPS include_rules

BUG=542807
==========

to

==========
Cleanup chrome/common/chrome_version.h handling

Rename "version.h" to "chrome/common/chrome_version.h"
Update #include and GYP/GN references
Generate on all platforms
Remove redundant generated chrome/common/chrome_version.cc.version
Fix DEPS include_rules

BUG=542807
==========

[mdempsky, 2015-10-20 21:02:53]

On 2015/10/16 22:17:25, brettw wrote:
> I'd say the build rules for both GYP and GN should be in the corresponding
> "common" files.

I've moved them both next to the "common_constants" rules, which also allowed me
to get rid of the redundant chrome_version.cc.version file.

After doing that, I decided to rename the header to
"chrome/common/chrome_version.h" to better match the convention of the other
"chrome/common/chrome_FOO.h" headers.

[brettw, 2015-10-20 23:25:48]

sweet, lgtm

[mdempsky, 2015-10-20 23:35:05]

The CQ bit was checked by mdempsky@chromium.org

[commit-bot: I haz the power, 2015-10-20 23:35:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1405633002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1405633002/120001

[commit-bot: I haz the power, 2015-10-21 00:16:50]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-21 00:17:48]

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

[Mathieu, 2015-10-21 03:16:06]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1411173003/ by mathp@chromium.org.

The reason for reverting is: Appears to be breaking at least Mac GN build

Example failure:
http://build.chromium.org/p/chromium.mac/builders/Mac%20GN%20%28dbg%29/builds....

[mdempsky, 2015-10-21 16:05:54]

Description was changed from

==========
Cleanup chrome/common/chrome_version.h handling

Rename "version.h" to "chrome/common/chrome_version.h"
Update #include and GYP/GN references
Generate on all platforms
Remove redundant generated chrome/common/chrome_version.cc.version
Fix DEPS include_rules

BUG=542807

Committed: https://crrev.com/59647ecac9524d954010d3d10b539ee4a340bf94
Cr-Commit-Position: refs/heads/master@{#355196}
==========

to

==========
Cleanup chrome/common/chrome_version.h handling

Rename "version.h" to "chrome/common/chrome_version.h"
Update #include and GYP/GN references
Generate on all platforms
Remove redundant generated chrome/common/chrome_version.cc.version
Fix DEPS include_rules

BUG=542807

Committed: https://crrev.com/59647ecac9524d954010d3d10b539ee4a340bf94
Cr-Commit-Position: refs/heads/master@{#355196}
==========

[mdempsky, 2015-10-21 16:09:12]

On 2015/10/21 03:16:06, Mathieu Perreault wrote:
> Example failure:
>
http://build.chromium.org/p/chromium.mac/builders/Mac%20GN%20%28dbg%29/builds....

This appears to be because chrome_tests.gypi:chromedriver_lib was simply
compiling chrome_constants.cc as a source, rather than depending on
common_constants.gyp:common_constants.  I've fixed that in patch set 8.

[mdempsky, 2015-10-21 16:09:16]

The CQ bit was checked by mdempsky@chromium.org

[mdempsky, 2015-10-21 16:09:17]

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/1405633002/#ps140001
(title: "Fix webdriver test sources dependency")

[commit-bot: I haz the power, 2015-10-21 16:09:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1405633002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1405633002/140001

[commit-bot: I haz the power, 2015-10-21 16:19:28]

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

[commit-bot: I haz the power, 2015-10-21 16:19:29]

Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mdempsky, 2015-10-21 18:46:22]

The CQ bit was checked by mdempsky@chromium.org

[mdempsky, 2015-10-21 18:46:22]

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/1405633002/#ps160001
(title: "Sigh")

[commit-bot: I haz the power, 2015-10-21 18:46:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1405633002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1405633002/160001

[commit-bot: I haz the power, 2015-10-21 19:56:39]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-10-21 19:57:34]

Patchset 9 (id:??) landed as
https://crrev.com/31f086c2013853749cac45522eb950b9d0ab0302
Cr-Commit-Position: refs/heads/master@{#355377}

[Mathieu, 2015-10-21 20:51:38]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/1418933002/ by mathp@chromium.org.

The reason for reverting is: Causing failures on the GN build (not gyp) for Mac:

http://build.chromium.org/p/chromium.mac/builders/Mac%20GN%20%28dbg%29/builds....

[mdempsky, 2015-10-21 21:16:12]

Description was changed from

==========
Cleanup chrome/common/chrome_version.h handling

Rename "version.h" to "chrome/common/chrome_version.h"
Update #include and GYP/GN references
Generate on all platforms
Remove redundant generated chrome/common/chrome_version.cc.version
Fix DEPS include_rules

BUG=542807

Committed: https://crrev.com/59647ecac9524d954010d3d10b539ee4a340bf94
Cr-Commit-Position: refs/heads/master@{#355196}

Committed: https://crrev.com/31f086c2013853749cac45522eb950b9d0ab0302
Cr-Commit-Position: refs/heads/master@{#355377}
==========

to

==========
Cleanup chrome/common/chrome_version.h handling

Rename "version.h" to "chrome/common/chrome_version.h"
Update #include and GYP/GN references
Generate on all platforms
Remove redundant generated chrome/common/chrome_version.cc.version
Fix DEPS include_rules

BUG=542807

Committed: https://crrev.com/59647ecac9524d954010d3d10b539ee4a340bf94
Cr-Commit-Position: refs/heads/master@{#355196}

Committed: https://crrev.com/31f086c2013853749cac45522eb950b9d0ab0302
Cr-Commit-Position: refs/heads/master@{#355377}
==========

[mdempsky, 2015-10-21 23:14:08]

The CQ bit was checked by mdempsky@chromium.org

[mdempsky, 2015-10-21 23:14:08]

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/1405633002/#ps180001
(title: "Building Chrome is hard")

[commit-bot: I haz the power, 2015-10-21 23:14:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1405633002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1405633002/180001

[commit-bot: I haz the power, 2015-10-21 23:38:04]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-10-21 23:38:49]

Patchset 10 (id:??) landed as
https://crrev.com/15a48be892b15ac3acc0eb4b823abfbaf75dfa15
Cr-Commit-Position: refs/heads/master@{#355434}