Support explicit pools in actions

Support explicit pools in actions

This change allows explicitly specifying pools for actions.
Furthermore, it is also possible to specify a default pool for
actions as part of the toolchain. Pools can be also defined as
console ones to emulate the original console attribute behavior.

BUG=635308
Review-Url: https://codereview.chromium.org/2926013002
Cr-Commit-Position: refs/heads/master@{#483252}
Committed: https://chromium.googlesource.com/chromium/src/+/43ee2b1163bdee1094aac234d25f52aab9decb62

[Petr Hosek, 2017-06-07 03:14:59]

Description was changed from

==========
Support explicit pools in actions

This change allows explicitly specifying pools for actions.
Furthermore, it is also possible to specify a default pool for
actions as part of the toolchain. Pools can be also defined as
console ones to emulate the original console attribute behavior.

BUG=635308
==========

to

==========
Support explicit pools in actions

This change allows explicitly specifying pools for actions.
Furthermore, it is also possible to specify a default pool for
actions as part of the toolchain. Pools can be also defined as
console ones to emulate the original console attribute behavior.

BUG=635308
==========

[Petr Hosek, 2017-06-07 03:15:00]

phosek@chromium.org changed reviewers:
+ brettw@chromium.org, dpranke@chromium.org

[Petr Hosek, 2017-06-07 03:16:34]

PTAL, at the moment it is possible to set the pool as being the console one
(instead of specifying its depth) as

pool("mypool") {
  console = true
}

I'm not sure whether that's the best choice though, maybe it'd be easier to just
continue supporting console attribute on actions (also for backwards
compatibility).

[brettw, 2017-06-08 18:36:20]

+Some Opera folks. I think Opera is the only one using "console=true". I believe
this will work for their use-case, but they should also have a heads-up for when
this is landed that they'll need to update their code.

https://codereview.chromium.org/2926013002/diff/1/tools/gn/function_toolchain.cc
File tools/gn/function_toolchain.cc (right):

https://codereview.chromium.org/2926013002/diff/1/tools/gn/function_toolchain...
tools/gn/function_toolchain.cc:531: Other tools:
I think you want "action" here.

https://codereview.chromium.org/2926013002/diff/1/tools/gn/function_toolchain...
tools/gn/function_toolchain.cc:641: pool [label, optional]
I think this is valid for all tools. Can you add a line here to that effect like
it lists for the other tools?

https://codereview.chromium.org/2926013002/diff/1/tools/gn/function_toolchain...
tools/gn/function_toolchain.cc:712: Valid for: all tools (optional)
This and the next one I think don't apply to action tools. Can you update this
to say "all tools except "action" (optional)"

https://codereview.chromium.org/2926013002/diff/1/tools/gn/functions.cc
File tools/gn/functions.cc (right):

https://codereview.chromium.org/2926013002/diff/1/tools/gn/functions.cc#newco...
tools/gn/functions.cc:736: A pool is referenced by its label just like a target.
Can you have a section here about the meaning of the console variable?

https://codereview.chromium.org/2926013002/diff/1/tools/gn/toolchain.cc
File tools/gn/toolchain.cc (right):

https://codereview.chromium.org/2926013002/diff/1/tools/gn/toolchain.cc#newco...
tools/gn/toolchain.cc:63: if (str == kToolAction)
Can you format this list of conditions (and the next one) consistently? If clang
wants to put them on separate lines, that's OK, though I prefer single-line
otherwise.

https://codereview.chromium.org/2926013002/diff/1/tools/gn/variables.cc
File tools/gn/variables.cc (right):

https://codereview.chromium.org/2926013002/diff/1/tools/gn/variables.cc#newco...
tools/gn/variables.cc:934: R"(console: Run this action in the console pool.
This documentation needs to be updated.

https://codereview.chromium.org/2926013002/diff/1/tools/gn/variables.cc#newco...
tools/gn/variables.cc:1444: const char kPool[] = "pool";
This will cause a problem for help, because "gn help pool" could mean either the
variable or the target type.

We could rename the variable but most things I can think of are worse than just
"pool" (ideas?). We could potentially also special-case this in "gn help" to
have a message about how it could be a function or a variable in an action, then
print out both outputs.

https://codereview.chromium.org/2926013002/diff/1/tools/gn/variables.cc#newco...
tools/gn/variables.cc:1451: action.
Can this add "Pools are defined using the pool() {...} declaration."

[tsniatowski, 2017-06-08 18:42:44]

On 2017/06/08 18:36:20, brettw wrote:
> +Some Opera folks. I think Opera is the only one using "console=true". I
believe
> this will work for their use-case, but they should also have a heads-up for
when
> this is landed that they'll need to update their code.

Thanks for the heads-up; to be fair console=true ended up far less useful than
thought initially. This thing here though is something else, looks much more
practical and will solve some long-standing issues (like heavy actions bogging
down dev machines).

[Petr Hosek, 2017-06-09 03:24:33]

Is the console pool actually needed? I haven't found any uses in Chromium, and
if Opera folks would be happy with this solution, maybe we could consider
dropping it altogether?

https://codereview.chromium.org/2926013002/diff/1/tools/gn/function_toolchain.cc
File tools/gn/function_toolchain.cc (right):

https://codereview.chromium.org/2926013002/diff/1/tools/gn/function_toolchain...
tools/gn/function_toolchain.cc:531: Other tools:
On 2017/06/08 18:36:20, brettw wrote:
> I think you want "action" here.

Done.

https://codereview.chromium.org/2926013002/diff/1/tools/gn/function_toolchain...
tools/gn/function_toolchain.cc:641: pool [label, optional]
On 2017/06/08 18:36:19, brettw wrote:
> I think this is valid for all tools. Can you add a line here to that effect
like
> it lists for the other tools?

Done.

https://codereview.chromium.org/2926013002/diff/1/tools/gn/function_toolchain...
tools/gn/function_toolchain.cc:712: Valid for: all tools (optional)
On 2017/06/08 18:36:20, brettw wrote:
> This and the next one I think don't apply to action tools. Can you update this
> to say "all tools except "action" (optional)"

Done.

https://codereview.chromium.org/2926013002/diff/1/tools/gn/toolchain.cc
File tools/gn/toolchain.cc (right):

https://codereview.chromium.org/2926013002/diff/1/tools/gn/toolchain.cc#newco...
tools/gn/toolchain.cc:63: if (str == kToolAction)
On 2017/06/08 18:36:20, brettw wrote:
> Can you format this list of conditions (and the next one) consistently? If
clang
> wants to put them on separate lines, that's OK, though I prefer single-line
> otherwise.

Done, this was done by clang-format.

https://codereview.chromium.org/2926013002/diff/1/tools/gn/variables.cc
File tools/gn/variables.cc (right):

https://codereview.chromium.org/2926013002/diff/1/tools/gn/variables.cc#newco...
tools/gn/variables.cc:1451: action.
On 2017/06/08 18:36:20, brettw wrote:
> Can this add "Pools are defined using the pool() {...} declaration."

Done.

[Petr Hosek, 2017-06-13 02:16:58]

https://codereview.chromium.org/2926013002/diff/1/tools/gn/functions.cc
File tools/gn/functions.cc (right):

https://codereview.chromium.org/2926013002/diff/1/tools/gn/functions.cc#newco...
tools/gn/functions.cc:736: A pool is referenced by its label just like a target.
On 2017/06/08 18:36:20, brettw wrote:
> Can you have a section here about the meaning of the console variable?

Done.

https://codereview.chromium.org/2926013002/diff/1/tools/gn/variables.cc
File tools/gn/variables.cc (right):

https://codereview.chromium.org/2926013002/diff/1/tools/gn/variables.cc#newco...
tools/gn/variables.cc:934: R"(console: Run this action in the console pool.
On 2017/06/08 18:36:20, brettw wrote:
> This documentation needs to be updated.

Done.

[brettw, 2017-06-13 21:59:16]

If Opera's not using the console pool, I'm supportive of just deleting it.

https://codereview.chromium.org/2926013002/diff/40001/tools/gn/toolchain.cc
File tools/gn/toolchain.cc (right):

https://codereview.chromium.org/2926013002/diff/40001/tools/gn/toolchain.cc#n...
tools/gn/toolchain.cc:85: return kToolAction;
Consistency in this switch.

https://codereview.chromium.org/2926013002/diff/40001/tools/gn/variables.cc
File tools/gn/variables.cc (right):

https://codereview.chromium.org/2926013002/diff/40001/tools/gn/variables.cc#n...
tools/gn/variables.cc:1444: const char kPool[] = "pool";
Did you have any thouhgs on how to fix the "gn help" issue for this?

[Petr Hosek, 2017-06-17 02:44:25]

https://codereview.chromium.org/2926013002/diff/40001/tools/gn/toolchain.cc
File tools/gn/toolchain.cc (right):

https://codereview.chromium.org/2926013002/diff/40001/tools/gn/toolchain.cc#n...
tools/gn/toolchain.cc:85: return kToolAction;
On 2017/06/13 21:59:16, brettw wrote:
> Consistency in this switch.

Done.

https://codereview.chromium.org/2926013002/diff/40001/tools/gn/variables.cc
File tools/gn/variables.cc (right):

https://codereview.chromium.org/2926013002/diff/40001/tools/gn/variables.cc#n...
tools/gn/variables.cc:1444: const char kPool[] = "pool";
On 2017/06/13 21:59:16, brettw wrote:
> Did you have any thouhgs on how to fix the "gn help" issue for this?

I tried to modify help command to handle the case where the same topic is
defined for functions and variables and it doesn't look too terrible, what do
you think?

[Petr Hosek, 2017-06-20 01:20:10]

@tsniatowski would it be fine with Opera if we remove console altogether? Would
this be a sufficient replacement?

[Petr Hosek, 2017-06-21 16:23:47]

On 2017/06/20 01:20:10, Petr Hosek wrote:
> @tsniatowski would it be fine with Opera if we remove console altogether?
Would
> this be a sufficient replacement?

@tsniatowski, @pmajewski, @ollel ping?

[tsniatowski, 2017-06-21 16:56:18]

On 2017/06/21 16:23:47, Petr Hosek wrote:
> On 2017/06/20 01:20:10, Petr Hosek wrote:
> > @tsniatowski would it be fine with Opera if we remove console altogether?
> Would
> > this be a sufficient replacement?
> 
> @tsniatowski, @pmajewski, @ollel ping?

Sorry, din't get the ping mail for the original question.

Anyway it definitely wouldn't be a problem for us, we don't have any uses where
the console pool is relevant (used for unbuffered status output in one place,
not a big deal), so we'll just remove the use if it stops working.

I wouldn't mind having it in GN for completion's sake -- it's a ninja feature,
feels good to be able to do use ninja features from gn. Not hugely important
though. Definitely don't want both console = true in an action and a separate
way of doing the same in a pool. Can you just make the "console" pool name be
magic and special, like in ninja?

[Petr Hosek, 2017-06-21 19:53:22]

On 2017/06/21 16:56:18, tsniatowski wrote:
> On 2017/06/21 16:23:47, Petr Hosek wrote:
> > On 2017/06/20 01:20:10, Petr Hosek wrote:
> > > @tsniatowski would it be fine with Opera if we remove console altogether?
> > Would
> > > this be a sufficient replacement?
> > 
> > @tsniatowski, @pmajewski, @ollel ping?
> 
> Sorry, din't get the ping mail for the original question.
> 
> Anyway it definitely wouldn't be a problem for us, we don't have any uses
where
> the console pool is relevant (used for unbuffered status output in one place,
> not a big deal), so we'll just remove the use if it stops working.
> 
> I wouldn't mind having it in GN for completion's sake -- it's a ninja feature,
> feels good to be able to do use ninja features from gn. Not hugely important
> though. Definitely don't want both console = true in an action and a separate
> way of doing the same in a pool. Can you just make the "console" pool name be
> magic and special, like in ninja?

Done, there's now a predefined console pool object and pool function no longer
takes console as an attribute. @brettw can you please take a look again if this
makes sense to you?

[Olle Liljenzin, 2017-06-22 07:41:39]

On 2017/06/21 16:23:47, Petr Hosek wrote:
> On 2017/06/20 01:20:10, Petr Hosek wrote:
> > @tsniatowski would it be fine with Opera if we remove console altogether?
> Would
> > this be a sufficient replacement?
> 
> @tsniatowski, @pmajewski, @ollel ping?

Sorry for not replying faster. I can only speak for the android team, but we are
fine with removing the console.

Thank for asking anyway, it is appreciated.

[Petr Hosek, 2017-06-28 18:46:07]

ping

[brettw, 2017-06-28 21:23:27]

Looks close but I have 2 nontrivial suggestions.

https://codereview.chromium.org/2926013002/diff/120001/tools/gn/builder.cc
File tools/gn/builder.cc (right):

https://codereview.chromium.org/2926013002/diff/120001/tools/gn/builder.cc#ne...
tools/gn/builder.cc:230: !AddDeps(record, target->public_configs(), err) ||
You need to add a line here to add the pool to the dependency graph. Otherwise
the definition of the pool won't block the resolution of the target.

https://codereview.chromium.org/2926013002/diff/120001/tools/gn/loader.cc
File tools/gn/loader.cc (right):

https://codereview.chromium.org/2926013002/diff/120001/tools/gn/loader.cc#new...
tools/gn/loader.cc:345: // Create the predefined console pool object.
This seems weird to me. It looks like this will be creating a pool in Chrome
named "//build/config:console". I don't know how one is supposed to know about
this.

From what Opera said we don't need this. Let's just delete the console support.
If we need console pools, I'd rather add a "console" variable to the pool
definition and map anything with this flag to the builtin one.

https://codereview.chromium.org/2926013002/diff/120001/tools/gn/variables.cc
File tools/gn/variables.cc (right):

https://codereview.chromium.org/2926013002/diff/120001/tools/gn/variables.cc#...
tools/gn/variables.cc:930: const char kConsole[] = "console";
I think this can be deleted.

[Petr Hosek, 2017-06-28 23:42:32]

https://codereview.chromium.org/2926013002/diff/120001/tools/gn/builder.cc
File tools/gn/builder.cc (right):

https://codereview.chromium.org/2926013002/diff/120001/tools/gn/builder.cc#ne...
tools/gn/builder.cc:230: !AddDeps(record, target->public_configs(), err) ||
On 2017/06/28 21:23:27, brettw wrote:
> You need to add a line here to add the pool to the dependency graph. Otherwise
> the definition of the pool won't block the resolution of the target.

Done.

https://codereview.chromium.org/2926013002/diff/120001/tools/gn/loader.cc
File tools/gn/loader.cc (right):

https://codereview.chromium.org/2926013002/diff/120001/tools/gn/loader.cc#new...
tools/gn/loader.cc:345: // Create the predefined console pool object.
On 2017/06/28 21:23:27, brettw wrote:
> This seems weird to me. It looks like this will be creating a pool in Chrome
> named "//build/config:console". I don't know how one is supposed to know about
> this.
> 
> From what Opera said we don't need this. Let's just delete the console
support.
> If we need console pools, I'd rather add a "console" variable to the pool
> definition and map anything with this flag to the builtin one.

It has "//:console" label but it's accessible through a prebuilt console
variable, similarly to e.g. current_os or current_cpu, so you can do pool =
console. I agree that it's probably easier to remove this altogether if it's no
longer needed.

https://codereview.chromium.org/2926013002/diff/120001/tools/gn/variables.cc
File tools/gn/variables.cc (right):

https://codereview.chromium.org/2926013002/diff/120001/tools/gn/variables.cc#...
tools/gn/variables.cc:930: const char kConsole[] = "console";
On 2017/06/28 21:23:27, brettw wrote:
> I think this can be deleted.

Done.

[brettw, 2017-06-29 00:03:31]

lgtm

[Petr Hosek, 2017-06-29 00:08:54]

The CQ bit was checked by phosek@chromium.org

[commit-bot: I haz the power, 2017-06-29 00:09:22]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-29 01:56:03]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1498694934041530,
"parent_rev": "6ee031b5c713ad27f3096dd0f5de7e6f9b7a8cc8", "commit_rev":
"43ee2b1163bdee1094aac234d25f52aab9decb62"}

[commit-bot: I haz the power, 2017-06-29 01:56:18]

Description was changed from

==========
Support explicit pools in actions

This change allows explicitly specifying pools for actions.
Furthermore, it is also possible to specify a default pool for
actions as part of the toolchain. Pools can be also defined as
console ones to emulate the original console attribute behavior.

BUG=635308
==========

to

==========
Support explicit pools in actions

This change allows explicitly specifying pools for actions.
Furthermore, it is also possible to specify a default pool for
actions as part of the toolchain. Pools can be also defined as
console ones to emulate the original console attribute behavior.

BUG=635308

Review-Url: https://codereview.chromium.org/2926013002
Cr-Commit-Position: refs/heads/master@{#483252}
Committed:
https://chromium.googlesource.com/chromium/src/+/43ee2b1163bdee1094aac234d25f...
==========

[commit-bot: I haz the power, 2017-06-29 01:56:19]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/43ee2b1163bdee1094aac234d25f...