Validate packages against their SDK constraints.

Validate packages against their SDK constraints.

After running pub install or update, it will look at all of the
selected packages and see if their SDK constraints match the 
current SDK. If not, it will show a (hopefully) helpful error
message.
BUG=dartbug.com/6285

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

[Bob Nystrom, 2013-01-31 00:50:26]

This will throw output at you like:

Some packages are not compatible with SDK version 0.1.2+3:
- 'myapp' requires >2.0.0
- 'foo' requires >0.1.3

You may be able to resolve this by upgrading to the latest Dart SDK
or adding a version constraint to use an older version of a package.

[nweiz, 2013-01-31 22:38:53]

https://codereview.chromium.org/12092080/diff/1022/utils/pub/entrypoint.dart
File utils/pub/entrypoint.dart (right):

https://codereview.chromium.org/12092080/diff/1022/utils/pub/entrypoint.dart#...
utils/pub/entrypoint.dart:146: Future<List<Package>> walkDependencies() {
Maybe this and validateSdkConstraints should be methods on LockFile? It weirds
me out that they have this weird "should only be called" requirement.

https://codereview.chromium.org/12092080/diff/1022/utils/pub/entrypoint.dart#...
utils/pub/entrypoint.dart:190: "${sdk.version}:\n${errors.join('\n')}\n\n"
When I'm formatting multiline strings like this, I usually add an additional
four spaces of indentation when there's a line break in the source but no line
break in the output. I'd also probably add a source newline after the first
"\n". For example:

    log.error("Some packages are not compatible with SDK version "
            "${sdk.version}:\n"
        "${errors.join('\n')}\n\n"
        "You may be able to resolve this by upgrading to the latest Dart "
            "SDK\n"
        "or adding a version constraint to use an older version of a "
            "package.");

Also, maybe "with SDK version" -> "with your SDK version".

https://codereview.chromium.org/12092080/diff/1022/utils/pub/git_source.dart
File utils/pub/git_source.dart (right):

https://codereview.chromium.org/12092080/diff/1022/utils/pub/git_source.dart#...
utils/pub/git_source.dart:63: Future<String> systemCacheDirectory(PackageId id)
=> _revisionCachePath(id);
Why not just rename "_revisionCachePath" to "systemCacheDirectory"?

https://codereview.chromium.org/12092080/diff/1022/utils/pub/io.dart
File utils/pub/io.dart (right):

https://codereview.chromium.org/12092080/diff/1022/utils/pub/io.dart#newcode91
utils/pub/io.dart:91: String readTextFileSync(file) {
I'm not a big fan of this intermediate duplication, but since you're working on
making it all sync right now whatever.

https://codereview.chromium.org/12092080/diff/1022/utils/pub/system_cache.dart
File utils/pub/system_cache.dart (right):

https://codereview.chromium.org/12092080/diff/1022/utils/pub/system_cache.dar...
utils/pub/system_cache.dart:65: Future<Package> describe(PackageId id) {
I don't like that this returns a Package whereas other describe() methods return
Pubspecs. It looks to me like you could make walkDependencies work with Pubspecs
rather than Packages, so I'd do that.

https://codereview.chromium.org/12092080/diff/1022/utils/pub/system_cache.dar...
utils/pub/system_cache.dart:68: return id.source.describe(id).then((pubspec) {
"id.source.describe(id)" -> "id.describe()"

https://codereview.chromium.org/12092080/diff/1022/utils/pub/system_cache.dar...
utils/pub/system_cache.dart:75: return
id.source.systemCacheDirectory(id).then((packageDir) {
This would be a little cleaner with an "id.systemCacheDirectory" getter.

https://codereview.chromium.org/12092080/diff/1022/utils/tests/pub/sdk_constr...
File utils/tests/pub/sdk_constraint_test.dart (right):

https://codereview.chromium.org/12092080/diff/1022/utils/tests/pub/sdk_constr...
utils/tests/pub/sdk_constraint_test.dart:14: var success = "Dependencies
installed!\$";
I'd make these RegExps, and use r""-style strings.

[Bob Nystrom, 2013-02-01 16:53:29]

Dang, forgot to send this out yesterday. :(

https://codereview.chromium.org/12092080/diff/1022/utils/pub/entrypoint.dart
File utils/pub/entrypoint.dart (right):

https://codereview.chromium.org/12092080/diff/1022/utils/pub/entrypoint.dart#...
utils/pub/entrypoint.dart:146: Future<List<Package>> walkDependencies() {
On 2013/01/31 22:38:53, nweiz wrote:
> Maybe this and validateSdkConstraints should be methods on LockFile?

I put it here because walking the locked transitive deps is really only
meaningful for the root package. Seemed like it made sense here. LockFile is
really just the state for the file itself and not the root package that owns it.

> It weirds me out that they have this weird "should only be called"
requirement.

Yeah, I'm not crazy about that either. Before when this was a separate command,
it would throw an error if an entry wasn't in the lock file. For now, I think
the comment is enough?

https://codereview.chromium.org/12092080/diff/1022/utils/pub/entrypoint.dart#...
utils/pub/entrypoint.dart:190: "${sdk.version}:\n${errors.join('\n')}\n\n"
On 2013/01/31 22:38:53, nweiz wrote:
> When I'm formatting multiline strings like this, I usually add an additional
> four spaces of indentation when there's a line break in the source but no line
> break in the output. I'd also probably add a source newline after the first
> "\n". For example:
> 
>     log.error("Some packages are not compatible with SDK version "
>             "${sdk.version}:\n"
>         "${errors.join('\n')}\n\n"
>         "You may be able to resolve this by upgrading to the latest Dart "
>             "SDK\n"
>         "or adding a version constraint to use an older version of a "
>             "package.");
> 
> Also, maybe "with SDK version" -> "with your SDK version".

Done and done.

https://codereview.chromium.org/12092080/diff/1022/utils/pub/git_source.dart
File utils/pub/git_source.dart (right):

https://codereview.chromium.org/12092080/diff/1022/utils/pub/git_source.dart#...
utils/pub/git_source.dart:63: Future<String> systemCacheDirectory(PackageId id)
=> _revisionCachePath(id);
On 2013/01/31 22:38:53, nweiz wrote:
> Why not just rename "_revisionCachePath" to "systemCacheDirectory"?

I kept it because I like that it helps clarify the git-specific semantics of
that path.

https://codereview.chromium.org/12092080/diff/1022/utils/pub/io.dart
File utils/pub/io.dart (right):

https://codereview.chromium.org/12092080/diff/1022/utils/pub/io.dart#newcode91
utils/pub/io.dart:91: String readTextFileSync(file) {
On 2013/01/31 22:38:53, nweiz wrote:
> I'm not a big fan of this intermediate duplication, but since you're working
on
> making it all sync right now whatever.

Yup. Once that patch lands, this will be gone.

https://codereview.chromium.org/12092080/diff/1022/utils/pub/system_cache.dart
File utils/pub/system_cache.dart (right):

https://codereview.chromium.org/12092080/diff/1022/utils/pub/system_cache.dar...
utils/pub/system_cache.dart:65: Future<Package> describe(PackageId id) {
On 2013/01/31 22:38:53, nweiz wrote:
> I don't like that this returns a Package whereas other describe() methods
return
> Pubspecs. It looks to me like you could make walkDependencies work with
Pubspecs
> rather than Packages, so I'd do that.

I had that originally, but Pubspec doesn't know how to find and read the file.
It's constructor just takes the contents of a pubspec file. So I had code here
that would locate the pubspec file, read it, and then create the pubspec.

That's exactly what Package's constructor does. So it seemed like Package was a
better object to use.

https://codereview.chromium.org/12092080/diff/1022/utils/pub/system_cache.dar...
utils/pub/system_cache.dart:68: return id.source.describe(id).then((pubspec) {
On 2013/01/31 22:38:53, nweiz wrote:
> "id.source.describe(id)" -> "id.describe()"

Done.

https://codereview.chromium.org/12092080/diff/1022/utils/pub/system_cache.dar...
utils/pub/system_cache.dart:75: return
id.source.systemCacheDirectory(id).then((packageDir) {
On 2013/01/31 22:38:53, nweiz wrote:
> This would be a little cleaner with an "id.systemCacheDirectory" getter.

Done.

https://codereview.chromium.org/12092080/diff/1022/utils/tests/pub/sdk_constr...
File utils/tests/pub/sdk_constraint_test.dart (right):

https://codereview.chromium.org/12092080/diff/1022/utils/tests/pub/sdk_constr...
utils/tests/pub/sdk_constraint_test.dart:14: var success = "Dependencies
installed!\$";
On 2013/01/31 22:38:53, nweiz wrote:
> I'd make these RegExps, and use r""-style strings.

Done.

[nweiz, 2013-02-01 22:24:54]

lgtm

https://codereview.chromium.org/12092080/diff/1022/utils/pub/entrypoint.dart
File utils/pub/entrypoint.dart (right):

https://codereview.chromium.org/12092080/diff/1022/utils/pub/entrypoint.dart#...
utils/pub/entrypoint.dart:146: Future<List<Package>> walkDependencies() {
On 2013/02/01 16:53:29, Bob Nystrom wrote:
> On 2013/01/31 22:38:53, nweiz wrote:
> > Maybe this and validateSdkConstraints should be methods on LockFile?
> 
> I put it here because walking the locked transitive deps is really only
> meaningful for the root package. Seemed like it made sense here. LockFile is
> really just the state for the file itself and not the root package that owns
it.
> 
> > It weirds me out that they have this weird "should only be called"
> requirement.
> 
> Yeah, I'm not crazy about that either. Before when this was a separate
command,
> it would throw an error if an entry wasn't in the lock file. For now, I think
> the comment is enough?

I'd like to at least get an exception if this is called incorrectly. Right now
it'll just use an empty lockfile and return [].

https://codereview.chromium.org/12092080/diff/1022/utils/pub/git_source.dart
File utils/pub/git_source.dart (right):

https://codereview.chromium.org/12092080/diff/1022/utils/pub/git_source.dart#...
utils/pub/git_source.dart:63: Future<String> systemCacheDirectory(PackageId id)
=> _revisionCachePath(id);
On 2013/02/01 16:53:29, Bob Nystrom wrote:
> On 2013/01/31 22:38:53, nweiz wrote:
> > Why not just rename "_revisionCachePath" to "systemCacheDirectory"?
> 
> I kept it because I like that it helps clarify the git-specific semantics of
> that path.

I don't like having two differently-named functions that do identical things.
I'd rather just document on this method that it's the part for the specific
revision.

https://codereview.chromium.org/12092080/diff/1022/utils/pub/system_cache.dart
File utils/pub/system_cache.dart (right):

https://codereview.chromium.org/12092080/diff/1022/utils/pub/system_cache.dar...
utils/pub/system_cache.dart:65: Future<Package> describe(PackageId id) {
On 2013/02/01 16:53:29, Bob Nystrom wrote:
> On 2013/01/31 22:38:53, nweiz wrote:
> > I don't like that this returns a Package whereas other describe() methods
> return
> > Pubspecs. It looks to me like you could make walkDependencies work with
> Pubspecs
> > rather than Packages, so I'd do that.
> 
> I had that originally, but Pubspec doesn't know how to find and read the file.
> It's constructor just takes the contents of a pubspec file. So I had code here
> that would locate the pubspec file, read it, and then create the pubspec.
> 
> That's exactly what Package's constructor does. So it seemed like Package was
a
> better object to use.

I think it's fine to use Package internally to load the pubspec, but I don't
think it makes sense to return it. It's not useful for the known use-case, and
it's inconsistent with other methods called "describe".

If you really feel like this should return a package, at least rename it load()
or something like that.

[Bob Nystrom, 2013-02-02 00:02:31]

Accidentally submitted this so I've made your suggestions here:
https://codereview.chromium.org/12171002

https://codereview.chromium.org/12092080/diff/1022/utils/pub/entrypoint.dart
File utils/pub/entrypoint.dart (right):

https://codereview.chromium.org/12092080/diff/1022/utils/pub/entrypoint.dart#...
utils/pub/entrypoint.dart:146: Future<List<Package>> walkDependencies() {
On 2013/02/01 22:24:54, nweiz wrote:
> On 2013/02/01 16:53:29, Bob Nystrom wrote:
> > On 2013/01/31 22:38:53, nweiz wrote:
> > > Maybe this and validateSdkConstraints should be methods on LockFile?
> > 
> > I put it here because walking the locked transitive deps is really only
> > meaningful for the root package. Seemed like it made sense here. LockFile is
> > really just the state for the file itself and not the root package that owns
> it.
> > 
> > > It weirds me out that they have this weird "should only be called"
> > requirement.
> > 
> > Yeah, I'm not crazy about that either. Before when this was a separate
> command,
> > it would throw an error if an entry wasn't in the lock file. For now, I
think
> > the comment is enough?
> 
> I'd like to at least get an exception if this is called incorrectly. Right now
> it'll just use an empty lockfile and return [].

It should fail if called with an out of date lockfile. It will try to look up a
dependency in it and get null back and then barf on that.

https://codereview.chromium.org/12092080/diff/1022/utils/pub/git_source.dart
File utils/pub/git_source.dart (right):

https://codereview.chromium.org/12092080/diff/1022/utils/pub/git_source.dart#...
utils/pub/git_source.dart:63: Future<String> systemCacheDirectory(PackageId id)
=> _revisionCachePath(id);
On 2013/02/01 22:24:54, nweiz wrote:
> On 2013/02/01 16:53:29, Bob Nystrom wrote:
> > On 2013/01/31 22:38:53, nweiz wrote:
> > > Why not just rename "_revisionCachePath" to "systemCacheDirectory"?
> > 
> > I kept it because I like that it helps clarify the git-specific semantics of
> > that path.
> 
> I don't like having two differently-named functions that do identical things.
> I'd rather just document on this method that it's the part for the specific
> revision.

Done.

https://codereview.chromium.org/12092080/diff/1022/utils/pub/system_cache.dart
File utils/pub/system_cache.dart (right):

https://codereview.chromium.org/12092080/diff/1022/utils/pub/system_cache.dar...
utils/pub/system_cache.dart:65: Future<Package> describe(PackageId id) {
On 2013/02/01 22:24:54, nweiz wrote:
> On 2013/02/01 16:53:29, Bob Nystrom wrote:
> > On 2013/01/31 22:38:53, nweiz wrote:
> > > I don't like that this returns a Package whereas other describe() methods
> > return
> > > Pubspecs. It looks to me like you could make walkDependencies work with
> > Pubspecs
> > > rather than Packages, so I'd do that.
> > 
> > I had that originally, but Pubspec doesn't know how to find and read the
file.
> > It's constructor just takes the contents of a pubspec file. So I had code
here
> > that would locate the pubspec file, read it, and then create the pubspec.
> > 
> > That's exactly what Package's constructor does. So it seemed like Package
was
> a
> > better object to use.
> 
> I think it's fine to use Package internally to load the pubspec, but I don't
> think it makes sense to return it. It's not useful for the known use-case, and
> it's inconsistent with other methods called "describe".
> 
> If you really feel like this should return a package, at least rename it
load()
> or something like that.

Done.