Gracefully handle pubspecs with dependencies using unknown sources.

Gracefully handle pubspecs with dependencies using unknown sources.

BUG=https://code.google.com/p/dart/issues/detail?id=10190
R=nweiz@google.com

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

[Bob Nystrom, 2013-05-17 22:30:31]

This makes PackageName (et. al.) just store the source name instead of a
reference to the object. That in turn lets pub parse pubspecs that use sources
it doesn't understand.

They aren't validated until the version solver tries to use a package. At that
point, it will make sure all of its dependencies have valid sources and fail if
not.

Backtracking can proceed past that, which effectively means that versions of
packages containing deps with bad sources are ignored.

[nweiz, 2013-05-17 23:55:55]

I don't see any tests for the lockfile referencing an unknown source, or for any
of the caching stuff that you added.

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/entrypoint.dart (left):

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:67: /// `id.source.shouldCache` is
true).
I'd say something like "if the package's source has [Source.shouldCache] as
true" rather than deleting this entirely.

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/lock_file.dart (right):

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/lock_file.dart:72: if
(sources.contains(sourceName)) {
This is already checked on l61. Did you mean to remove the previous check? Are
you missing a test that would catch this?

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/solver/version_solver.dart (right):

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/solver/version_solver.dart:75: /// The already
requested cached version lists.
"already-requested" (also below)

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/solver/version_solver.dart:89: final
_pendingPubspecs = new Map<PackageId, Future<Pubspec>>();
I don't understand why this change is part of this CL.

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/solver/version_solver.dart:148:
Future<List<PackageId>> getVersions(PackageDep package) {
Why is this taking a PackageDep now? Why is the version constraint relevant?

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/solver/version_solver.dart:180: return ids;
Right now you're returning `ids` for the first future that gets returned for a
[getVersions] call, but if it gets called again and hits [_pendingVersions]
you're returning `versions` instead.

I'm worried that the tests don't catch this.

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/source.dart (right):

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/source.dart:85: Future<Pubspec> describe(PackageId
id) {
I don't understand why the describe/describeUncached change is part of this CL
either.

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/source.dart:87: if (id.source != name) throw new
ArgumentError(
Use {} if the value is smaller than a single line.

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/source.dart:88: "Package $id does not use this
source.");
Include the name of this source.

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/source.dart:106: /// already in the system cache.
For cached sources, by default this uses
I learned recently that docstrings are supposed to begin with a single-sentence
paragraph.

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/source.dart:108: /// implementation for non-cached
sources; they must implement it manually.
Mention that this is intended to be a protected method, and that people should
call [describe] instead.

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/system_cache.dart (right):

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/system_cache.dart:79: var source =
sources[id.source];
What if id.source is an invalid name?

https://codereview.chromium.org/15347004/diff/7001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.chromium.org/15347004/diff/7001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:86: source =
cache.sources[id.source];
What if id.source is an invalid name?

[Bob Nystrom, 2013-05-20 20:06:00]

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/entrypoint.dart (left):

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:67: /// `id.source.shouldCache` is
true).
On 2013/05/17 23:55:55, nweiz wrote:
> I'd say something like "if the package's source has [Source.shouldCache] as
> true" rather than deleting this entirely.

Done.

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/lock_file.dart (right):

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/lock_file.dart:72: if
(sources.contains(sourceName)) {
On 2013/05/17 23:55:55, nweiz wrote:
> This is already checked on l61. Did you mean to remove the previous check? Are
> you missing a test that would catch this?

Removed the previous check and added a test that catches this.

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/solver/version_solver.dart (right):

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/solver/version_solver.dart:75: /// The already
requested cached version lists.
On 2013/05/17 23:55:55, nweiz wrote:
> "already-requested" (also below)

Done.

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/solver/version_solver.dart:89: final
_pendingPubspecs = new Map<PackageId, Future<Pubspec>>();
On 2013/05/17 23:55:55, nweiz wrote:
> I don't understand why this change is part of this CL.

The behavior here was always the intended behavior. You shouldn't have multiple
concurrent requests for the same pubspec or version list in flight at the same
time. It was a bug that it didn't do that before.

However, the implementation of the solver, or perhaps the specific tests
themselves happened to never actually kick off a concurrent request and tickle
this bug.

When I moved things around for this patch, that was no longer the case and the
existing bug surfaced. If I recall, this is because _traversePackage calls
Future.wait() to get the lists of versions for each dep in parallel.

But now that I look, that shouldn't cause a concurrent request though because
you shouldn't have the same dep in there twice. I know adding this fixed an
issue, but I'm not sure why/how.

I just tore the code back out and everything still seems fine.

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/solver/version_solver.dart:148:
Future<List<PackageId>> getVersions(PackageDep package) {
On 2013/05/17 23:55:55, nweiz wrote:
> Why is this taking a PackageDep now? Why is the version constraint relevant?

That was related to another change I ended up abandoning. Put it back.

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/solver/version_solver.dart:180: return ids;
On 2013/05/17 23:55:55, nweiz wrote:
> Right now you're returning `ids` for the first future that gets returned for a
> [getVersions] call, but if it gets called again and hits [_pendingVersions]
> you're returning `versions` instead.
> 
> I'm worried that the tests don't catch this.

Yeah, this code is clearly wrong. Removed.

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/source.dart (right):

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/source.dart:85: Future<Pubspec> describe(PackageId
id) {
On 2013/05/17 23:55:55, nweiz wrote:
> I don't understand why the describe/describeUncached change is part of this CL
> either.

This moves the cache checking into Source from SystemCache. We want to make sure
we use cached pubspecs whenever possible when describing packages. In the
solver, there was at least one place where the SystemCache wasn't handy, so I
thought it made the most sense to just move that logic directly into Source. I
think I also found a place that was inadvertently not checking the cache first
because it just called describe() directly on the source. This makes that less
likely.

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/source.dart:87: if (id.source != name) throw new
ArgumentError(
On 2013/05/17 23:55:55, nweiz wrote:
> Use {} if the value is smaller than a single line.

Done.

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/source.dart:88: "Package $id does not use this
source.");
On 2013/05/17 23:55:55, nweiz wrote:
> Include the name of this source.

Done.

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/source.dart:106: /// already in the system cache.
For cached sources, by default this uses
On 2013/05/17 23:55:55, nweiz wrote:
> I learned recently that docstrings are supposed to begin with a
single-sentence
> paragraph.

Done.

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/source.dart:108: /// implementation for non-cached
sources; they must implement it manually.
On 2013/05/17 23:55:55, nweiz wrote:
> Mention that this is intended to be a protected method, and that people should
> call [describe] instead.

Done.

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/system_cache.dart (right):

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/system_cache.dart:79: var source =
sources[id.source];
On 2013/05/17 23:55:55, nweiz wrote:
> What if id.source is an invalid name?

It will throw an ArgumentError. That would be a programmatic bug in pub: the
sources for all package IDs should have been validated by the solver before you
get here to actually install it.

[nweiz, 2013-05-20 23:54:05]

lgtm

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/source.dart (right):

https://codereview.chromium.org/15347004/diff/4001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/source.dart:85: Future<Pubspec> describe(PackageId
id) {
On 2013/05/20 20:06:00, Bob Nystrom wrote:
> On 2013/05/17 23:55:55, nweiz wrote:
> > I don't understand why the describe/describeUncached change is part of this
CL
> > either.
> 
> This moves the cache checking into Source from SystemCache. We want to make
sure
> we use cached pubspecs whenever possible when describing packages. In the
> solver, there was at least one place where the SystemCache wasn't handy, so I
> thought it made the most sense to just move that logic directly into Source. I
> think I also found a place that was inadvertently not checking the cache first
> because it just called describe() directly on the source. This makes that less
> likely.

It would be nice if this sort of change were separated out into a different CL
in the future.

[Bob Nystrom, 2013-05-21 00:09:32]

Committed patchset #6 manually as r22913 (presubmit successful).