gn BUILD fixes for disabling enable_extensions and use_ash feature flags.

gn BUILD fixes for disabling enable_extensions and use_ash feature flags.

BUG=

Committed: https://crrev.com/2e75c702de464a5cc1cd2272ef720fbe1068c141
Cr-Commit-Position: refs/heads/master@{#399693}

[asaka, 2016-05-04 13:54:02]

Description was changed from

==========
gn BUILD fixes for disabling enable_extensions and use_ash feature flags.

BUG=
==========

to

==========
gn BUILD fixes for disabling enable_extensions and use_ash feature flags.

BUG=
==========

[asaka, 2016-05-04 13:54:02]

asaka@opera.com changed reviewers:
+ dpranke@google.com

[asaka, 2016-05-04 14:03:25]

@dpranke, this patch is needed for us to do gn builds without extensions.

The chrome/browser/media/router/BUILD.gn and chrome/test/BUILD.gn updates are
not necessary on our side but was required for me to test this in your
repository. I have confirmed I can call 'gn gen' with
--args="enable_extensions=false" and --args="enable_ash=false", as well as build
content_shell with enable_extensions=false.

Does it look OK to you?

BR,
Åsa

[asaka, 2016-05-04 14:04:48]

On 2016/05/04 14:03:25, asaka wrote:
> @dpranke, this patch is needed for us to do gn builds without extensions.
> 
> The chrome/browser/media/router/BUILD.gn and chrome/test/BUILD.gn updates are
> not necessary on our side but was required for me to test this in your
> repository. I have confirmed I can call 'gn gen' with
> --args="enable_extensions=false" and --args="enable_ash=false", as well as
build
> content_shell with enable_extensions=false.
> 
> Does it look OK to you?
> 
> BR,
> Åsa

I mean --args="use_ash=false" of course!

[Dirk Pranke, 2016-05-05 02:38:47]

dpranke@chromium.org changed reviewers:
+ dpranke@chromium.org

[Dirk Pranke, 2016-05-05 02:38:48]

https://codereview.chromium.org/1950003002/diff/1/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1950003002/diff/1/BUILD.gn#newcode198
BUILD.gn:198: }
Can you check if we can lift these blocks out of the os-specific if blocks,
since presumably enable_extensions and use_ash is not true on the platforms that
we're excluding?

[asaka, 2016-05-11 05:54:14]

https://codereview.chromium.org/1950003002/diff/1/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1950003002/diff/1/BUILD.gn#newcode198
BUILD.gn:198: }
On 2016/05/05 02:38:48, Dirk Pranke wrote:
> Can you check if we can lift these blocks out of the os-specific if blocks,
> since presumably enable_extensions and use_ash is not true on the platforms
that
> we're excluding?

I agree it sounds plausible, but I don't know how to verify it. Any suggestions
of targets/configurations I should test build?

From features.gni:
enable_extensions = !is_android && !is_ios

is_chromecast it also supported on is_linux afaict so I think I should test
build that if you can suggest the target?

[asaka, 2016-05-11 07:52:31]

On 2016/05/11 05:54:14, asaka wrote:
> https://codereview.chromium.org/1950003002/diff/1/BUILD.gn
> File BUILD.gn (right):
> 
> https://codereview.chromium.org/1950003002/diff/1/BUILD.gn#newcode198
> BUILD.gn:198: }
> On 2016/05/05 02:38:48, Dirk Pranke wrote:
> > Can you check if we can lift these blocks out of the os-specific if blocks,
> > since presumably enable_extensions and use_ash is not true on the platforms
> that
> > we're excluding?
> 
> I agree it sounds plausible, but I don't know how to verify it. Any
suggestions
> of targets/configurations I should test build?
> 
> From features.gni:
> enable_extensions = !is_android && !is_ios
> 
> is_chromecast it also supported on is_linux afaict so I think I should test
> build that if you can suggest the target?

I found these instructions:
https://chromium.googlesource.com/chromium/src/+/master/docs/cast_build_instr...

gn gen out/Debug --args='is_chromecast=true is_debug=true'
ninja -C out/Debug cast_shell

worked fine after the change so I'll upload a patch!

[asaka, 2016-05-11 13:01:53]

I made a mistake in the update and the deps ended up under !is_chromecast
anyway. I'm correct this by moving it all the way outside and test building
again, but I'm starting to wonder what the "correct" way of doing it is?

The original BUILD.gn had all the extensions deps except
"//extensions/shell:app_shell_unittests" under the "!is_chromecast" check. This
seems like a strange deps to leave outside so I don't want to copy that behavior
in my patch. 

For this to be a correct patch, I'm wondering if the features.gni should not
rather be updated to:
  enable_extensions = !is_android && !is_ios && !is_chromecast
?

[Dirk Pranke, 2016-05-12 02:01:01]

I would adjust features.gni as you suggest, and then make the changes I suggest
below.

https://codereview.chromium.org/1950003002/diff/20001/BUILD.gn
File BUILD.gn (left):

https://codereview.chromium.org/1950003002/diff/20001/BUILD.gn#oldcode257
BUILD.gn:257: }
I would leave this block in, and move lines 147 and 148 here.

https://codereview.chromium.org/1950003002/diff/20001/BUILD.gn#oldcode488
BUILD.gn:488: deps += [ "//extensions/shell:app_shell" ]
and move this up into the block on line 256 as well.

[asaka, 2016-05-19 11:51:03]

@dpranke, do you think you will have time to look at the new patch soon? It
would be great to see this land since it's causing some conflicts in our regular
imports.

Thanks,
Åsa
On 2016/05/12 02:01:01, Dirk Pranke wrote:
> I would adjust features.gni as you suggest, and then make the changes I
suggest
> below.
> 
> https://codereview.chromium.org/1950003002/diff/20001/BUILD.gn
> File BUILD.gn (left):
> 
> https://codereview.chromium.org/1950003002/diff/20001/BUILD.gn#oldcode257
> BUILD.gn:257: }
> I would leave this block in, and move lines 147 and 148 here.
> 
> https://codereview.chromium.org/1950003002/diff/20001/BUILD.gn#oldcode488
> BUILD.gn:488: deps += [ "//extensions/shell:app_shell" ]
> and move this up into the block on line 256 as well.

[Dirk Pranke, 2016-05-19 18:28:50]

lgtm :).

Sorry, I didn't realize you had posted an updated patch.

[mark a. foltz, 2016-05-19 20:00:38]

mfoltz@chromium.org changed reviewers:
+ mfoltz@chromium.org

[mark a. foltz, 2016-05-19 20:00:39]

https://codereview.chromium.org/1950003002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/BUILD.gn (right):

https://codereview.chromium.org/1950003002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/BUILD.gn:32: if (enable_extensions) {
Is there now a valid build target with !is_android and !enable_extensions? 
media_router_non_android_sources won't compile without these deps.

[asaka, 2016-05-20 05:59:41]

https://codereview.chromium.org/1950003002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/BUILD.gn (right):

https://codereview.chromium.org/1950003002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/BUILD.gn:32: if (enable_extensions) {
On 2016/05/19 20:00:39, mark a. foltz wrote:
> Is there now a valid build target with !is_android and !enable_extensions? 
> media_router_non_android_sources won't compile without these deps.

Hi!

I think chromecast may end up as that with this patch? The linux build worked
for me on my local build from your repository. (gn gen out/Debug
--args='is_chromecast=true is_debug=true'; ninja -C out/Debug cast_shell) 

What do you think?

[mark a. foltz, 2016-05-20 17:48:21]

https://codereview.chromium.org/1950003002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/BUILD.gn (right):

https://codereview.chromium.org/1950003002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/BUILD.gn:32: if (enable_extensions) {
On 2016/05/20 at 05:59:41, asaka wrote:
> On 2016/05/19 20:00:39, mark a. foltz wrote:
> > Is there now a valid build target with !is_android and !enable_extensions? 
> > media_router_non_android_sources won't compile without these deps.
> 
> Hi!
> 
> I think chromecast may end up as that with this patch? The linux build worked
for me on my local build from your repository. (gn gen out/Debug
--args='is_chromecast=true is_debug=true'; ninja -C out/Debug cast_shell) 
> 
> What do you think?

I think this works because //chromecast is prevented from depending on //chrome,
so cast_shell doesn't depend on this target anyway.  (You can double check with
gn.)

The right fix would probably be to add more conditions to enable_media_router to
exclude is_chromecast, and ensure that enable_extensions is set on the
appropriate platforms.  We can make that fix in a separate patch.

As is, this change doesn't make a lot of sense in isolation.  For example try
building chrome or unit_tests with is_android=false enable_extensions=false.  I
recommend reverting this file.

[asaka, 2016-05-24 10:57:32]

https://codereview.chromium.org/1950003002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/BUILD.gn (right):

https://codereview.chromium.org/1950003002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/BUILD.gn:32: if (enable_extensions) {
On 2016/05/20 17:48:21, mark a. foltz wrote:
> On 2016/05/20 at 05:59:41, asaka wrote:
> > On 2016/05/19 20:00:39, mark a. foltz wrote:
> > > Is there now a valid build target with !is_android and !enable_extensions?

> > > media_router_non_android_sources won't compile without these deps.
> > 
> > Hi!
> > 
> > I think chromecast may end up as that with this patch? The linux build
worked
> for me on my local build from your repository. (gn gen out/Debug
> --args='is_chromecast=true is_debug=true'; ninja -C out/Debug cast_shell) 
> > 
> > What do you think?
> 
> I think this works because //chromecast is prevented from depending on
//chrome,
> so cast_shell doesn't depend on this target anyway.  (You can double check
with
> gn.)
> 
> The right fix would probably be to add more conditions to enable_media_router
to
> exclude is_chromecast, and ensure that enable_extensions is set on the
> appropriate platforms.  We can make that fix in a separate patch.
> 
> As is, this change doesn't make a lot of sense in isolation.  For example try
> building chrome or unit_tests with is_android=false enable_extensions=false. 
I
> recommend reverting this file.

Reverting this change only will not build:

$ gn gen out/Debug --args='is_chromecast=true is_debug=true'
ERROR at //extensions/strings/BUILD.gn:7:1: Assertion failed.
assert(enable_extensions)
^-----
See //extensions/browser/BUILD.gn:29:5: which caused the file to be included.
    "//extensions/strings",
    ^---------------------

gn gen out/Debug --args='is_chromecast=true is_debug=true
enable_media_router=false' seems to work. I can build it and "something" starts
up on linux.

For generic build I also need to do --args="enable_extensions=false
enable_media_router=false" since --args="enable_extensions=false" 'gn gen' will
fail. I think making this dependency sounds suboptimal, but since this file is
not the main conflict in out imports I can accept it in this patch from our pov.


is_andorid is not a valid argument for out/Default afaict. But I assume that
when I do a general chrome build with just "--enable_extensions=false" I will
also get is_android = false since I runt the binary on my linux host after? I
verified this works with this patch earlier.

[mark a. foltz, 2016-05-25 01:23:44]

LGTM

On 2016/05/24 at 10:57:32, asaka wrote:
>
https://codereview.chromium.org/1950003002/diff/40001/chrome/browser/media/ro...
> File chrome/browser/media/router/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1950003002/diff/40001/chrome/browser/media/ro...
> chrome/browser/media/router/BUILD.gn:32: if (enable_extensions) {
> On 2016/05/20 17:48:21, mark a. foltz wrote:
> > On 2016/05/20 at 05:59:41, asaka wrote:
> > > On 2016/05/19 20:00:39, mark a. foltz wrote:
> > > > Is there now a valid build target with !is_android and
!enable_extensions? 
> > > > media_router_non_android_sources won't compile without these deps.
> > > 
> > > Hi!
> > > 
> > > I think chromecast may end up as that with this patch? The linux build
worked
> > for me on my local build from your repository. (gn gen out/Debug
> > --args='is_chromecast=true is_debug=true'; ninja -C out/Debug cast_shell) 
> > > 
> > > What do you think?
> > 
> > I think this works because //chromecast is prevented from depending on
//chrome,
> > so cast_shell doesn't depend on this target anyway.  (You can double check
with
> > gn.)
> > 
> > The right fix would probably be to add more conditions to
enable_media_router to
> > exclude is_chromecast, and ensure that enable_extensions is set on the
> > appropriate platforms.  We can make that fix in a separate patch.
> > 
> > As is, this change doesn't make a lot of sense in isolation.  For example
try
> > building chrome or unit_tests with is_android=false enable_extensions=false.
 I
> > recommend reverting this file.
> 
> Reverting this change only will not build:
> 
> $ gn gen out/Debug --args='is_chromecast=true is_debug=true'
> ERROR at //extensions/strings/BUILD.gn:7:1: Assertion failed.
> assert(enable_extensions)
> ^-----
> See //extensions/browser/BUILD.gn:29:5: which caused the file to be included.
>     "//extensions/strings",
>     ^---------------------
> 

Right, is_chromecast = true should imply enable_media_router = false.

> gn gen out/Debug --args='is_chromecast=true is_debug=true
enable_media_router=false' seems to work. I can build it and "something" starts
up on linux.
> 
> For generic build I also need to do --args="enable_extensions=false
enable_media_router=false" since --args="enable_extensions=false" 'gn gen' will
fail. I think making this dependency sounds suboptimal, but since this file is
not the main conflict in out imports I can accept it in this patch from our pov.


This is not valid, as you can't build media_router without extensions on
non-Android platforms.

> 
> is_andorid is not a valid argument for out/Default afaict. But I assume that
when I do a general chrome build with just "--enable_extensions=false" I will
also get is_android = false since I runt the binary on my linux host after? I
verified this works with this patch earlier.

I think enable_extensions is forced to false if is_android is true.  I'm not
sure about the converse though.

I don't think it's worth blocking this patch if it is solving some problem for
Chromecast, even if the solution is suboptimal and will need to be undone later.
 I will file an issue to try to more correctly define the conditions for
enable_media_router and allow all platforms to build with legal combinations of
these flags.

[asaka, 2016-05-25 07:41:41]

Sorry, did not catch the conclusion here :)

Do I revert the changes in this file and disable media_router for chromecast in
features.gni to allow it to build, or do I leave it as is?

(actually, if I do revert, should features.gni rather have something like
'enable_media_router = !is_ios && (enable_extensions || is_android)') ?

On 2016/05/25 01:23:44, mark a. foltz wrote:
> LGTM
> 
> On 2016/05/24 at 10:57:32, asaka wrote:
> >
>
https://codereview.chromium.org/1950003002/diff/40001/chrome/browser/media/ro...
> > File chrome/browser/media/router/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/1950003002/diff/40001/chrome/browser/media/ro...
> > chrome/browser/media/router/BUILD.gn:32: if (enable_extensions) {
> > On 2016/05/20 17:48:21, mark a. foltz wrote:
> > > On 2016/05/20 at 05:59:41, asaka wrote:
> > > > On 2016/05/19 20:00:39, mark a. foltz wrote:
> > > > > Is there now a valid build target with !is_android and
> !enable_extensions? 
> > > > > media_router_non_android_sources won't compile without these deps.
> > > > 
> > > > Hi!
> > > > 
> > > > I think chromecast may end up as that with this patch? The linux build
> worked
> > > for me on my local build from your repository. (gn gen out/Debug
> > > --args='is_chromecast=true is_debug=true'; ninja -C out/Debug cast_shell) 
> > > > 
> > > > What do you think?
> > > 
> > > I think this works because //chromecast is prevented from depending on
> //chrome,
> > > so cast_shell doesn't depend on this target anyway.  (You can double check
> with
> > > gn.)
> > > 
> > > The right fix would probably be to add more conditions to
> enable_media_router to
> > > exclude is_chromecast, and ensure that enable_extensions is set on the
> > > appropriate platforms.  We can make that fix in a separate patch.
> > > 
> > > As is, this change doesn't make a lot of sense in isolation.  For example
> try
> > > building chrome or unit_tests with is_android=false
enable_extensions=false.
>  I
> > > recommend reverting this file.
> > 
> > Reverting this change only will not build:
> > 
> > $ gn gen out/Debug --args='is_chromecast=true is_debug=true'
> > ERROR at //extensions/strings/BUILD.gn:7:1: Assertion failed.
> > assert(enable_extensions)
> > ^-----
> > See //extensions/browser/BUILD.gn:29:5: which caused the file to be
included.
> >     "//extensions/strings",
> >     ^---------------------
> > 
> 
> Right, is_chromecast = true should imply enable_media_router = false.
> 
> > gn gen out/Debug --args='is_chromecast=true is_debug=true
> enable_media_router=false' seems to work. I can build it and "something"
starts
> up on linux.
> > 
> > For generic build I also need to do --args="enable_extensions=false
> enable_media_router=false" since --args="enable_extensions=false" 'gn gen'
will
> fail. I think making this dependency sounds suboptimal, but since this file is
> not the main conflict in out imports I can accept it in this patch from our
pov.
> 
> 
> This is not valid, as you can't build media_router without extensions on
> non-Android platforms.
> 
> > 
> > is_andorid is not a valid argument for out/Default afaict. But I assume that
> when I do a general chrome build with just "--enable_extensions=false" I will
> also get is_android = false since I runt the binary on my linux host after? I
> verified this works with this patch earlier.
> 
> I think enable_extensions is forced to false if is_android is true.  I'm not
> sure about the converse though.
> 
> I don't think it's worth blocking this patch if it is solving some problem for
> Chromecast, even if the solution is suboptimal and will need to be undone
later.
>  I will file an issue to try to more correctly define the conditions for
> enable_media_router and allow all platforms to build with legal combinations
of
> these flags.

[mark a. foltz, 2016-05-25 15:50:01]

It's okay to submit as is.
On May 25, 2016 12:41 AM, <asaka@opera.com> wrote:

> Sorry, did not catch the conclusion here :)
>
> Do I revert the changes in this file and disable media_router for
> chromecast in
> features.gni to allow it to build, or do I leave it as is?
>
> (actually, if I do revert, should features.gni rather have something like
> 'enable_media_router = !is_ios && (enable_extensions || is_android)') ?
>
> On 2016/05/25 01:23:44, mark a. foltz wrote:
> > LGTM
> >
> > On 2016/05/24 at 10:57:32, asaka wrote:
> > >
> >
>
>
https://codereview.chromium.org/1950003002/diff/40001/chrome/browser/media/ro...
> > > File chrome/browser/media/router/BUILD.gn (right):
> > >
> > >
> >
>
>
https://codereview.chromium.org/1950003002/diff/40001/chrome/browser/media/ro...
> > > chrome/browser/media/router/BUILD.gn:32: if (enable_extensions) {
> > > On 2016/05/20 17:48:21, mark a. foltz wrote:
> > > > On 2016/05/20 at 05:59:41, asaka wrote:
> > > > > On 2016/05/19 20:00:39, mark a. foltz wrote:
> > > > > > Is there now a valid build target with !is_android and
> > !enable_extensions?
> > > > > > media_router_non_android_sources won't compile without these
> deps.
> > > > >
> > > > > Hi!
> > > > >
> > > > > I think chromecast may end up as that with this patch? The linux
> build
> > worked
> > > > for me on my local build from your repository. (gn gen out/Debug
> > > > --args='is_chromecast=true is_debug=true'; ninja -C out/Debug
> cast_shell)
> > > > >
> > > > > What do you think?
> > > >
> > > > I think this works because //chromecast is prevented from depending
> on
> > //chrome,
> > > > so cast_shell doesn't depend on this target anyway. (You can double
> check
> > with
> > > > gn.)
> > > >
> > > > The right fix would probably be to add more conditions to
> > enable_media_router to
> > > > exclude is_chromecast, and ensure that enable_extensions is set on
> the
> > > > appropriate platforms. We can make that fix in a separate patch.
> > > >
> > > > As is, this change doesn't make a lot of sense in isolation. For
> example
> > try
> > > > building chrome or unit_tests with is_android=false
> enable_extensions=false.
> > I
> > > > recommend reverting this file.
> > >
> > > Reverting this change only will not build:
> > >
> > > $ gn gen out/Debug --args='is_chromecast=true is_debug=true'
> > > ERROR at //extensions/strings/BUILD.gn:7:1: Assertion failed.
> > > assert(enable_extensions)
> > > ^-----
> > > See //extensions/browser/BUILD.gn:29:5: which caused the file to be
> included.
> > > "//extensions/strings",
> > > ^---------------------
> > >
> >
> > Right, is_chromecast = true should imply enable_media_router = false.
> >
> > > gn gen out/Debug --args='is_chromecast=true is_debug=true
> > enable_media_router=false' seems to work. I can build it and "something"
> starts
> > up on linux.
> > >
> > > For generic build I also need to do --args="enable_extensions=false
> > enable_media_router=false" since --args="enable_extensions=false" 'gn
> gen'
> will
> > fail. I think making this dependency sounds suboptimal, but since this
> file is
> > not the main conflict in out imports I can accept it in this patch from
> our
> pov.
> >
> >
> > This is not valid, as you can't build media_router without extensions on
> > non-Android platforms.
> >
> > >
> > > is_andorid is not a valid argument for out/Default afaict. But I
> assume that
> > when I do a general chrome build with just "--enable_extensions=false" I
> will
> > also get is_android = false since I runt the binary on my linux host
> after? I
> > verified this works with this patch earlier.
> >
> > I think enable_extensions is forced to false if is_android is true. I'm
> not
> > sure about the converse though.
> >
> > I don't think it's worth blocking this patch if it is solving some
> problem for
> > Chromecast, even if the solution is suboptimal and will need to be undone
> later.
> > I will file an issue to try to more correctly define the conditions for
> > enable_media_router and allow all platforms to build with legal
> combinations
> of
> > these flags.
>
>
> https://codereview.chromium.org/1950003002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[asaka, 2016-05-26 09:19:31]

The CQ bit was checked by asaka@opera.com

[commit-bot: I haz the power, 2016-05-26 09:19:42]

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

[asaka, 2016-05-26 09:20:02]

Thanks! Commiting...

On 2016/05/25 15:50:01, mark a. foltz wrote:
> It's okay to submit as is.
> On May 25, 2016 12:41 AM, <mailto:asaka@opera.com> wrote:
> 
> > Sorry, did not catch the conclusion here :)
> >
> > Do I revert the changes in this file and disable media_router for
> > chromecast in
> > features.gni to allow it to build, or do I leave it as is?
> >
> > (actually, if I do revert, should features.gni rather have something like
> > 'enable_media_router = !is_ios && (enable_extensions || is_android)') ?
> >
> > On 2016/05/25 01:23:44, mark a. foltz wrote:
> > > LGTM
> > >
> > > On 2016/05/24 at 10:57:32, asaka wrote:
> > > >
> > >
> >
> >
>
https://codereview.chromium.org/1950003002/diff/40001/chrome/browser/media/ro...
> > > > File chrome/browser/media/router/BUILD.gn (right):
> > > >
> > > >
> > >
> >
> >
>
https://codereview.chromium.org/1950003002/diff/40001/chrome/browser/media/ro...
> > > > chrome/browser/media/router/BUILD.gn:32: if (enable_extensions) {
> > > > On 2016/05/20 17:48:21, mark a. foltz wrote:
> > > > > On 2016/05/20 at 05:59:41, asaka wrote:
> > > > > > On 2016/05/19 20:00:39, mark a. foltz wrote:
> > > > > > > Is there now a valid build target with !is_android and
> > > !enable_extensions?
> > > > > > > media_router_non_android_sources won't compile without these
> > deps.
> > > > > >
> > > > > > Hi!
> > > > > >
> > > > > > I think chromecast may end up as that with this patch? The linux
> > build
> > > worked
> > > > > for me on my local build from your repository. (gn gen out/Debug
> > > > > --args='is_chromecast=true is_debug=true'; ninja -C out/Debug
> > cast_shell)
> > > > > >
> > > > > > What do you think?
> > > > >
> > > > > I think this works because //chromecast is prevented from depending
> > on
> > > //chrome,
> > > > > so cast_shell doesn't depend on this target anyway. (You can double
> > check
> > > with
> > > > > gn.)
> > > > >
> > > > > The right fix would probably be to add more conditions to
> > > enable_media_router to
> > > > > exclude is_chromecast, and ensure that enable_extensions is set on
> > the
> > > > > appropriate platforms. We can make that fix in a separate patch.
> > > > >
> > > > > As is, this change doesn't make a lot of sense in isolation. For
> > example
> > > try
> > > > > building chrome or unit_tests with is_android=false
> > enable_extensions=false.
> > > I
> > > > > recommend reverting this file.
> > > >
> > > > Reverting this change only will not build:
> > > >
> > > > $ gn gen out/Debug --args='is_chromecast=true is_debug=true'
> > > > ERROR at //extensions/strings/BUILD.gn:7:1: Assertion failed.
> > > > assert(enable_extensions)
> > > > ^-----
> > > > See //extensions/browser/BUILD.gn:29:5: which caused the file to be
> > included.
> > > > "//extensions/strings",
> > > > ^---------------------
> > > >
> > >
> > > Right, is_chromecast = true should imply enable_media_router = false.
> > >
> > > > gn gen out/Debug --args='is_chromecast=true is_debug=true
> > > enable_media_router=false' seems to work. I can build it and "something"
> > starts
> > > up on linux.
> > > >
> > > > For generic build I also need to do --args="enable_extensions=false
> > > enable_media_router=false" since --args="enable_extensions=false" 'gn
> > gen'
> > will
> > > fail. I think making this dependency sounds suboptimal, but since this
> > file is
> > > not the main conflict in out imports I can accept it in this patch from
> > our
> > pov.
> > >
> > >
> > > This is not valid, as you can't build media_router without extensions on
> > > non-Android platforms.
> > >
> > > >
> > > > is_andorid is not a valid argument for out/Default afaict. But I
> > assume that
> > > when I do a general chrome build with just "--enable_extensions=false" I
> > will
> > > also get is_android = false since I runt the binary on my linux host
> > after? I
> > > verified this works with this patch earlier.
> > >
> > > I think enable_extensions is forced to false if is_android is true. I'm
> > not
> > > sure about the converse though.
> > >
> > > I don't think it's worth blocking this patch if it is solving some
> > problem for
> > > Chromecast, even if the solution is suboptimal and will need to be undone
> > later.
> > > I will file an issue to try to more correctly define the conditions for
> > > enable_media_router and allow all platforms to build with legal
> > combinations
> > of
> > > these flags.
> >
> >
> > https://codereview.chromium.org/1950003002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-05-26 09:21:17]

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

[commit-bot: I haz the power, 2016-05-26 09:21:19]

Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[asaka, 2016-05-26 11:53:42]

The CQ bit was checked by asaka@opera.com

[asaka, 2016-05-26 11:53:43]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org,
dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1950003002/#ps60001
(title: "Rebasing on latest master")

[commit-bot: I haz the power, 2016-05-26 11:53:51]

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

[commit-bot: I haz the power, 2016-05-26 11:59:23]

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

[commit-bot: I haz the power, 2016-05-26 11:59:24]

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...)

[asaka, 2016-05-26 13:18:15]

asaka@opera.com changed reviewers:
+ dgozman@chromium.org

[asaka, 2016-05-26 13:18:48]

On 2016/05/26 11:59:24, 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...)

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/devtools/BUILD.gn


@dgozman, do you think you can help out with this review?

[dgozman, 2016-05-26 14:13:56]

lgtm

[asaka, 2016-05-27 05:51:01]

The CQ bit was checked by asaka@opera.com

[commit-bot: I haz the power, 2016-05-27 05:51:22]

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

[commit-bot: I haz the power, 2016-05-27 06:27:58]

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

[commit-bot: I haz the power, 2016-05-27 06:28:00]

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

[fs, 2016-05-27 13:47:32]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-27 13:47:59]

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

[commit-bot: I haz the power, 2016-05-27 15:33:10]

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

[commit-bot: I haz the power, 2016-05-27 15:33:12]

Dry run: This issue passed the CQ dry run.

[asaka, 2016-05-30 06:34:44]

@dpranke, I had to make an additional change in testing/buildbot json files,
could you take a look? Builds look good now!

[fs, 2016-05-30 07:59:33]

fs@opera.com changed reviewers:
- fs@opera.com

[Dirk Pranke, 2016-05-31 20:09:37]

dpranke@chromium.org changed reviewers:
+ ben@chromium.org

[Dirk Pranke, 2016-05-31 20:09:38]

I'm not sure about the mash changes ... I think we need mash even if
use_ash=false.

cc'ing ben@ to check.

https://codereview.chromium.org/1950003002/diff/80001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1950003002/diff/80001/BUILD.gn#newcode294
BUILD.gn:294: }
nit: does this even need to be nested inside use_aura?

[Ben Goodger (Google), 2016-05-31 20:20:13]

https://codereview.chromium.org/1950003002/diff/80001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1950003002/diff/80001/BUILD.gn#newcode662
BUILD.gn:662: deps += [ "//mash:all" ]
This is incorrect.

use_ash is meant to trigger "in-process ash" (i.e. ash run in the browser
process, main thread), a la ChromeOS today.

without use_ash set, it should still be possible to build the ash lib into a
mojo app apart from chrome, and run chrome in it. (In that out-of-process ash
world, chrome behaves more like the conventional desktop client).

So this target should stay in the deps list it was in originally.

[asaka, 2016-06-03 12:30:59]

The CQ bit was checked by asaka@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-03 12:31:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1950003002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1950003002/100001

[commit-bot: I haz the power, 2016-06-03 12:33:02]

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

[commit-bot: I haz the power, 2016-06-03 12:33:04]

Dry run: Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[asaka, 2016-06-03 13:14:43]

The CQ bit was checked by asaka@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-03 13:14:56]

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

[commit-bot: I haz the power, 2016-06-03 14:35:01]

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

[commit-bot: I haz the power, 2016-06-03 14:35:02]

Dry run: This issue passed the CQ dry run.

[asaka, 2016-06-07 07:56:09]

On 2016/06/03 14:35:02, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

Build bots seem happy now, look good to you @ben?

[asaka, 2016-06-07 07:56:15]

On 2016/06/03 14:35:02, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

Build bots seem happy now, look good to you @ben?

[asaka, 2016-06-10 06:16:48]

On 2016/06/07 07:56:15, asaka wrote:
> On 2016/06/03 14:35:02, commit-bot: I haz the power wrote:
> > Dry run: This issue passed the CQ dry run.
> 
> Build bots seem happy now, look good to you @ben?

@ben *ping*

(I think I need another lgtm here after my recent changes, but can I see that in
the web ui somehow or only when I do 'commit'?)

[asaka, 2016-06-10 06:31:39]

On 2016/06/10 06:16:48, asaka wrote:
> On 2016/06/07 07:56:15, asaka wrote:
> > On 2016/06/03 14:35:02, commit-bot: I haz the power wrote:
> > > Dry run: This issue passed the CQ dry run.
> > 
> > Build bots seem happy now, look good to you @ben?
> 
> @ben *ping*
> 
> (I think I need another lgtm here after my recent changes, but can I see that
in
> the web ui somehow or only when I do 'commit'?)

@dpranke^

[Dirk Pranke, 2016-06-10 20:49:27]

On 2016/06/10 06:31:39, asaka wrote:
> On 2016/06/10 06:16:48, asaka wrote:
> > On 2016/06/07 07:56:15, asaka wrote:
> > > On 2016/06/03 14:35:02, commit-bot: I haz the power wrote:
> > > > Dry run: This issue passed the CQ dry run.
> > > 
> > > Build bots seem happy now, look good to you @ben?
> > 
> > @ben *ping*
> > 
> > (I think I need another lgtm here after my recent changes, but can I see
that
> in
> > the web ui somehow or only when I do 'commit'?)
> 
> @dpranke^

If you have a green 'chromium_presubmit' build bubble on your current tryjob,
you don't need new LGTMs.

Generally speaking, the code doesn't invalidate previous LGTMs on a CL, so if
you got one on an older
patchset, that's good enough. However, we do expect you to ask for new reviews
if your patch changes
substantially.

So, no, you don't need another lgtm.

[asaka, 2016-06-13 07:05:50]

On 2016/06/10 20:49:27, Dirk Pranke wrote:
> On 2016/06/10 06:31:39, asaka wrote:
> > On 2016/06/10 06:16:48, asaka wrote:
> > > On 2016/06/07 07:56:15, asaka wrote:
> > > > On 2016/06/03 14:35:02, commit-bot: I haz the power wrote:
> > > > > Dry run: This issue passed the CQ dry run.
> > > > 
> > > > Build bots seem happy now, look good to you @ben?
> > > 
> > > @ben *ping*
> > > 
> > > (I think I need another lgtm here after my recent changes, but can I see
> that
> > in
> > > the web ui somehow or only when I do 'commit'?)
> > 
> > @dpranke^
> 
> If you have a green 'chromium_presubmit' build bubble on your current tryjob,
> you don't need new LGTMs.
> 
> Generally speaking, the code doesn't invalidate previous LGTMs on a CL, so if
> you got one on an older
> patchset, that's good enough. However, we do expect you to ask for new reviews
> if your patch changes
> substantially.
> 
> So, no, you don't need another lgtm.

ok, since I reverted some of the changes in BUILD.gn I thought I needed a
re-approval but it's great to hear this is good to go! (I'll just do another
rebase before pushing the button...)

Thank you for the explanation @dpranke!

[asaka, 2016-06-13 07:51:12]

The CQ bit was checked by asaka@opera.com

[asaka, 2016-06-13 07:51:13]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org,
dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/1950003002/#ps120001
(title: "rebasing on latest master")

[commit-bot: I haz the power, 2016-06-13 07:51:25]

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

[commit-bot: I haz the power, 2016-06-13 09:02:21]

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

[commit-bot: I haz the power, 2016-06-13 09:02:23]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[asaka, 2016-06-14 13:01:19]

The CQ bit was checked by asaka@opera.com

[commit-bot: I haz the power, 2016-06-14 13:01:41]

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

[commit-bot: I haz the power, 2016-06-14 13:42:21]

Description was changed from

==========
gn BUILD fixes for disabling enable_extensions and use_ash feature flags.

BUG=
==========

to

==========
gn BUILD fixes for disabling enable_extensions and use_ash feature flags.

BUG=
==========

[commit-bot: I haz the power, 2016-06-14 13:42:22]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-14 13:44:05]

Description was changed from

==========
gn BUILD fixes for disabling enable_extensions and use_ash feature flags.

BUG=
==========

to

==========
gn BUILD fixes for disabling enable_extensions and use_ash feature flags.

BUG=

Committed: https://crrev.com/2e75c702de464a5cc1cd2272ef720fbe1068c141
Cr-Commit-Position: refs/heads/master@{#399693}
==========

[commit-bot: I haz the power, 2016-06-14 13:44:07]

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

[James Cook, 2016-06-14 16:24:04]

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

The reason for reverting is: Breaks the chromium.chromiumos gn builder.

Repro:
gn args out/Default

Set to:
is_component_build = true
use_goma = true
target_os = "chromeos"

gn gen reports error:
ERROR Unresolved dependencies.
//:both_gn_and_gyp(//build/toolchain/cros:clang_target)
  needs //extensions/shell:app_shell(//build/toolchain/cros:clang_target)

See
https://build.chromium.org/p/chromium.chromiumos/buildstatus?builder=Linux%20...
.