Fix dependencies on policy_component_test_support

Fix dependencies on policy_component_test_support

BUG=

Committed: https://crrev.com/ef5d5ac94ec7a6e8e6b687da4ae4dfc408107697
Cr-Commit-Position: refs/heads/master@{#359529}

[Tomasz Moniuszko, 2015-09-08 13:17:08]

tmoniuszko@opera.com changed reviewers:
+ bartfab@chromium.org

[bartfab (slow), 2015-09-08 15:01:03]

https://codereview.chromium.org/1321013007/diff/1/components/policy/core/brow...
File components/policy/core/browser/BUILD.gn (right):

https://codereview.chromium.org/1321013007/diff/1/components/policy/core/brow...
components/policy/core/browser/BUILD.gn:96: if (enable_configuration_policy) {
I cannot see the purpose of trying to build policy unit tests with policy
disabled at build-time?

[Tomasz Moniuszko, 2015-09-09 07:54:51]

https://codereview.chromium.org/1321013007/diff/1/components/policy/core/brow...
File components/policy/core/browser/BUILD.gn (right):

https://codereview.chromium.org/1321013007/diff/1/components/policy/core/brow...
components/policy/core/browser/BUILD.gn:96: if (enable_configuration_policy) {
On 2015/09/08 15:01:03, bartfab wrote:
> I cannot see the purpose of trying to build policy unit tests with policy
> disabled at build-time?

//components/policy:policy_component_test_support is defined only if
enable_configuration_policy==true.

Do you think whole //components/policy/core/browser:unit_tests and
//components/policy/core/common:unit_tests (or even all targets in
//components/policy) should be conditionally defined?

On the other hand enable_configuration_policy causes some issues occasionally.
See https://codereview.chromium.org/952163002/,
https://codereview.chromium.org/1009883002,
https://codereview.chromium.org/1079083003 and
https://codereview.chromium.org/1133853005 (which is still hanging). It seems
enable_configuration_policy is now true for all Chromium configurations so maybe
it would be a better idea to remove it completely?

[bartfab (slow), 2015-09-14 17:38:56]

https://codereview.chromium.org/1321013007/diff/1/components/policy/core/brow...
File components/policy/core/browser/BUILD.gn (right):

https://codereview.chromium.org/1321013007/diff/1/components/policy/core/brow...
components/policy/core/browser/BUILD.gn:96: if (enable_configuration_policy) {
On 2015/09/09 07:54:51, Tomasz Moniuszko wrote:
> On 2015/09/08 15:01:03, bartfab wrote:
> > I cannot see the purpose of trying to build policy unit tests with policy
> > disabled at build-time?
> 
> //components/policy:policy_component_test_support is defined only if
> enable_configuration_policy==true.
> 
> Do you think whole //components/policy/core/browser:unit_tests and
> //components/policy/core/common:unit_tests (or even all targets in
> //components/policy) should be conditionally defined?
> 
> On the other hand enable_configuration_policy causes some issues occasionally.
> See https://codereview.chromium.org/952163002/,
> https://codereview.chromium.org/1009883002,
> https://codereview.chromium.org/1079083003 and
> https://codereview.chromium.org/1133853005 (which is still hanging). It seems
> enable_configuration_policy is now true for all Chromium configurations so
maybe
> it would be a better idea to remove it completely?

We do like the idea of a lighter-weight option for those who do not need policy.
But as you noted yourself, enable_configuration_policy=true for Chrome on all
platforms, which means that enable_configuration_policy=false gets no bot
coverage and slowly rots away. Fixes are welcome but we are not officially
maintaining this configuration in any way.

Regarding your specific CL: With your fix applied, does a
enable_configuration_policy=false build still build the unit_test target in this
file (I presume yes)? What happens if you run the tests (I presume they all
fail)? To me, the entire unit_tests target makes no sense if
enable_configuration_policy=false. The whole thing should be disabled.

componets/policy cannot be completely yanked when
enable_configuration_policy=false because there is a tiny bit of glue code that
is used unconditionally. But all the real functionality and tests should be
disabled.

[Tomasz Moniuszko, 2015-09-15 12:05:11]

tmoniuszko@opera.com changed reviewers:
+ maniscalco@chromium.org, stevet@chromium.org

[Tomasz Moniuszko, 2015-09-15 12:05:12]

https://codereview.chromium.org/1321013007/diff/1/components/policy/core/brow...
File components/policy/core/browser/BUILD.gn (right):

https://codereview.chromium.org/1321013007/diff/1/components/policy/core/brow...
components/policy/core/browser/BUILD.gn:96: if (enable_configuration_policy) {
On 2015/09/14 17:38:56, bartfab wrote:
> componets/policy cannot be completely yanked when
> enable_configuration_policy=false because there is a tiny bit of glue code
that
> is used unconditionally. But all the real functionality and tests should be
> disabled.

Thank you for your reply. In Patch Set 2 I disabled unit_tests targets in
//components/policy/core/browser and //components/policy/core/common if
enable_configuration_policy=false as you suggested. Also two tests in
//components/search_engines:unit_tests and //components/sync_driver:unit_tests
are now included conditionally as they fail to link if
enable_configuration_policy=false.

[maniscalco, 2015-09-15 15:23:47]

maniscalco@chromium.org changed reviewers:
+ zea@chromium.org
- maniscalco@chromium.org

[maniscalco, 2015-09-15 15:23:47]

-maniscalco, +zea as I no longer work on sync.

[bartfab (slow), 2015-09-16 13:59:25]

LGTM. Thanks for bearing with me as we figured out the best way to unbreak this
build-time flag.

For bonus points, it would be nice to clean this up in the corresponding gyp
files as well of course :).

[Nicolas Zea, 2015-09-16 15:55:31]

sync_driver LGTM

[SteveT, 2015-09-16 17:28:08]

stevet@chromium.org changed reviewers:
+ pkasting@chromium.org

[SteveT, 2015-09-16 17:28:08]

R: -me, +pkasting

I've never touched the gn build stuff so I'm not the right person to speak about
that. Peter, do you know anything about this.


Steve

[Peter Kasting, 2015-09-16 20:28:22]

On 2015/09/16 17:28:08, SteveT wrote:
> I've never touched the gn build stuff so I'm not the right person to speak
about
> that. Peter, do you know anything about this.

I don't know much about gn; more importantly I'm not sure what the question here
is.  What additional review, approval, or answers to questions are needed here?

[Tomasz Moniuszko, 2015-09-18 07:38:06]

On 2015/09/16 13:59:25, bartfab wrote:
> LGTM. Thanks for bearing with me as we figured out the best way to unbreak
this
> build-time flag.
> 
> For bonus points, it would be nice to clean this up in the corresponding gyp
> files as well of course :).

Done in Patch Set 3. Could you please have a look?

[Tomasz Moniuszko, 2015-09-18 07:52:06]

On 2015/09/16 20:28:22, Peter Kasting wrote:
> On 2015/09/16 17:28:08, SteveT wrote:
> > I've never touched the gn build stuff so I'm not the right person to speak
> about
> > that. Peter, do you know anything about this.
> 
> I don't know much about gn; more importantly I'm not sure what the question
here
> is.  What additional review, approval, or answers to questions are needed
here?

Well, I touched file in components/search_engines and "git cl" suggested adding
a reviewer there. There are no additional questions. If you don't see anything
suspicious in my change, I'm probably not doing anything wrong to this code :)

[bartfab (slow), 2015-09-18 12:18:06]

Interesting. It looks like gyp excluded policy tests already when policies were
disabled. gn did not match that behavior and we are only noticing now as we
slowly switch from gyp to gn.

LGTM

[Tomasz Moniuszko, 2015-09-18 12:34:03]

The CQ bit was checked by tmoniuszko@opera.com

[Tomasz Moniuszko, 2015-09-18 12:34:03]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1321013007/#ps40001
(title: "Cleanup in corresponding GYP file")

[commit-bot: I haz the power, 2015-09-18 12:34:12]

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

[commit-bot: I haz the power, 2015-09-18 12:35:37]

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

[commit-bot: I haz the power, 2015-09-18 12:35:38]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Tomasz Moniuszko, 2015-09-21 12:52:21]

Somebody was faster than me and integrated the solution similar to my Patch Set
1 so the original issue is no longer reproducible. However Patch Set 4 includes
more of cleanup changes and I think it's still worth to integrate it.

[Tomasz Moniuszko, 2015-09-22 07:28:34]

The CQ bit was checked by tmoniuszko@opera.com

[Tomasz Moniuszko, 2015-09-22 07:28:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
bartfab@chromium.org, zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1321013007/#ps60001
(title: "Rebase to current master")

[commit-bot: I haz the power, 2015-09-22 07:28:38]

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

[commit-bot: I haz the power, 2015-09-22 07:38:11]

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

[commit-bot: I haz the power, 2015-09-22 07:38:12]

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

[Tomasz Moniuszko, 2015-09-22 08:18:18]

On 2015/09/22 07:38:12, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

@pkasting: It looks like this CL still needs LGTM in
components/search_engines/BUILD.gn. Could you please take a look?

[Tomasz Moniuszko, 2015-11-12 10:36:05]

components/search_engines owners: ping?

[Peter Kasting, 2015-11-12 22:58:47]

I'm sorry, I missed this; LGTM.

[Tomasz Moniuszko, 2015-11-13 09:44:30]

search_engines and sync_driver changes are present on master already.

[Tomasz Moniuszko, 2015-11-13 09:44:46]

The CQ bit was checked by tmoniuszko@opera.com

[Tomasz Moniuszko, 2015-11-13 09:44:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
bartfab@chromium.org, zea@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1321013007/#ps80001
(title: "Rebase onto current master")

[commit-bot: I haz the power, 2015-11-13 09:45:14]

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

[commit-bot: I haz the power, 2015-11-13 10:29:05]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-13 10:30:44]

Patchset 5 (id:??) landed as
https://crrev.com/ef5d5ac94ec7a6e8e6b687da4ae4dfc408107697
Cr-Commit-Position: refs/heads/master@{#359529}