Disable printing in kernel-driver

Disable printing in kernel-driver

R=scheglov@google.com

Committed: https://github.com/dart-lang/sdk/commit/66b4efaa306e5ed9b55423b884b6378491cf80a2

[Siggi Cherem (dart-lang), 2017-07-14 15:39:17]

sigmund@google.com changed reviewers:
+ ahe@google.com, scheglov@google.com

[Siggi Cherem (dart-lang), 2017-07-14 15:39:18]

Konstantin - this should now make your unit tests clean.

Is the plan to always ignore errors here or to collect them and report them
somehow? Let me know and I'll add a TODO accordingly.

FYI - This will change again when we switch to use ProcessedOptions as the
context (instead of CommandLineOptions).

[scheglov, 2017-07-14 15:49:17]

LGTM to remove printed output, but I don't like globals.

I don't know yet whether we will use reported errors.
It depends on how much analysis that is not performed in Analyzer will be
delegated to Fasta. We will decide later, I guess. For now, implementation will
use just kernels, and do all analysis in Analyzer, so ignoring errors is the
way.

https://codereview.chromium.org/2980093002/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental/kernel_driver.dart (right):

https://codereview.chromium.org/2980093002/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental/kernel_driver.dart:152: return await
CompilerCommandLine.withGlobalOptions("", [""],
The name CompilerCommandLine does not make sense in KernelDriver.
It not a command line compiler, and Fasta is not a command line compiler.

And in general using global state smells.
This in particular means that we cannot have parallel execution of
KernelDriver(s).
But we might have to, because we create AnalysisDriver (and so will
KernelDriver) for each project in Analysis Server. And because Fasta is
asynchronous, we cannot guarantee that there wouldn't be execution flow switch
from one driver to another.

[Siggi Cherem (dart-lang), 2017-07-14 16:34:04]

thanks

https://codereview.chromium.org/2980093002/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental/kernel_driver.dart (right):

https://codereview.chromium.org/2980093002/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental/kernel_driver.dart:152: return await
CompilerCommandLine.withGlobalOptions("", [""],
On 2017/07/14 15:49:17, scheglov wrote:
> The name CompilerCommandLine does not make sense in KernelDriver.
> It not a command line compiler, and Fasta is not a command line compiler.
> 
Completely agree - this is exactly what I'm working on fixing next: when I say I
want to unify ProcessedOptions and CompilerContext is also to get rid of
CommandLine* objects.

> And in general using global state smells.
> This in particular means that we cannot have parallel execution of
> KernelDriver(s).
> But we might have to, because we create AnalysisDriver (and so will
> KernelDriver) for each project in Analysis Server. And because Fasta is
> asynchronous, we cannot guarantee that there wouldn't be execution flow switch
> from one driver to another.

Note that this is actually a misnomer here. Internally this is using a
zone-scope, everything within this call (including async calls) are captured by
a zone that contains the context. It is equivalent to adding a Context argument
and passing it around everywhere.

Today there is a default context, and that's why we didn't notice this so far.
In my refactor I'll get rid of the default context as well, so we know when we
are invoking fasta without a required context (which would be equivalent to not
passing an argument).

To support parallel calls to fasta, that would be completely fine. We can create
and store the context objects separately and reuse them if we are invoking fasta
twice on the same context. If you need to use run fasta with different context
options in parallel, that would also work: each call will have it's own zone
with its own context in scope.

Hope this addresses your concerns. Happy to chat more over VC about this.

[Siggi Cherem (dart-lang), 2017-07-14 16:47:13]

Description was changed from

==========
Disable printing in kernel-driver
==========

to

==========
Disable printing in kernel-driver

R=scheglov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/66b4efaa306e5ed9b55423b884b6378491cf80a2
==========

[Siggi Cherem (dart-lang), 2017-07-14 16:47:14]

Committed patchset #1 (id:1) manually as
66b4efaa306e5ed9b55423b884b6378491cf80a2 (presubmit successful).

[scheglov, 2017-07-14 17:00:57]

On 2017/07/14 16:34:04, Siggi Cherem (dart-lang) wrote:
> thanks
> 
>
https://codereview.chromium.org/2980093002/diff/1/pkg/front_end/lib/src/incre...
> File pkg/front_end/lib/src/incremental/kernel_driver.dart (right):
> 
>
https://codereview.chromium.org/2980093002/diff/1/pkg/front_end/lib/src/incre...
> pkg/front_end/lib/src/incremental/kernel_driver.dart:152: return await
> CompilerCommandLine.withGlobalOptions("", [""],
> On 2017/07/14 15:49:17, scheglov wrote:
> > The name CompilerCommandLine does not make sense in KernelDriver.
> > It not a command line compiler, and Fasta is not a command line compiler.
> > 
> Completely agree - this is exactly what I'm working on fixing next: when I say
I
> want to unify ProcessedOptions and CompilerContext is also to get rid of
> CommandLine* objects.
> 
> > And in general using global state smells.
> > This in particular means that we cannot have parallel execution of
> > KernelDriver(s).
> > But we might have to, because we create AnalysisDriver (and so will
> > KernelDriver) for each project in Analysis Server. And because Fasta is
> > asynchronous, we cannot guarantee that there wouldn't be execution flow
switch
> > from one driver to another.
> 
> Note that this is actually a misnomer here. Internally this is using a
> zone-scope, everything within this call (including async calls) are captured
by
> a zone that contains the context. It is equivalent to adding a Context
argument
> and passing it around everywhere.
> 
> Today there is a default context, and that's why we didn't notice this so far.
> In my refactor I'll get rid of the default context as well, so we know when we
> are invoking fasta without a required context (which would be equivalent to
not
> passing an argument).
> 
> To support parallel calls to fasta, that would be completely fine. We can
create
> and store the context objects separately and reuse them if we are invoking
fasta
> twice on the same context. If you need to use run fasta with different context
> options in parallel, that would also work: each call will have it's own zone
> with its own context in scope.
> 
> Hope this addresses your concerns. Happy to chat more over VC about this.

Yes, thank you for the explanation!