Added PkgBuildTestSuite (aka 'pkgbuild')

Added PkgBuildTestSuite (aka 'pkgbuild')

This CL adds
 - support for fetching dependencies via 'pub get' of samples/packages
 - support for building applications/samples via 'pub build' (or a custom
   build.dart file)
 - pkg/pkgbuild.status: Status files for all package checkouts/builds
 - the '--use-public-packages' option to test.dart
 - the CleanDirectoryCopyCommand, PubCommand, ModifyPubspecYamlCommand commands

The test suite can be executed with:
 $ ./tools/test.py -mrelease --use-sdk pkgbuild
 $ ./tools/test.py -mrelease --use-sdk --use-public-packages pkgbuild

R=ricow@google.com

Committed: https://code.google.com/p/dart/source/detail?r=31535

[kustermann, 2014-01-06 16:21:23]

I've added the PubCommand (from CL https://codereview.chromium.org/98183009/) to
this one, so pkg/pkgbuild.status can have more specific status file markers.

PTAL

[ricow1, 2014-01-07 07:38:29]

https://codereview.chromium.org/122443003/diff/1/dart/tools/testing/dart/test...
File dart/tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/122443003/diff/1/dart/tools/testing/dart/test...
dart/tools/testing/dart/test_runner.dart:396: // FIXME: This is linux only.
move this to TestUtils and special case windows

https://codereview.chromium.org/122443003/diff/1/dart/tools/testing/dart/test...
dart/tools/testing/dart/test_runner.dart:448: return file.openRead()
file.readAsString().then(String yamlString)

https://codereview.chromium.org/122443003/diff/1/dart/tools/testing/dart/test...
dart/tools/testing/dart/test_runner.dart:467: return
((destinationFile.openWrite())..write(modifiedYamlString)).close();
return destinationFile.writeAsString(modifiedYamlString);

https://codereview.chromium.org/122443003/diff/1/dart/tools/testing/dart/test...
dart/tools/testing/dart/test_runner.dart:582: pubspecYamlFile, destinationFile,
depsOverrides));
indentation seems odd

https://codereview.chromium.org/122443003/diff/1/dart/tools/testing/dart/test...
File dart/tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/122443003/diff/1/dart/tools/testing/dart/test...
dart/tools/testing/dart/test_suite.dart:459: DebugLogger.warning("Expected a
$yamlFile while discovering all "
commented out code

[kustermann, 2014-01-07 08:24:12]

PTAL

https://codereview.chromium.org/122443003/diff/1/dart/tools/testing/dart/test...
File dart/tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/122443003/diff/1/dart/tools/testing/dart/test...
dart/tools/testing/dart/test_runner.dart:396: // FIXME: This is linux only.
On 2014/01/07 07:38:29, ricow1 wrote:
> move this to TestUtils and special case windows

Done.

https://codereview.chromium.org/122443003/diff/1/dart/tools/testing/dart/test...
dart/tools/testing/dart/test_runner.dart:448: return file.openRead()
On 2014/01/07 07:38:29, ricow1 wrote:
> file.readAsString().then(String yamlString)

Done.

https://codereview.chromium.org/122443003/diff/1/dart/tools/testing/dart/test...
dart/tools/testing/dart/test_runner.dart:467: return
((destinationFile.openWrite())..write(modifiedYamlString)).close();
On 2014/01/07 07:38:29, ricow1 wrote:
> return destinationFile.writeAsString(modifiedYamlString);

Done.

https://codereview.chromium.org/122443003/diff/1/dart/tools/testing/dart/test...
dart/tools/testing/dart/test_runner.dart:582: pubspecYamlFile, destinationFile,
depsOverrides));
On 2014/01/07 07:38:29, ricow1 wrote:
> indentation seems odd

Done.

https://codereview.chromium.org/122443003/diff/1/dart/tools/testing/dart/test...
File dart/tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/122443003/diff/1/dart/tools/testing/dart/test...
dart/tools/testing/dart/test_suite.dart:459: DebugLogger.warning("Expected a
$yamlFile while discovering all "
On 2014/01/07 07:38:29, ricow1 wrote:
> commented out code

I removed the warning now.

[ricow1, 2014-01-07 09:11:25]

LGTM, maybe get Bob to take a look as well to make sure we don't have any
assumptions about pub/pkg layout that is incorrect

https://codereview.chromium.org/122443003/diff/190001/dart/tools/testing/dart...
File dart/tools/testing/dart/test_options.dart (right):

https://codereview.chromium.org/122443003/diff/190001/dart/tools/testing/dart...
dart/tools/testing/dart/test_options.dart:276: 'For tests inside a package: Use
pub.dartlang.org packages '
For tests inside a package -> For tests using packages

https://codereview.chromium.org/122443003/diff/190001/dart/tools/testing/dart...
File dart/tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/122443003/diff/190001/dart/tools/testing/dart...
dart/tools/testing/dart/test_suite.dart:178: String suffix =
getExecutableSuffix('pub');
move this down 3 lines to keep prefix generation first then suffix generation

https://codereview.chromium.org/122443003/diff/190001/dart/tools/testing/dart...
dart/tools/testing/dart/test_suite.dart:191: String suffix =
getExecutableSuffix('vm');
same as above

https://codereview.chromium.org/122443003/diff/190001/dart/tools/testing/dart...
dart/tools/testing/dart/test_suite.dart:439: isValid(packageName) => packageName
!= 'third_party';
to me this is pretty obvious, but maybe just add a comment giving the layout of
our package dirs for future readability

https://codereview.chromium.org/122443003/diff/190001/dart/tools/testing/dart...
dart/tools/testing/dart/test_suite.dart:1887: return 0;
do you need an explicit return here?

[kustermann, 2014-01-07 09:36:07]

@bob:
If you have comments, I'll address them in another CL.

https://codereview.chromium.org/122443003/diff/190001/dart/tools/testing/dart...
File dart/tools/testing/dart/test_options.dart (right):

https://codereview.chromium.org/122443003/diff/190001/dart/tools/testing/dart...
dart/tools/testing/dart/test_options.dart:276: 'For tests inside a package: Use
pub.dartlang.org packages '
On 2014/01/07 09:11:25, ricow1 wrote:
> For tests inside a package -> For tests using packages

Done.

https://codereview.chromium.org/122443003/diff/190001/dart/tools/testing/dart...
File dart/tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/122443003/diff/190001/dart/tools/testing/dart...
dart/tools/testing/dart/test_suite.dart:178: String suffix =
getExecutableSuffix('pub');
On 2014/01/07 09:11:25, ricow1 wrote:
> move this down 3 lines to keep prefix generation first then suffix generation

Done.

https://codereview.chromium.org/122443003/diff/190001/dart/tools/testing/dart...
dart/tools/testing/dart/test_suite.dart:191: String suffix =
getExecutableSuffix('vm');
On 2014/01/07 09:11:25, ricow1 wrote:
> same as above

Done.

https://codereview.chromium.org/122443003/diff/190001/dart/tools/testing/dart...
dart/tools/testing/dart/test_suite.dart:439: isValid(packageName) => packageName
!= 'third_party';
On 2014/01/07 09:11:25, ricow1 wrote:
> to me this is pretty obvious, but maybe just add a comment giving the layout
of
> our package dirs for future readability

Done.

https://codereview.chromium.org/122443003/diff/190001/dart/tools/testing/dart...
dart/tools/testing/dart/test_suite.dart:1887: return 0;
On 2014/01/07 09:11:25, ricow1 wrote:
> do you need an explicit return here?

I don't

[kustermann, 2014-01-07 09:36:39]

Committed patchset #4 manually as r31535 (presubmit successful).

[Bob Nystrom, 2014-01-07 17:13:43]

I didn't scrutinize the patch in detail but I really like what I see. A couple
of suggestions/comments, but otherwise LGTM!

https://codereview.chromium.org/122443003/diff/280001/dart/tools/test.dart
File dart/tools/test.dart (right):

https://codereview.chromium.org/122443003/diff/280001/dart/tools/test.dart#ne...
dart/tools/test.dart:200: exit(1);
Instead of making this an error, why not just have "pkgbuild" implicitly set
"use_sdk" too?

https://codereview.chromium.org/122443003/diff/280001/dart/tools/testing/dart...
File dart/tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/122443003/diff/280001/dart/tools/testing/dart...
dart/tools/testing/dart/test_runner.dart:477: "dependency_overrides:\n");
This will do bad things if the pubspec already contains dependency_overrides.

One option is to parse the entire pubspec using the yaml package (which is in
the Dart repo). That will give you back a regular map. You can then find/add the
"dependency_overrides" key, and add them to that. Then you can just resave that
pubspec by serializing it to JSON. YAML is a superset of JSON, so pub will be
able to read that in fine.

https://codereview.chromium.org/122443003/diff/280001/dart/tools/testing/dart...
File dart/tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/122443003/diff/280001/dart/tools/testing/dart...
dart/tools/testing/dart/test_suite.dart:374: * Create a directories for
generated assets (tests, html files,
"a directories" -> "directories".

https://codereview.chromium.org/122443003/diff/280001/dart/tools/testing/dart...
dart/tools/testing/dart/test_suite.dart:424: .list(recursive: false)
These should be indented +4 according to the style guide.

[kustermann, 2014-01-10 15:48:04]

https://codereview.chromium.org/122443003/diff/280001/dart/tools/test.dart
File dart/tools/test.dart (right):

https://codereview.chromium.org/122443003/diff/280001/dart/tools/test.dart#ne...
dart/tools/test.dart:200: exit(1);
On 2014/01/07 17:13:43, Bob Nystrom wrote:
> Instead of making this an error, why not just have "pkgbuild" implicitly set
> "use_sdk" too?

Because setting use_sdk will change the configuration and the configuration is
shared across multiple test suites (and it does make a difference whether
usd_sdk is true or not).

https://codereview.chromium.org/122443003/diff/280001/dart/tools/testing/dart...
File dart/tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/122443003/diff/280001/dart/tools/testing/dart...
dart/tools/testing/dart/test_runner.dart:477: "dependency_overrides:\n");
On 2014/01/07 17:13:43, Bob Nystrom wrote:
> This will do bad things if the pubspec already contains dependency_overrides.
> 
> One option is to parse the entire pubspec using the yaml package (which is in
> the Dart repo). That will give you back a regular map. You can then find/add
the
> "dependency_overrides" key, and add them to that. Then you can just resave
that
> pubspec by serializing it to JSON. YAML is a superset of JSON, so pub will be
> able to read that in fine.

I was aware that this could cause issues. But so far it doesn't.

At the beginning I actually started out with using a copy of pkg/yaml (we need a
copy, we can't just import this one, because the testing scripts have to be
independent of the packages we're testing).

Then I ran into two issues:
- pkg/yaml didn't provide a way to save it again
- manipulating json is not what I want:
  * what I would like to have is a 'class PubspecYaml' represents exactly what
is allowed in pubspec.yaml files

So I started looking into sdk/lib/_internal/pub/lib/src/pubspec.dart. But this
file doesn't just represent pubspec.yaml, it's importing tons of other dart
files (and I don't want to make a copy these).

I've tried going the simple hackisch way, and it works, so I stayed with that.

I don't understand why yaml should be a subset/superset of json. Looking at
syntax it's just different. But I guess you're right and I just have to look
into the yaml/json specs to figure it out.

[Bob Nystrom, 2014-01-10 17:54:58]

https://codereview.chromium.org/122443003/diff/280001/dart/tools/testing/dart...
File dart/tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/122443003/diff/280001/dart/tools/testing/dart...
dart/tools/testing/dart/test_runner.dart:477: "dependency_overrides:\n");
On 2014/01/10 15:48:05, kustermann wrote:
> - pkg/yaml didn't provide a way to save it again

Yeah, that's an open issue, but just serializing to JSON is a valid way to save
back to YAML. It's just not the same YAML you originally parsed.

> - manipulating json is not what I want:
>   * what I would like to have is a 'class PubspecYaml' represents exactly what
> is allowed in pubspec.yaml files

Yeah, that would be nice, I agree.

> I don't understand why yaml should be a subset/superset of json. Looking at
> syntax it's just different.

YAML has a pretty flexible syntax. Maps and lists can be created using
whitespace for indentation, but it also allows {} and []. When you do that, you
effectively have JSON. From the YAML spec:

1.3. Relation to JSON

Both JSON and YAML aim to be human readable data interchange formats. However,
JSON and YAML have different priorities. JSON’s foremost design goal is
simplicity and universality. Thus, JSON is trivial to generate and parse, at the
cost of reduced human readability. It also uses a lowest common denominator
information model, ensuring any JSON data can be easily processed by every
modern programming environment.

In contrast, YAML’s foremost design goals are human readability and support for
serializing arbitrary native data structures. Thus, YAML allows for extremely
readable files, but is more complex to generate and parse. In addition, YAML
ventures beyond the lowest common denominator data types, requiring more complex
processing when crossing between different programming environments.

YAML can therefore be viewed as a natural superset of JSON, offering improved
human readability and a more complete information model. This is also the case
in practice; every JSON file is also a valid YAML file. This makes it easy to
migrate from JSON to YAML if/when the additional features are required.