Add support for pub install.

Add support for pub install.

This CL does several things:

* Adds explicit infrastructure for package sources.
* Adds the Dart SDK as such a source.
* Adds an explicit class for managing the packages/ directory (AppCache, as
  opposed to SystemCache).
* Adds an "install" command that installs to packages/, backed up by the system
  cache.

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

[Bob Nystrom, 2012-05-03 00:15:48]

Some minor suggestions but overall looks excellent. I'd like to talk about names
a bit in person before I OK this.

https://chromiumcodereview.appspot.com/10340005/diff/1/utils/pub/app_cache.dart
File utils/pub/app_cache.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/1/utils/pub/app_cache.da...
utils/pub/app_cache.dart:12: * This corresponds to the application's packages
directory.
Put "packages" in quotes for clarity?

https://chromiumcodereview.appspot.com/10340005/diff/1/utils/pub/app_cache.da...
utils/pub/app_cache.dart:16: * The root directory of the application containing
this cache.
Instead of root directory, can this just be the package that represents the app
itself?

https://chromiumcodereview.appspot.com/10340005/diff/1/utils/pub/app_cache.da...
utils/pub/app_cache.dart:47: // TODO(rnystrom): Make this path configurable.
TODO(nweiz)

I believe the goog convention is to not add TODOs that aren't your name. :)

https://chromiumcodereview.appspot.com/10340005/diff/1/utils/pub/app_cache.da...
utils/pub/app_cache.dart:65: if (package != null) return package;
new Future.immediate(package);

https://chromiumcodereview.appspot.com/10340005/diff/1/utils/pub/app_cache.da...
utils/pub/app_cache.dart:79: chain((pkg) => createSymlink(pkg.dir, packageDir));
Style nit, consider moving chain( to the previous line?

https://chromiumcodereview.appspot.com/10340005/diff/1/utils/pub/app_cache.da...
utils/pub/app_cache.dart:83: throw 'Package ${id.name} not found in source
"${id.source.name}".';
We should eventually have a more robust error-handling system, logging, etc.
Leave a TODO here to do something more sophisticated. At the very least it
should eventually throw a real exception type.

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/command_l...
File utils/pub/command_list.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/command_l...
utils/pub/command_list.dart:15: printIds(String title, List<PackageId> ids) {
I would go ahead and make this a method. Part of the reason I made commands
classes instead of just functions is to enable that.

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/command_l...
utils/pub/command_list.dart:16: ids = new List<PackageId>.from(ids);
How about making PackageId implement Comparable?

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/io.dart
File utils/pub/io.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/io.dart#n...
utils/pub/io.dart:70: });
I hate to suggest this, but ideally you could kick off fileExists and dirExists
in parallel, right?

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/io.dart#n...
utils/pub/io.dart:79: // Currently File#exists will not detect the existence of
symlinks. Issue 2765
Make this a TODO so it's greppable.

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/io.dart#n...
utils/pub/io.dart:151: // Error 17 means the directory already exists.
Move comment above preceding line.

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/io.dart#n...
utils/pub/io.dart:164: Future<Directory> ensureNestedDir(path) {
Is there any reason someone would prefer ensureDir over ensureNestedDir? If not,
and I can't think why they would, why not just make ensureDir implicitly handle
nesting?

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/sdk_sourc...
File utils/pub/sdk_source.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/sdk_sourc...
utils/pub/sdk_source.dart:28: var sourcePath = join(rootDir, "lib", id.name);
Might want to look in utils too at some point.

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/source.dart
File utils/pub/source.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/source.da...
utils/pub/source.dart:52: * false, [path] will be a path to the package
directory.
"package directory" -> 'application's "packages" directory'

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/system_ca...
File utils/pub/system_cache.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/system_ca...
utils/pub/system_cache.dart:53: if (!id.source.shouldCache) throw "[PUB BUG]
Package $id is not cacheable.";
Throw an IllegalArgumentException here and remove the [PUB BUG] part.

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/system_ca...
utils/pub/system_cache.dart:54: 
Should this have a cache of already-loaded-into-memory packages like AppCache
does?

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/system_ca...
utils/pub/system_cache.dart:60: if (exists) throw 'Package ${id.name} is already
installed.';
Add a TODO for better error-handling. Anytime we're throwing a string, I think
we should consider that temp code.

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/utils.dart
File utils/pub/utils.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/utils.dar...
utils/pub/utils.dart:27: always(Future future, fn()) {
I sketched out something like this once but didn't end up keeping it. It might
be nice to have it take (optional) arguments for the then and exception cases
too so you don't have to capture the future in a variable at all in the calling
code.

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/utils.dar...
utils/pub/utils.dart:42: for (var e in list) {
e -> element or item

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/utils.dar...
utils/pub/utils.dart:42: for (var e in list) {
"e" -> element or item

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/utils.dar...
utils/pub/utils.dart:43: if (e is List) {
Iterable?

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/tests/pub/tes...
File utils/tests/pub/test_pub.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/tests/pub/tes...
utils/tests/pub/test_pub.dart:97: try {
Is this temp code? If not, leave a comment explaining what it's for (I assume to
give you the right stack trace).

[Bob Nystrom, 2012-05-03 00:23:02]

Name changes like we discussed.

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/app_cache...
File utils/pub/app_cache.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/app_cache...
utils/pub/app_cache.dart:14: class AppCache {
"EntrypointCache"

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/package.dart
File utils/pub/package.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/package.d...
utils/pub/package.dart:125: final String name;
"fullName".

You may also want to make a "name" getter that just does
source.packageName(this);.

[nweiz, 2012-05-04 01:03:43]

https://chromiumcodereview.appspot.com/10340005/diff/1/utils/pub/app_cache.dart
File utils/pub/app_cache.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/1/utils/pub/app_cache.da...
utils/pub/app_cache.dart:12: * This corresponds to the application's packages
directory.
On 2012/05/03 00:15:49, Bob Nystrom wrote:
> Put "packages" in quotes for clarity?

Done.

https://chromiumcodereview.appspot.com/10340005/diff/1/utils/pub/app_cache.da...
utils/pub/app_cache.dart:16: * The root directory of the application containing
this cache.
On 2012/05/03 00:15:49, Bob Nystrom wrote:
> Instead of root directory, can this just be the package that represents the
app
> itself?

Done.

https://chromiumcodereview.appspot.com/10340005/diff/1/utils/pub/app_cache.da...
utils/pub/app_cache.dart:47: // TODO(rnystrom): Make this path configurable.
On 2012/05/03 00:15:49, Bob Nystrom wrote:
> TODO(nweiz)
> 
> I believe the goog convention is to not add TODOs that aren't your name. :)

This was a pre-existing comment on code I moved from command_update.dart.

https://chromiumcodereview.appspot.com/10340005/diff/1/utils/pub/app_cache.da...
utils/pub/app_cache.dart:65: if (package != null) return package;
On 2012/05/03 00:15:49, Bob Nystrom wrote:
> new Future.immediate(package);

Done.

https://chromiumcodereview.appspot.com/10340005/diff/1/utils/pub/app_cache.da...
utils/pub/app_cache.dart:79: chain((pkg) => createSymlink(pkg.dir, packageDir));
On 2012/05/03 00:15:49, Bob Nystrom wrote:
> Style nit, consider moving chain( to the previous line?

Done.

https://chromiumcodereview.appspot.com/10340005/diff/1/utils/pub/app_cache.da...
utils/pub/app_cache.dart:83: throw 'Package ${id.name} not found in source
"${id.source.name}".';
On 2012/05/03 00:15:49, Bob Nystrom wrote:
> We should eventually have a more robust error-handling system, logging, etc.
> Leave a TODO here to do something more sophisticated. At the very least it
> should eventually throw a real exception type.

Done.

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/command_l...
File utils/pub/command_list.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/command_l...
utils/pub/command_list.dart:15: printIds(String title, List<PackageId> ids) {
On 2012/05/03 00:15:49, Bob Nystrom wrote:
> I would go ahead and make this a method. Part of the reason I made commands
> classes instead of just functions is to enable that.

Done.

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/command_l...
utils/pub/command_list.dart:16: ids = new List<PackageId>.from(ids);
On 2012/05/03 00:15:49, Bob Nystrom wrote:
> How about making PackageId implement Comparable?

Done.

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/io.dart
File utils/pub/io.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/io.dart#n...
utils/pub/io.dart:70: });
On 2012/05/03 00:15:49, Bob Nystrom wrote:
> I hate to suggest this, but ideally you could kick off fileExists and
dirExists
> in parallel, right?

Done.

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/io.dart#n...
utils/pub/io.dart:79: // Currently File#exists will not detect the existence of
symlinks. Issue 2765
On 2012/05/03 00:15:49, Bob Nystrom wrote:
> Make this a TODO so it's greppable.

Done.

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/io.dart#n...
utils/pub/io.dart:151: // Error 17 means the directory already exists.
On 2012/05/03 00:15:49, Bob Nystrom wrote:
> Move comment above preceding line.

Done.

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/io.dart#n...
utils/pub/io.dart:164: Future<Directory> ensureNestedDir(path) {
On 2012/05/03 00:15:49, Bob Nystrom wrote:
> Is there any reason someone would prefer ensureDir over ensureNestedDir? If
not,
> and I can't think why they would, why not just make ensureDir implicitly
handle
> nesting?

Done.

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/package.dart
File utils/pub/package.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/package.d...
utils/pub/package.dart:125: final String name;
On 2012/05/03 00:23:02, Bob Nystrom wrote:
> "fullName".
> 
> You may also want to make a "name" getter that just does
> source.packageName(this);.

Done.

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/sdk_sourc...
File utils/pub/sdk_source.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/sdk_sourc...
utils/pub/sdk_source.dart:28: var sourcePath = join(rootDir, "lib", id.name);
On 2012/05/03 00:15:49, Bob Nystrom wrote:
> Might want to look in utils too at some point.

Something to talk to Dan about.

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/source.dart
File utils/pub/source.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/source.da...
utils/pub/source.dart:52: * false, [path] will be a path to the package
directory.
On 2012/05/03 00:15:49, Bob Nystrom wrote:
> "package directory" -> 'application's "packages" directory'

Done.

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/system_ca...
File utils/pub/system_cache.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/system_ca...
utils/pub/system_cache.dart:53: if (!id.source.shouldCache) throw "[PUB BUG]
Package $id is not cacheable.";
On 2012/05/03 00:15:49, Bob Nystrom wrote:
> Throw an IllegalArgumentException here and remove the [PUB BUG] part.

Done.

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/system_ca...
utils/pub/system_cache.dart:54: 
On 2012/05/03 00:15:49, Bob Nystrom wrote:
> Should this have a cache of already-loaded-into-memory packages like AppCache
> does?

I don't think so. Most if not all of the actual Package objects are going to be
accessed via PackageDir.

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/system_ca...
utils/pub/system_cache.dart:60: if (exists) throw 'Package ${id.name} is already
installed.';
On 2012/05/03 00:15:49, Bob Nystrom wrote:
> Add a TODO for better error-handling. Anytime we're throwing a string, I think
> we should consider that temp code.

Done.

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/utils.dart
File utils/pub/utils.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/utils.dar...
utils/pub/utils.dart:27: always(Future future, fn()) {
On 2012/05/03 00:15:49, Bob Nystrom wrote:
> I sketched out something like this once but didn't end up keeping it. It might
> be nice to have it take (optional) arguments for the then and exception cases
> too so you don't have to capture the future in a variable at all in the
calling
> code.

I don't follow... what would these arguments do?

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/utils.dar...
utils/pub/utils.dart:42: for (var e in list) {
On 2012/05/03 00:15:49, Bob Nystrom wrote:
> "e" -> element or item

Done.

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/utils.dar...
utils/pub/utils.dart:43: if (e is List) {
On 2012/05/03 00:15:49, Bob Nystrom wrote:
> Iterable?

That's how I had it originally, but I changed it to list because I wanted it to
be very clear which objects would get flattened and which wouldn't. Imagine if
you were flattening [[{"foo": "bar"}, 12]] and then Josh changes Map to
implement Iterable and the behavior changes. Ick.

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/tests/pub/tes...
File utils/tests/pub/test_pub.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/tests/pub/tes...
utils/tests/pub/test_pub.dart:97: try {
On 2012/05/03 00:15:49, Bob Nystrom wrote:
> Is this temp code? If not, leave a comment explaining what it's for (I assume
to
> give you the right stack trace).

Done.

[Bob Nystrom, 2012-05-04 17:02:06]

A couple of suggestions but otherwise LGTM!

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/utils.dart
File utils/pub/utils.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/utils.dar...
utils/pub/utils.dart:27: always(Future future, fn()) {
On 2012/05/04 01:03:43, nweiz wrote:
> On 2012/05/03 00:15:49, Bob Nystrom wrote:
> > I sketched out something like this once but didn't end up keeping it. It
might
> > be nice to have it take (optional) arguments for the then and exception
cases
> > too so you don't have to capture the future in a variable at all in the
> calling
> > code.
> 
> I don't follow... what would these arguments do?

I don't know if this is a good idea, but for example, here's some (simplified
code using always:


Future<Package> install(PackageId id) {
  var future = ensureDir(dirname(packageDir)).chain((_) {
    return exists(packageDir);
  }).chain((exists) {
    ...
  }).chain((_) => Package.load(packageDir));

  future.then((pkg) => _loadedPackages[id] = pkg);
  always(future, () => _pendingInstalls.remove(id));
  return future;
}

Instead of "always" imagine you replace that with "foo" (need some real name),
like:

Future<Package> install(PackageId id) {
  return foo(ensureDir(dirname(packageDir)).chain((_) {
      return exists(packageDir);
    }).chain((exists) {
      ...
    }).chain((_) => Package.load(packageDir)),
    then: (pkg) => _loadedPackages[id] = pkg,
    always: () => _pendingInstalls.remove(id));
  return future;
}

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/utils.dar...
utils/pub/utils.dart:43: if (e is List) {
On 2012/05/04 01:03:43, nweiz wrote:
> On 2012/05/03 00:15:49, Bob Nystrom wrote:
> > Iterable?
> 
> That's how I had it originally, but I changed it to list because I wanted it
to
> be very clear which objects would get flattened and which wouldn't. Imagine if
> you were flattening [[{"foo": "bar"}, 12]] and then Josh changes Map to
> implement Iterable and the behavior changes. Ick.

I was thinking the same thing. Type tests are always a little risky. :(

https://chromiumcodereview.appspot.com/10340005/diff/29001/utils/pub/command_...
File utils/pub/command_install.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/29001/utils/pub/command_...
utils/pub/command_install.dart:10: var installedDeps =
packagesDir.package.dependencies.map(
Seems like this could be a method on PackagesDir itself. Feels a bit non-OOP to
pull out a property of packagesDir and call a method on it passing the
packagesDir back in. Maybe just do:

packagesDir.install();

https://chromiumcodereview.appspot.com/10340005/diff/29001/utils/pub/io.dart
File utils/pub/io.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/29001/utils/pub/io.dart#...
utils/pub/io.dart:146: if (path == '.') return new Future.immediate(new
Directory('.'));
What if path == new File('.')?

Should probably do the _getPath() call first.

https://chromiumcodereview.appspot.com/10340005/diff/29001/utils/pub/packages...
File utils/pub/packages_dir.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/29001/utils/pub/packages...
utils/pub/packages_dir.dart:16: final Package package;
I found this name confusing when I saw it being used. How about "owner" or
something along those lines?

[nweiz, 2012-05-07 18:28:35]

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/utils.dart
File utils/pub/utils.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/6006/utils/pub/utils.dar...
utils/pub/utils.dart:27: always(Future future, fn()) {
On 2012/05/04 17:02:06, Bob Nystrom wrote:
> On 2012/05/04 01:03:43, nweiz wrote:
> > On 2012/05/03 00:15:49, Bob Nystrom wrote:
> > > I sketched out something like this once but didn't end up keeping it. It
> might
> > > be nice to have it take (optional) arguments for the then and exception
> cases
> > > too so you don't have to capture the future in a variable at all in the
> > calling
> > > code.
> > 
> > I don't follow... what would these arguments do?
> 
> I don't know if this is a good idea, but for example, here's some (simplified
> code using always:
> 
> 
> Future<Package> install(PackageId id) {
>   var future = ensureDir(dirname(packageDir)).chain((_) {
>     return exists(packageDir);
>   }).chain((exists) {
>     ...
>   }).chain((_) => Package.load(packageDir));
> 
>   future.then((pkg) => _loadedPackages[id] = pkg);
>   always(future, () => _pendingInstalls.remove(id));
>   return future;
> }
> 
> Instead of "always" imagine you replace that with "foo" (need some real name),
> like:
> 
> Future<Package> install(PackageId id) {
>   return foo(ensureDir(dirname(packageDir)).chain((_) {
>       return exists(packageDir);
>     }).chain((exists) {
>       ...
>     }).chain((_) => Package.load(packageDir)),
>     then: (pkg) => _loadedPackages[id] = pkg,
>     always: () => _pendingInstalls.remove(id));
>   return future;
> }

I don't think that really adds any clarity.

https://chromiumcodereview.appspot.com/10340005/diff/29001/utils/pub/command_...
File utils/pub/command_install.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/29001/utils/pub/command_...
utils/pub/command_install.dart:10: var installedDeps =
packagesDir.package.dependencies.map(
On 2012/05/04 17:02:07, Bob Nystrom wrote:
> Seems like this could be a method on PackagesDir itself. Feels a bit non-OOP
to
> pull out a property of packagesDir and call a method on it passing the
> packagesDir back in. Maybe just do:
> 
> packagesDir.install();

Done.

https://chromiumcodereview.appspot.com/10340005/diff/29001/utils/pub/io.dart
File utils/pub/io.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/29001/utils/pub/io.dart#...
utils/pub/io.dart:146: if (path == '.') return new Future.immediate(new
Directory('.'));
On 2012/05/04 17:02:07, Bob Nystrom wrote:
> What if path == new File('.')?
> 
> Should probably do the _getPath() call first.

Done.

https://chromiumcodereview.appspot.com/10340005/diff/29001/utils/pub/packages...
File utils/pub/packages_dir.dart (right):

https://chromiumcodereview.appspot.com/10340005/diff/29001/utils/pub/packages...
utils/pub/packages_dir.dart:16: final Package package;
On 2012/05/04 17:02:07, Bob Nystrom wrote:
> I found this name confusing when I saw it being used. How about "owner" or
> something along those lines?

Done.