Replace page_runner.py --output (and -o) flag with --output-dir

Kill the page_runner.py --output (and -o) flag and use --output-dir instead.
This makes some sense, since as we potentially output more and more files
(instead of a single results.html), we want to store them all in the same
location.

SHERIFFS: If error "This flag is deprecated. Please use --output-dir instead."
arises, this patch can be safely reverted.

Original discussion at https://codereview.chromium.org/616063004/

BUG=

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

[ariblue, 2014-10-15 02:01:42]

ariblue@google.com changed reviewers:
+ dtu@chromium.org, nednguyen@google.com, tonyg@chromium.org

[ariblue, 2014-10-15 02:01:42]

A quick follow up to tonyg loving it.

A few open questions:
(1) What should the default output dir be? Profilers created a temporary
directory, results used util.GetBaseDir(), and profile_generator.py required one
to be specified. Currently this update uses util.GetBaseDir() as the default.

(2) Do we want to throw a 'this is deprecated' error for --output or just get
rid of it? 

(3) Are there any places where --output or -o are being used in the code
somewhere that this patch would need to update?

[nednguyen, 2014-10-15 02:31:34]

On 2014/10/15 02:01:42, ariblue wrote:
> A quick follow up to tonyg loving it.
> 
> A few open questions:
> (1) What should the default output dir be? Profilers created a temporary
> directory, results used util.GetBaseDir(), and profile_generator.py required
one
> to be specified. Currently this update uses util.GetBaseDir() as the default.

util.GetBaseDir() as default seems to be the safest choice. I think there is no
bot that consumes artifacts produced by profilers.

> 
> (2) Do we want to throw a 'this is deprecated' error for --output or just get
> rid of it? 
> 
> (3) Are there any places where --output or -o are being used in the code
> somewhere that this patch would need to update?

[nednguyen, 2014-10-15 02:31:53]

https://codereview.chromium.org/651383002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/results/results_options.py (right):

https://codereview.chromium.org/651383002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/results_options.py:58: parser.error('This flag
is deprecated. Please use --output-dir instead.')
+1. Can you add this to your description?

SHERIFFS: If error "This flag is deprecated. Please use --output-dir instead." 
arises, this patch can be safely reverted.

[ariblue, 2014-10-15 18:28:17]

On 2014/10/15 02:31:53, nednguyen wrote:
>
https://codereview.chromium.org/651383002/diff/40001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/results/results_options.py (right):
> 
>
https://codereview.chromium.org/651383002/diff/40001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/results/results_options.py:58: parser.error('This
flag
> is deprecated. Please use --output-dir instead.')
> +1. Can you add this to your description?
> 
> SHERIFFS: If error "This flag is deprecated. Please use --output-dir instead."

> arises, this patch can be safely reverted.

Done. I've added the message to the sheriffs.

I also notice one additional issue. Previously the --output-file flag was used
as a way to specify "use an output file rather than stdout" for things like the
buildbot output format. How do we want to handle this case? A couple possible
options:
  (1) Add a new flag? This seems to defeat the purpose of killing off this flag.
  (2) Use stdout unless output-dir is specified, and then always create a
results.{html,json,buildbot,...} file?
  (3) Something else?

[ariblue, 2014-10-15 22:42:22]

On 2014/10/15 18:28:17, ariblue wrote:
> On 2014/10/15 02:31:53, nednguyen wrote:
> >
>
https://codereview.chromium.org/651383002/diff/40001/tools/telemetry/telemetr...
> > File tools/telemetry/telemetry/results/results_options.py (right):
> > 
> >
>
https://codereview.chromium.org/651383002/diff/40001/tools/telemetry/telemetr...
> > tools/telemetry/telemetry/results/results_options.py:58: parser.error('This
> flag
> > is deprecated. Please use --output-dir instead.')
> > +1. Can you add this to your description?
> > 
> > SHERIFFS: If error "This flag is deprecated. Please use --output-dir
instead."
> 
> > arises, this patch can be safely reverted.
> 
> Done. I've added the message to the sheriffs.
> 
> I also notice one additional issue. Previously the --output-file flag was used
> as a way to specify "use an output file rather than stdout" for things like
the
> buildbot output format. How do we want to handle this case? A couple possible
> options:
>   (1) Add a new flag? This seems to defeat the purpose of killing off this
flag.
>   (2) Use stdout unless output-dir is specified, and then always create a
> results.{html,json,buildbot,...} file?
>   (3) Something else?

Okay, so sending buildbot output to a file may not be needed*. I've updated the
one unittest that was doing this to capture the stdout rather than creating a
file.


* Feel free to correct me if this is terribly wrong.

[nednguyen, 2014-10-15 23:21:40]

nednguyen@google.com changed reviewers:
+ sullivan@chromium.org

[nednguyen, 2014-10-15 23:24:52]

On 2014/10/15 22:42:22, ariblue wrote:
> On 2014/10/15 18:28:17, ariblue wrote:
> > On 2014/10/15 02:31:53, nednguyen wrote:
> > >
> >
>
https://codereview.chromium.org/651383002/diff/40001/tools/telemetry/telemetr...
> > > File tools/telemetry/telemetry/results/results_options.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/651383002/diff/40001/tools/telemetry/telemetr...
> > > tools/telemetry/telemetry/results/results_options.py:58:
parser.error('This
> > flag
> > > is deprecated. Please use --output-dir instead.')
> > > +1. Can you add this to your description?
> > > 
> > > SHERIFFS: If error "This flag is deprecated. Please use --output-dir
> instead."
> > 
> > > arises, this patch can be safely reverted.
> > 
> > Done. I've added the message to the sheriffs.
> > 
> > I also notice one additional issue. Previously the --output-file flag was
used
> > as a way to specify "use an output file rather than stdout" for things like
> the
> > buildbot output format. How do we want to handle this case? A couple
possible
> > options:
> >   (1) Add a new flag? This seems to defeat the purpose of killing off this
> flag.
> >   (2) Use stdout unless output-dir is specified, and then always create a
> > results.{html,json,buildbot,...} file?
> >   (3) Something else?
> 
> Okay, so sending buildbot output to a file may not be needed*. I've updated
the
> one unittest that was doing this to capture the stdout rather than creating a
> file.
> 
> 
> * Feel free to correct me if this is terribly wrong.

Lgtm.

[sullivan, 2014-10-16 15:28:40]

> > I also notice one additional issue. Previously the --output-file flag was
used
> > as a way to specify "use an output file rather than stdout" for things like
> the
> > buildbot output format. How do we want to handle this case? A couple
possible
> > options:
> >   (1) Add a new flag? This seems to defeat the purpose of killing off this
> flag.
> >   (2) Use stdout unless output-dir is specified, and then always create a
> > results.{html,json,buildbot,...} file?
> >   (3) Something else?
> 
> Okay, so sending buildbot output to a file may not be needed*. I've updated
the
> one unittest that was doing this to capture the stdout rather than creating a
> file.
> 
> 
> * Feel free to correct me if this is terribly wrong.

There are several different code paths that use buildbot output (chromium.perf,
tryserver.chromium.perf, and an internal one), but as far as I know they all
read buildbot output from stdout. So sending buildbot output to a file should
not be needed.

Ideally, they would all read chartjson output from a file instead, but there are
a lot of blockers (see http://crbug.com/420044). IIUC, after this CL we'd also
need to add an additional blocking bug to read chartjson from --output-dir
instead of --output? Can you file it?

[ariblue, 2014-10-16 17:54:34]

On 2014/10/16 15:28:40, sullivan wrote:
> > > I also notice one additional issue. Previously the --output-file flag was
> used
> > > as a way to specify "use an output file rather than stdout" for things
like
> > the
> > > buildbot output format. How do we want to handle this case? A couple
> possible
> > > options:
> > >   (1) Add a new flag? This seems to defeat the purpose of killing off this
> > flag.
> > >   (2) Use stdout unless output-dir is specified, and then always create a
> > > results.{html,json,buildbot,...} file?
> > >   (3) Something else?
> > 
> > Okay, so sending buildbot output to a file may not be needed*. I've updated
> the
> > one unittest that was doing this to capture the stdout rather than creating
a
> > file.
> > 
> > 
> > * Feel free to correct me if this is terribly wrong.
> 
> There are several different code paths that use buildbot output
(chromium.perf,
> tryserver.chromium.perf, and an internal one), but as far as I know they all
> read buildbot output from stdout. So sending buildbot output to a file should
> not be needed.
> 
> Ideally, they would all read chartjson output from a file instead, but there
are
> a lot of blockers (see http://crbug.com/420044). IIUC, after this CL we'd also
> need to add an additional blocking bug to read chartjson from --output-dir
> instead of --output? Can you file it?

Sounds good. I've created a bug (crbug.com/424266) for this.

[nednguyen, 2014-10-20 17:12:54]

The CQ bit was checked by nednguyen@google.com

[ariblue, 2014-10-20 17:13:29]

The CQ bit was unchecked by ariblue@google.com

[nednguyen, 2014-10-20 17:16:02]

https://codereview.chromium.org/651383002/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/results/results_options.py (right):

https://codereview.chromium.org/651383002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/results_options.py:75: if output_format !=
'html' and output_format != 'json':
can you change this so that we will only return sys.stdout in case of buildbot?

[ariblue, 2014-10-20 17:34:57]

On 2014/10/20 17:16:02, nednguyen wrote:
>
https://codereview.chromium.org/651383002/diff/80001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/results/results_options.py (right):
> 
>
https://codereview.chromium.org/651383002/diff/80001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/results/results_options.py:75: if output_format !=
> 'html' and output_format != 'json':
> can you change this so that we will only return sys.stdout in case of
buildbot?

PTAL. let me know if the _GetOutputFilename stuff is overkill.

[nednguyen, 2014-10-20 17:41:23]

nednguyen@google.com changed reviewers:
+ vogelheim@chromium.org

[nednguyen, 2014-10-20 17:41:24]

LGTM

+vogelheim FYI

[nednguyen, 2014-10-20 17:43:23]

https://codereview.chromium.org/651383002/diff/120001/tools/telemetry/telemet...
File tools/telemetry/telemetry/results/results_options.py (right):

https://codereview.chromium.org/651383002/diff/120001/tools/telemetry/telemet...
tools/telemetry/telemetry/results/results_options.py:86: def
_GetOutputFilename(output_format):
I would just make a dict that maps output_formats to file names just to be
explicit.

[ariblue, 2014-10-20 17:50:48]

https://codereview.chromium.org/651383002/diff/120001/tools/telemetry/telemet...
File tools/telemetry/telemetry/results/results_options.py (right):

https://codereview.chromium.org/651383002/diff/120001/tools/telemetry/telemet...
tools/telemetry/telemetry/results/results_options.py:86: def
_GetOutputFilename(output_format):
On 2014/10/20 17:43:23, nednguyen wrote:
> I would just make a dict that maps output_formats to file names just to be
> explicit. 

Done.

[nednguyen, 2014-10-20 17:53:07]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2014-10-20 17:53:53]

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

[commit-bot: I haz the power, 2014-10-20 19:18:07]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-10-20 19:18:48]

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