Use precompiled dependencies in pub.

Use precompiled dependencies in pub.

BUG=20859
R=rnystrom@google.com

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

[nweiz, 2014-09-19 01:24:34]

nweiz@google.com changed reviewers:
+ rnystrom@google.com

[Bob Nystrom, 2014-09-23 19:47:46]

The new methods on Package are super nice. Great idea.

https://codereview.chromium.org/583853002/diff/1/samples/third_party/todomvc/...
File samples/third_party/todomvc/pubspec.yaml (left):

https://codereview.chromium.org/583853002/diff/1/samples/third_party/todomvc/...
samples/third_party/todomvc/pubspec.yaml:8: entry_points: web/index.html
?

https://codereview.chromium.org/583853002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/cached_package.dart (right):

https://codereview.chromium.org/583853002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/cached_package.dart:16: class CachedPackage
extends Package {
Doc comment.

https://codereview.chromium.org/583853002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/cached_package.dart:23: String part5, String
part6, String part7]) {
The indent is weird here.

https://codereview.chromium.org/583853002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command/global_activate.dart (right):

https://codereview.chromium.org/583853002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/global_activate.dart:34: help: "Overwrite
executables from other packages with the same name.");
You'll want to rebase this.

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart (right):

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart:354: if
(!graph.packages.containsKey(components[1])) return new Future.value([]);
async?

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/cached_package.dart (right):

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/cached_package.dart:29: /// Creates a new cached
package wrapping [inner] with the cache at [_cacheDir].
Long line.

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/cached_package.dart:47: List<String>
listFiles({String beneath, recursive: true}) {
Should probably document that this includes post-transformed files in lib, but
not elsewhere. Unless "beneath" is null in which case it will have the
pre-transformed "lib" contents too, right?

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:76: String get packagesDir =>
root.path('packages');
What do you think about moving these three getters to package? It seems a bit
weird that they're here.

(An even more radical change would be to have Entrypoint extend Package, so that
it explicitly *is* the entrypoint package. But that might be weird.)

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/package.dart (right):

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/package.dart:129: /// Returns the concrete
location of a path relative to this package's root.
"concrete" and "relative" are a bit confusing here. How about: "Given a relative
path within this package, returns its absolute path".

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/package.dart:144: /// Given a path within this
package (such as that returned by [path] or
"a path" -> "an absolute path".

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/package.dart:145: /// [listFiles]), returns that
path relative to the package root.
"that path" -> "it".

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/package.dart:156: /// Throws an [ArgumentError] if
[id.package] isn't [name].
I wouldn't bother documenting this.

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/package_graph.dart (right):

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/package_graph.dart:27: /// All transitive
dependencies of the entrypoint (including itself).
Should update this since now it's not always all the deps.

[nweiz, 2014-09-23 21:34:43]

Code review changes

[nweiz, 2014-09-23 21:34:50]

https://codereview.chromium.org/583853002/diff/1/samples/third_party/todomvc/...
File samples/third_party/todomvc/pubspec.yaml (left):

https://codereview.chromium.org/583853002/diff/1/samples/third_party/todomvc/...
samples/third_party/todomvc/pubspec.yaml:8: entry_points: web/index.html
On 2014/09/23 19:47:45, Bob Nystrom wrote:
> ?

This diff is removed in the latest upload.

https://codereview.chromium.org/583853002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/cached_package.dart (right):

https://codereview.chromium.org/583853002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/cached_package.dart:16: class CachedPackage
extends Package {
On 2014/09/23 19:47:45, Bob Nystrom wrote:
> Doc comment.

Done.

https://codereview.chromium.org/583853002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/cached_package.dart:23: String part5, String
part6, String part7]) {
On 2014/09/23 19:47:45, Bob Nystrom wrote:
> The indent is weird here.

Done.

https://codereview.chromium.org/583853002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command/global_activate.dart (right):

https://codereview.chromium.org/583853002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/global_activate.dart:34: help: "Overwrite
executables from other packages with the same name.");
On 2014/09/23 19:47:45, Bob Nystrom wrote:
> You'll want to rebase this.

Also fixed in the latest upload.

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart (right):

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart:354: if
(!graph.packages.containsKey(components[1])) return new Future.value([]);
On 2014/09/23 19:47:45, Bob Nystrom wrote:
> async?

I didn't want to make a bunch of async transformations in methods that just had
mechanical method-reference changes. Too much churn at once.

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/cached_package.dart (right):

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/cached_package.dart:29: /// Creates a new cached
package wrapping [inner] with the cache at [_cacheDir].
On 2014/09/23 19:47:45, Bob Nystrom wrote:
> Long line.

Done.

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/cached_package.dart:47: List<String>
listFiles({String beneath, recursive: true}) {
On 2014/09/23 19:47:45, Bob Nystrom wrote:
> Should probably document that this includes post-transformed files in lib, but
> not elsewhere. Unless "beneath" is null in which case it will have the
> pre-transformed "lib" contents too, right?

Done.

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:76: String get packagesDir =>
root.path('packages');
On 2014/09/23 19:47:45, Bob Nystrom wrote:
> What do you think about moving these three getters to package? It seems a bit
> weird that they're here.
> 
> (An even more radical change would be to have Entrypoint extend Package, so
that
> it explicitly *is* the entrypoint package. But that might be weird.)

I don't think I like that change. Packages directories and lock files are
intimately tied to the fact that a package is an entrypoint. It doesn't make
sense to have a lock file for some arbitrary dependency. Even when we're doing
global activate, we create a virtual entrypoint.

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/package.dart (right):

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/package.dart:129: /// Returns the concrete
location of a path relative to this package's root.
On 2014/09/23 19:47:45, Bob Nystrom wrote:
> "concrete" and "relative" are a bit confusing here. How about: "Given a
relative
> path within this package, returns its absolute path".

Done.

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/package.dart:145: /// [listFiles]), returns that
path relative to the package root.
On 2014/09/23 19:47:45, Bob Nystrom wrote:
> "that path" -> "it".

Done.

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/package.dart:156: /// Throws an [ArgumentError] if
[id.package] isn't [name].
On 2014/09/23 19:47:45, Bob Nystrom wrote:
> I wouldn't bother documenting this.

Done.

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/package_graph.dart (right):

https://codereview.chromium.org/583853002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/package_graph.dart:27: /// All transitive
dependencies of the entrypoint (including itself).
On 2014/09/23 19:47:45, Bob Nystrom wrote:
> Should update this since now it's not always all the deps.

Done.

[Bob Nystrom, 2014-09-23 21:49:32]

lgtm

[nweiz, 2014-09-23 22:27:08]

Committed patchset #3 (id:40001) manually as 40612 (presubmit successful).