Additional tools for gardening.

Additional tools for gardening.
- multitest will generate a specific case of a multitest file
- luci_api is the start of a project, to make an api to ease the life     of the gardener. At this point, it can answer, given a commit, which     builds on bots does that commit belong to.

R=johnniwinther@google.com

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

[mkroghj, 2017-08-23 11:13:30]

mkroghj@google.com changed reviewers:
+ johnniwinther@google.com, whesse@google.com

[mkroghj, 2017-08-23 11:13:30]

ptal

[Johnni Winther, 2017-08-23 12:40:48]

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
File tools/gardening_tools/base_lib/lib/base_lib.dart (right):

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
tools/gardening_tools/base_lib/lib/base_lib.dart:1: library base_lib;
Just add all tools to the tools/gardening package

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
File tools/gardening_tools/base_lib/lib/src/cache.dart (right):

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
tools/gardening_tools/base_lib/lib/src/cache.dart:10: typedef Future<String>
Call();
Rename to [ReadResultFunction] and variables of this type to [readResult].

Maybe [readData], [fetchData] or similar.

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
tools/gardening_tools/base_lib/lib/src/cache.dart:12: typedef WithCache
PerformWithCache({String overrideKey, Duration duration});
Rename to [CreateCacheFunction] and variables of this type to [createCache].

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
tools/gardening_tools/base_lib/lib/src/cache.dart:16: logger = new
StdOutLogger(Level.warning);
Just write this as:

logger ??= new StdOutLogger(Level.warning);

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
tools/gardening_tools/base_lib/lib/src/cache.dart:37: Try<ReadFromCacheResult>
readResult =
Rename to [cacheResult].

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
tools/gardening_tools/base_lib/lib/src/cache.dart:73: class Cache {
Add a TODO (for me?) to use this instead of the Cache class in  tools/gardening.

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
tools/gardening_tools/base_lib/lib/src/cache.dart:97:
Future<Try<ReadFromCacheResult>> tryRead(String path,
Rename to [read] (since we always just try)

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
tools/gardening_tools/base_lib/lib/src/cache.dart:114: 'File $path was found but
the information is too stale, for the duration: $duration');
Split long strings manual:

logger.debug(
    'File $path was found but the information is too stale, '
    'for the duration: $duration');

Here and elsewhere.

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
tools/gardening_tools/base_lib/lib/src/cache.dart:139: // needs to be done
Write this as

  // TODO(mkroghj): Implement this.

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
tools/gardening_tools/base_lib/lib/src/cache.dart:143: class ReadFromCacheResult
{
Rename to [CacheResult] (the correctly name reads weirdly - and so did that
sentence!)

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
tools/gardening_tools/base_lib/lib/src/cache.dart:144: bool hasResult;
Make the fields final.

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
tools/gardening_tools/base_lib/lib/src/cache.dart:145: String res;
Rename to [result]. We generally use unabbreviated names - in particular in
interfaces.

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
tools/gardening_tools/base_lib/lib/src/cache.dart:152:
ReadFromCacheResult.result(String result) {
Rename to the no-name constructor and use initializing formals:

  CacheResult(this.result) : hasResult = true;

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
File tools/gardening_tools/base_lib/lib/src/logger.dart (right):

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
tools/gardening_tools/base_lib/lib/src/logger.dart:21: msg += ":
$error\n$stackTrace";
Support nice printing of an error without a stack trace as well.

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
File tools/gardening_tools/base_lib/lib/src/try.dart (right):

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
tools/gardening_tools/base_lib/lib/src/try.dart:43: if (_err != null)
Add curly braces (dangling statements are brittle).

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
tools/gardening_tools/base_lib/lib/src/try.dart:49: bool isError() => _err !=
null;
Make this a getter:

  bool get isError => _error != null;

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
tools/gardening_tools/base_lib/lib/src/try.dart:51: Exception getError() =>
_err;
Ditto.

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
tools/gardening_tools/base_lib/lib/src/try.dart:52: StackTrace getStackTrace()
=> _stackTrace;
Ditto.

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
tools/gardening_tools/base_lib/lib/src/try.dart:54: T get() => _val;
Make this a getter:

  bool get value => _val;

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/l...
File tools/gardening_tools/luci_api/bin/luci_api.dart (right):

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/l...
tools/gardening_tools/luci_api/bin/luci_api.dart:12: abbr: "v", negatable:
false, help: "Print out debugging information")
-> "Print debugging information."

Generally, consistently make the help messages full sentences; start with a
capital letter and end in period.

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/l...
tools/gardening_tools/luci_api/bin/luci_api.dart:20: help: "Use this flag to see
the primary build bots for --client")
-> ... the most recent build bots ...

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/l...
tools/gardening_tools/luci_api/bin/luci_api.dart:27: "use this flag as
--build-bot-details <name> where <name> is the name of the build bot")
-> ... as `--build-bot-details <name>` where <name> ...

Similarly below.

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/l...
tools/gardening_tools/luci_api/bin/luci_api.dart:35: "Fetches all builds for a
specific commit.. Use this flag as --commit-builds <commit-hash> where the
<commit-hash> is the hash of the commit");
-> ... specific commit. Use ...

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/l...
tools/gardening_tools/luci_api/bin/luci_api.dart:68: print("No command found.
Use --help for more information.");
Just print the help here.

[Bill Hesse, 2017-08-23 15:41:25]

I'll look at this again when Johnni's comments are incorporated.

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
File tools/gardening_tools/base_lib/lib/base_lib.dart (right):

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
tools/gardening_tools/base_lib/lib/base_lib.dart:1: library base_lib;
On 2017/08/23 12:40:46, Johnni Winther wrote:
> Just add all tools to the tools/gardening package

Yes, unless these parts are going to be moved independently of each other, or
some subset will be released for public use, I would just put all the libraries
in the same big package.

I don't think relative links should be used in package specs except in
extraordinary circumstances, since packages should be independently versioned,
downloaded, etc.

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
File tools/gardening_tools/base_lib/lib/src/cache.dart (right):

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
tools/gardening_tools/base_lib/lib/src/cache.dart:145: String res;
On 2017/08/23 12:40:47, Johnni Winther wrote:
> Rename to [result]. We generally use unabbreviated names - in particular in
> interfaces.

I agree - alway use full, correctly-spelled words in identifiers.

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
File tools/gardening_tools/base_lib/lib/src/try.dart (right):

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
tools/gardening_tools/base_lib/lib/src/try.dart:30: Future<Try<S>>
bindAsync<S>(Future<S> f(T)) async {
Future itself has an error channel and a value channel, so should an error be
sent on the error channel rather than the error of a Try on the value channel?

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/l...
File tools/gardening_tools/luci_api/lib/src/luci_api.dart (right):

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/l...
tools/gardening_tools/luci_api/lib/src/luci_api.dart:19: LuciApi();
Please contact the makers of the application to see if they have a direct api,
rather than decoding the html.

[mkroghj, 2017-08-25 09:28:28]

ptal

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
File tools/gardening_tools/base_lib/lib/src/try.dart (right):

https://codereview.chromium.org/3005443002/diff/20001/tools/gardening_tools/b...
tools/gardening_tools/base_lib/lib/src/try.dart:30: Future<Try<S>>
bindAsync<S>(Future<S> f(T)) async {
On 2017/08/23 15:41:25, Bill Hesse wrote:
> Future itself has an error channel and a value channel, so should an error be
> sent on the error channel rather than the error of a Try on the value channel?

The idea for bindAsync is to change Try<Future<X>> into Future<Try<X>> and be
able to await the try of an operation. One could argue that one should use the
error channel, which basically means I would get rid of the try/catch around it.
However, if an error now occurs, it will be thrown, so any clients of bindAsync
would have to do their error handling there, and then wrap it in the Try, or
completely get rid of the try. So I think this is what one wants (if they want
to use it :))

[Johnni Winther, 2017-08-28 07:14:04]

lgtm

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/bin/luc...
File tools/gardening/bin/luci_api.dart (right):

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/bin/luc...
tools/gardening/bin/luci_api.dart:1: import 'dart:async';
Just call this file 'luci.dart' - we're using it from the command_line so it is
not really an API.

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/bin/luc...
tools/gardening/bin/luci_api.dart:41: "`--commit-builds <commit-hash>` where the
<commit-hash> is the hash "
Long line.

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/bin/luc...
tools/gardening/bin/luci_api.dart:47: "This tool calls different pages on Luci
and aggregate the information found. "
Long line.

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/bin/luc...
tools/gardening/bin/luci_api.dart:128: print("The buildnumber
${results['build-details']} must be a true integer "
a true integer -> an integer

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/bin/luc...
tools/gardening/bin/luci_api.dart:167: print("${detail.botName}:
#${detail.buildNumber}\t$url");
Long line.

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
File tools/gardening/lib/src/_luci_api.dart (right):

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
tools/gardening/lib/src/_luci_api.dart:1: import 'dart:io';
Remove this file.

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
File tools/gardening/lib/src/luci_api.dart (right):

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
tools/gardening/lib/src/luci_api.dart:1: import 'dart:io';
Add copyright header (for all .dart files):

// Copyright (c) 2017, the Dart project authors.  Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
tools/gardening/lib/src/luci_api.dart:10: // const String BUILD_CHROMIUM_HOST =
"build.chromium.org";
Remove?

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
tools/gardening/lib/src/luci_api.dart:19: class LuciApi {
Just call this [Luci].

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
tools/gardening/lib/src/luci_api.dart:35: ///     <a
href="/buildbot/client.dart/analyze-linux-dev">analyze-linux-dev</a>
Long line.

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
tools/gardening/lib/src/luci_api.dart:58: false; // this is really dirty, but
the structure of
Move the comment about the variable declaration (dartfmt will be happier).

Also, start with a capital letter and end with a period.

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
tools/gardening/lib/src/luci_api.dart:101: /// where the field [amount] is the
number of recent builds to fetch
Add period to end the sentence.

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
tools/gardening/lib/src/luci_api.dart:120:
'${uri.path.toString()}_${botName}_$amount'));
Just ${uri.path} since [path] is already a string.

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
tools/gardening/lib/src/luci_api.dart:130: /// where the field [amount] is the
number of recent builds to fetch
Add period to end the sentence.

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
tools/gardening/lib/src/luci_api.dart:167: // Prpc outputs a prefix to combat
vulnerability
Add period to end the sentence.

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
tools/gardening/lib/src/luci_api.dart:179: BuildDetail getBuildDetailFromJson(
Add documentation.

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
tools/gardening/lib/src/luci_api.dart:230: String url;
Make the fields final.

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
tools/gardening/lib/src/luci_api.dart:240: class BuildDetail {
Add document to the class and all its fields.

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
tools/gardening/lib/src/luci_api.dart:250: List<GitCommit> allChanges;
Add a constructor that takes these as argument and make the fields final.

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
tools/gardening/lib/src/luci_api.dart:270: class BuildStep {
Add document to the class and all its fields.

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
tools/gardening/lib/src/luci_api.dart:280: BuildStep(this.name,
this.description, this.result, this.start, this.end,
Add an empty line before the constructor.

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
tools/gardening/lib/src/luci_api.dart:287: "${result == '[0, []]' ? 'SUCCESS' :
result}: $name - $description ($start, $end)");
Long line.

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
tools/gardening/lib/src/luci_api.dart:295: class BuildLog {
Add document to the class and its fields.

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
tools/gardening/lib/src/luci_api.dart:306: class BuildProperty {
Add document to the class and its fields.

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
tools/gardening/lib/src/luci_api.dart:310: BuildProperty(this.name, this.value,
this.source);
Add an empty line before the constructor.

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
tools/gardening/lib/src/luci_api.dart:329: class GitCommit {
Add document to the class and its fields.

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
tools/gardening/lib/src/luci_api.dart:335: GitCommit(this.revision,
this.commitUrl, this.changedBy, this.comments);
Add an empty line before the constructor.

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
File tools/gardening/lib/src/luci_services.dart (right):

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
tools/gardening/lib/src/luci_services.dart:39: /// Fetches all builds for a
given [commit]-hash, by searching the latest [amount] builds.
Long line.

https://codereview.chromium.org/3005443002/diff/40001/tools/gardening/lib/src...
tools/gardening/lib/src/luci_services.dart:57: "All latest $amount builds found
for client $client. Processing results...");
Long line.

[mkroghj, 2017-08-28 10:37:21]

Description was changed from

==========
Additional tools for gardening.
- multitest will generate a specific case of a multitest file
- luci_api is the start of a project, to make an api to ease the life     of the
gardener. At this point, it can answer, given a commit, which     builds on bots
does that commit belong to.
==========

to

==========
Additional tools for gardening.
- multitest will generate a specific case of a multitest file
- luci_api is the start of a project, to make an api to ease the life     of the
gardener. At this point, it can answer, given a commit, which     builds on bots
does that commit belong to.

R=johnniwinther@google.com

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

[mkroghj, 2017-08-28 10:37:22]

Committed patchset #4 (id:60001) manually as
fe94ff5ec05ed299b8af761c6f87eefa0e780a5c (presubmit successful).