Stop running pub get at gclient sync time and fix build bugs

+ Stop running `pub get` at gclient sync time. This was slow and unnecessary.
+ Stop symlinking non-dart_pkg packages into gen/dart-pkg/packages. This suffered from three problems:
   1) These symlinks were not being specified as build output by any rule. They were a magic side effect.
   2) There was a race where many symlinks to the same package could be created, the last one winning.
   3) Related to 2, if two packages relied on two different versions of the same package, we could have hard to reproduce bugs that depend upon the winner of the race.

We now have a checked in copy of all third-party Dart packages needed by the Mojo Dart SDK. These live under //mojo/public/dart/third_party. There is a script 'update_packages.py' which uses `pub get` to handle all version constraints and updates the source tree after pub is run. It's now trivial to ensure we have a compatible set of packages checked into the tree and to update the package set.

Fixes #395
Fixes #401
Fixes #403
Fixes #429

R=jamesr@chromium.org, tonyg@chromium.org, zra@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/9f60559afee8613b43714fa37792f080a09125b4

[Cutch, 2015-09-16 13:51:51]

johnmccutchan@google.com changed reviewers:
+ jamesr@chromium.org, tonyg@chromium.org

[tonyg, 2015-09-16 14:00:38]

Seems neat. A couple of questions inline and I'm also curious how this interacts
with the dependencies specified in the pubspec.yaml. Do we need to make those
all path relative so that the editor doesn't point at a different version?

https://codereview.chromium.org/1346773002/diff/40001/.gitignore
File .gitignore (right):

https://codereview.chromium.org/1346773002/diff/40001/.gitignore#newcode43
.gitignore:43: /third_party/dart-pkg/archive
This list seems hard to maintain.

It looks like the only thing we don't ignore in the dart-pkg dir is BUILD.gn. If
so, we could move that file to //build/secondary/third_party/dart-pkg/BUILD.gn.
The build system finds it there automatically and then we can just ignore this
entire dir.

https://codereview.chromium.org/1346773002/diff/40001/DEPS
File DEPS (right):

https://codereview.chromium.org/1346773002/diff/40001/DEPS#newcode35
DEPS:35: 'args_dart_revision': '@0.13.2',
Is there a straightforward way to update all of these to the most recent
compatible versions just like a `pub upgrade` would do? Otherwise, I don't see
how we'd maintain all these versions here.

[tonyg, 2015-09-16 14:00:49]

tonyg@chromium.org changed reviewers:
+ zra@google.com

[tonyg, 2015-09-16 14:00:59]

[+zra]

[Cutch, 2015-09-16 14:13:26]

On 2015/09/16 14:00:38, tonyg wrote:
> Seems neat. A couple of questions inline and I'm also curious how this
interacts
> with the dependencies specified in the pubspec.yaml. Do we need to make those
> all path relative so that the editor doesn't point at a different version?
> 

No. Instead we should tighten the version constraints in the pubspec.yaml so
that we are always in sync.

> https://codereview.chromium.org/1346773002/diff/40001/.gitignore
> File .gitignore (right):
> 
> https://codereview.chromium.org/1346773002/diff/40001/.gitignore#newcode43
> .gitignore:43: /third_party/dart-pkg/archive
> This list seems hard to maintain.
> 
> It looks like the only thing we don't ignore in the dart-pkg dir is BUILD.gn.
If
> so, we could move that file to
//build/secondary/third_party/dart-pkg/BUILD.gn.
> The build system finds it there automatically and then we can just ignore this
> entire dir.
> 

I think git allows for ignore exclusions, so we could add the following to the
.gitignore:

third_party/dart-pkg/*
!third_party/dart-pkg/BUILD.gn

> https://codereview.chromium.org/1346773002/diff/40001/DEPS
> File DEPS (right):
> 
> https://codereview.chromium.org/1346773002/diff/40001/DEPS#newcode35
> DEPS:35: 'args_dart_revision': mailto:,
> Is there a straightforward way to update all of these to the most recent
> compatible versions just like a `pub upgrade` would do? Otherwise, I don't see
> how we'd maintain all these versions here.

The Dart team has maintained this list by hand for years. It's slightly tedious
and we could write a script to help automate this down the road.

[Cutch, 2015-09-16 14:18:34]

https://codereview.chromium.org/1346773002/diff/40001/.gitignore
File .gitignore (right):

https://codereview.chromium.org/1346773002/diff/40001/.gitignore#newcode43
.gitignore:43: /third_party/dart-pkg/archive
On 2015/09/16 14:00:37, tonyg wrote:
> This list seems hard to maintain.
> 
> It looks like the only thing we don't ignore in the dart-pkg dir is BUILD.gn.
If
> so, we could move that file to
//build/secondary/third_party/dart-pkg/BUILD.gn.
> The build system finds it there automatically and then we can just ignore this
> entire dir.

Done.

[tonyg, 2015-09-16 14:27:01]

I'm still slightly concerned that a lot of magic files have to line up in order
to use a package. Previously we only had to add it to the pubspec.yaml.

Is there a good place for a comment or documentation explaining all the various
files to touch when adding a package as well as how to upgrade packages?

[Cutch, 2015-09-16 14:44:05]

On 2015/09/16 14:27:01, tonyg wrote:
> I'm still slightly concerned that a lot of magic files have to line up in
order
> to use a package. Previously we only had to add it to the pubspec.yaml.
> 
> Is there a good place for a comment or documentation explaining all the
various
> files to touch when adding a package as well as how to upgrade packages?

I've added documentation under third_party/dart-pkg explaining the steps to add
a new dependency and to upgrade an existing dependency. Both require few steps:

There are three files that need to be edited to add a new dependency:

1. /DEPS
2. /third_party/dart-pkg/BUILD.gn
3. pubspec.yaml


There are two files that need to be edited to upgrade a dependency:

1. /DEPS
2. pubspec.yaml

The previous system was convenient but it suffers from difficult to track down
races and cannot properly specify build output. The new system is well vetted by
the Dart team in the Dart SDK tree.

[tonyg, 2015-09-16 14:47:27]

lgtm

I'd recommend letting James take a look too.

[Cutch, 2015-09-16 14:48:15]

On 2015/09/16 14:47:27, tonyg wrote:
> lgtm
> 
> I'd recommend letting James take a look too.

Yes, I'm waiting on James' LGTM because I want to discuss some details with him
before landing.

[zra, 2015-09-16 16:03:53]

I think this is the right thing to do. As we get more experience with this
approach, I think we'll find out what additional tooling support is needed. For
example, it might be nice to have a presubmit check to fail if a pubspec.yaml
has been updated with a new dependency, but that dependency hasn't been added to
//third-party/dart-pkg/.

https://codereview.chromium.org/1346773002/diff/100001/DEPS
File DEPS (right):

https://codereview.chromium.org/1346773002/diff/100001/DEPS#newcode38
DEPS:38: 'collection_dart_revision': '79ebc6fc2dae581cb23ad50a5c600c1b7dd132f8',
Maybe file github issues with these to add version tags.

https://codereview.chromium.org/1346773002/diff/100001/mojo/dart/BUILD.gn
File mojo/dart/BUILD.gn (right):

https://codereview.chromium.org/1346773002/diff/100001/mojo/dart/BUILD.gn#new...
mojo/dart/BUILD.gn:11: "//third_party/dart-pkg",
why does this need to be both here and under //mojo:mojo?

https://codereview.chromium.org/1346773002/diff/100001/mojo/public/dart/mojo_...
File mojo/public/dart/mojo_apptest/BUILD.gn (right):

https://codereview.chromium.org/1346773002/diff/100001/mojo/public/dart/mojo_...
mojo/public/dart/mojo_apptest/BUILD.gn:8: deps = [
I think we've been following the convention that libs and apps come first
followed by sources with deps last.

https://codereview.chromium.org/1346773002/diff/100001/mojo/public/tools/git/...
File mojo/public/tools/git/dart_pub_get.py (left):

https://codereview.chromium.org/1346773002/diff/100001/mojo/public/tools/git/...
mojo/public/tools/git/dart_pub_get.py:1: #!/usr/bin/python
One of the things that pub does is ensure that package versions are compatible.
The set of versions in our DEPS may now all be compatible with eachother, but
upgrading one package may cause a cascade of upgrades to other packages to be
required. Is there some way to still use pub to figure out what all the new
versions should be? Maybe this script could still do it:
- Update the pubspec.yaml you care about,
- run this script,
- inspect dart-pub-cache to see what versions pub thinks you need,
- update DEPS to those versions.

https://codereview.chromium.org/1346773002/diff/100001/third_party/dart-pkg/R...
File third_party/dart-pkg/README.md (right):

https://codereview.chromium.org/1346773002/diff/100001/third_party/dart-pkg/R...
third_party/dart-pkg/README.md:12: 2. Edit any related `pubspec.yaml` files and
adjust version constraints accordingly
Yah, I think this could use a little fleshing out.

[Cutch, 2015-09-16 16:50:25]

On 2015/09/16 16:03:53, zra wrote:
> I think this is the right thing to do. As we get more experience with this
> approach, I think we'll find out what additional tooling support is needed.
For
> example, it might be nice to have a presubmit check to fail if a pubspec.yaml
> has been updated with a new dependency, but that dependency hasn't been added
to
> //third-party/dart-pkg/.
> 

Agreed on long tail tooling additions. The goal of this CL is just move us
closer to the ideal end state.

> https://codereview.chromium.org/1346773002/diff/100001/DEPS
> File DEPS (right):
> 
> https://codereview.chromium.org/1346773002/diff/100001/DEPS#newcode38
> DEPS:38: 'collection_dart_revision':
'79ebc6fc2dae581cb23ad50a5c600c1b7dd132f8',
> Maybe file github issues with these to add version tags.
> 
> https://codereview.chromium.org/1346773002/diff/100001/mojo/dart/BUILD.gn
> File mojo/dart/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1346773002/diff/100001/mojo/dart/BUILD.gn#new...
> mojo/dart/BUILD.gn:11: "//third_party/dart-pkg",
> why does this need to be both here and under //mojo:mojo?
> 

This is related to what I need to discuss with James before committing. We have
an issue where //mojo/public/dart depends on //third_party/dart-pkg (at build
time because we run the analyzer at build time) but we cannot express this in
the build system because we are forbidden in //mojo/public/ from depending on
//third_party. Likely we need to move these dependencies to
//mojo/public/third_party before committing.

>
https://codereview.chromium.org/1346773002/diff/100001/mojo/public/dart/mojo_...
> File mojo/public/dart/mojo_apptest/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1346773002/diff/100001/mojo/public/dart/mojo_...
> mojo/public/dart/mojo_apptest/BUILD.gn:8: deps = [
> I think we've been following the convention that libs and apps come first
> followed by sources with deps last.
> 
>
https://codereview.chromium.org/1346773002/diff/100001/mojo/public/tools/git/...
> File mojo/public/tools/git/dart_pub_get.py (left):
> 
>
https://codereview.chromium.org/1346773002/diff/100001/mojo/public/tools/git/...
> mojo/public/tools/git/dart_pub_get.py:1: #!/usr/bin/python
> One of the things that pub does is ensure that package versions are
compatible.
> The set of versions in our DEPS may now all be compatible with eachother, but
> upgrading one package may cause a cascade of upgrades to other packages to be
> required. Is there some way to still use pub to figure out what all the new
> versions should be? Maybe this script could still do it:
> - Update the pubspec.yaml you care about,
> - run this script,
> - inspect dart-pub-cache to see what versions pub thinks you need,
> - update DEPS to those versions.
> 

It should be easy to write a script around running pub get in a temporary
directory and parsing the pubspec.lock to scrape version numbers.

[Cutch, 2015-09-16 16:56:17]

https://codereview.chromium.org/1346773002/diff/100001/DEPS
File DEPS (right):

https://codereview.chromium.org/1346773002/diff/100001/DEPS#newcode38
DEPS:38: 'collection_dart_revision': '79ebc6fc2dae581cb23ad50a5c600c1b7dd132f8',
On 2015/09/16 16:03:53, zra wrote:
> Maybe file github issues with these to add version tags.

Done for collection, crypto, http_parser, and utf.

[zra, 2015-09-16 17:31:35]

lgtm after jamesr signs off.

[Cutch, 2015-09-16 22:19:59]

James- PTAL.

The CL has grown because I had to move dart and all of the third-party dart
packages under mojo/public/third_party. This fixes another missing build
dependency (//third_party/dart-pkg is needed by //mojo/public/dart/mojo but was
never expressed.)

[jamesr, 2015-09-16 22:32:52]

Code changes LGTM.  As we discussed, mechanics of this will have to change
before landing given the way we currently mirror+distribute the mojo sdk

https://codereview.chromium.org/1346773002/diff/120001/DEPS
File DEPS (right):

https://codereview.chromium.org/1346773002/diff/120001/DEPS#newcode34
DEPS:34: 'args_dart_revision': '@0.13.2',
what are these @ annotations referring to? git tags?

https://codereview.chromium.org/1346773002/diff/120001/DEPS#newcode101
DEPS:101:
'src/mojo/public/third_party/dart/third_party/observatory_pub_packages':
as we discussed in person DEPSing stuff into mojo/public/.. will break the way
we currently package and distribute the mojo sdk, so these need to be checked in
for now.

[Cutch, 2015-09-17 20:55:23]

I'm now checking in all third-party package dependencies under
mojo/public/dart/third_party and have written a tool to automate the updating of
packages.

https://codereview.chromium.org/1346773002/diff/120001/DEPS
File DEPS (right):

https://codereview.chromium.org/1346773002/diff/120001/DEPS#newcode34
DEPS:34: 'args_dart_revision': '@0.13.2',
On 2015/09/16 22:32:52, jamesr wrote:
> what are these @ annotations referring to? git tags?

These annotations are gone now but yes, they refer to git tags.

https://codereview.chromium.org/1346773002/diff/120001/DEPS#newcode101
DEPS:101:
'src/mojo/public/third_party/dart/third_party/observatory_pub_packages':
On 2015/09/16 22:32:52, jamesr wrote:
> as we discussed in person DEPSing stuff into mojo/public/.. will break the way
> we currently package and distribute the mojo sdk, so these need to be checked
in
> for now.

Done.

[Cutch, 2015-09-17 21:10:07]

Committed patchset #8 (id:140001) manually as
9f60559afee8613b43714fa37792f080a09125b4 (presubmit successful).