Add “pub cache repair” to forcibly re-install previously cached packages.

Add “pub cache repair” to forcibly re-install previously cached packages.

BUG=https://code.google.com/p/dart/issues/detail?id=5391
RELNOTE=Add "pub cache repair" command to re-install cached packages.
R=nweiz@google.com

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

[Bob Nystrom, 2014-04-10 17:57:17]

Hi, Sharon! Any chance we can get some docs for this?

It's possible for packages in your pub cache to get changed or broken. A common
occurrence is that a user accidentally walks into one of the symlinks in their
"packages" directory and modifies a package in there. They don't realize it, but
that touches the canonical copy of that package in their system cache.

The usual workaround to fix that is to just nuke your .pub-cache directory, but
that's pretty painful. This adds a command that will just do a clean reinstall
of all hosted and git packages that are in your system cache.

[nweiz, 2014-04-10 20:24:33]

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

https://codereview.chromium.org/228703006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/cache_repair.dart:40: // TODO(rnystrom):
Allow specifying the server.
It's weird that this only repairs packages from the default server, despite also
repairing packages from a completely separate source.

https://codereview.chromium.org/228703006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/cache_repair.dart:41: var packages =
source.getCachedPackages();
Sort these by package name before downloading.

https://codereview.chromium.org/228703006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/cache_repair.dart:45: return
cache.download(id, force: true);
Add error-handling here. An individual failed download shouldn't cause the
entire process to crash; it should be reported in some user-friendly way at the
end. For example, a user may have a tombstoned package version, or some packages
from a localhost server that's no longer running.

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

https://codereview.chromium.org/228703006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/git.dart:134: .where((entry) =>
fileExists(path.join(entry, "pubspec.yaml")))
I don't like detecting "pubspec.yaml" here; it leaves a way the package could be
corrupted that we would silently ignore. I kind of like just explicitly
excluding "cache"; that way a completely empty directory will produce visible
error output, even if we can't resync it. Alternately, you could detect a ".git"
directory.

https://codereview.chromium.org/228703006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/git.dart:158: });
Gracefully handle errors here as well.

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

https://codereview.chromium.org/228703006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/hosted.dart:133: List<Package>
getCachedPackages([String url]) {
Since this takes an additional argument relative to the superclass, it should
have some documentation of its own.

https://codereview.chromium.org/228703006/diff/1/sdk/lib/_internal/pub/test/c...
File sdk/lib/_internal/pub/test/cache/repair/reinstalls_git_packages_test.dart
(right):

https://codereview.chromium.org/228703006/diff/1/sdk/lib/_internal/pub/test/c...
sdk/lib/_internal/pub/test/cache/repair/reinstalls_git_packages_test.dart:39:
.where((dir) => path.basename(dir).startsWith("foo-")).toList();
There's a cleaner way to do this. See [gitPackageRevisionCacheDir] in
pub/test/descriptor.dart.

https://codereview.chromium.org/228703006/diff/1/sdk/lib/_internal/pub/test/c...
sdk/lib/_internal/pub/test/cache/repair/reinstalls_git_packages_test.dart:48: //
TODO(bob): Output for git?
I don't understand this TODO.

https://codereview.chromium.org/228703006/diff/1/sdk/lib/_internal/pub/test/c...
sdk/lib/_internal/pub/test/cache/repair/reinstalls_git_packages_test.dart:48: //
TODO(bob): Output for git?
I don't understand this TODO.

https://codereview.chromium.org/228703006/diff/1/sdk/lib/_internal/pub/test/c...
sdk/lib/_internal/pub/test/cache/repair/reinstalls_git_packages_test.dart:57:
var fooLibs = fooDirs.map((dir) {
Rather than doing contortions here to validate the cache directory, why not just
validate the packages dir?

[Bob Nystrom, 2014-04-14 23:16:00]

Thanks!

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

https://codereview.chromium.org/228703006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/cache_repair.dart:40: // TODO(rnystrom):
Allow specifying the server.
On 2014/04/10 20:24:33, nweiz wrote:
> It's weird that this only repairs packages from the default server, despite
also
> repairing packages from a completely separate source.

Done.

https://codereview.chromium.org/228703006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/cache_repair.dart:41: var packages =
source.getCachedPackages();
On 2014/04/10 20:24:33, nweiz wrote:
> Sort these by package name before downloading.

Done.

https://codereview.chromium.org/228703006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/cache_repair.dart:45: return
cache.download(id, force: true);
On 2014/04/10 20:24:33, nweiz wrote:
> Add error-handling here. An individual failed download shouldn't cause the
> entire process to crash; it should be reported in some user-friendly way at
the
> end. For example, a user may have a tombstoned package version, or some
packages
> from a localhost server that's no longer running.

Done.

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

https://codereview.chromium.org/228703006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/git.dart:134: .where((entry) =>
fileExists(path.join(entry, "pubspec.yaml")))
On 2014/04/10 20:24:33, nweiz wrote:
> I don't like detecting "pubspec.yaml" here; it leaves a way the package could
be
> corrupted that we would silently ignore. I kind of like just explicitly
> excluding "cache"; that way a completely empty directory will produce visible
> error output, even if we can't resync it.

I thought about that, but I was worried that either future versions of pub, or
other software could leave files/dirs in there that we don't know about (think
.DS_Store, .svn, etc. though those specific examples would get caught by
listDir).

> Alternately, you could detect a ".git" directory.

Good idea. Done.

https://codereview.chromium.org/228703006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/git.dart:158: });
On 2014/04/10 20:24:33, nweiz wrote:
> Gracefully handle errors here as well.

Done.

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

https://codereview.chromium.org/228703006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/hosted.dart:133: List<Package>
getCachedPackages([String url]) {
On 2014/04/10 20:24:33, nweiz wrote:
> Since this takes an additional argument relative to the superclass, it should
> have some documentation of its own.

Reorganized the code a bit to fix this.

https://codereview.chromium.org/228703006/diff/1/sdk/lib/_internal/pub/test/c...
File sdk/lib/_internal/pub/test/cache/repair/reinstalls_git_packages_test.dart
(right):

https://codereview.chromium.org/228703006/diff/1/sdk/lib/_internal/pub/test/c...
sdk/lib/_internal/pub/test/cache/repair/reinstalls_git_packages_test.dart:39:
.where((dir) => path.basename(dir).startsWith("foo-")).toList();
On 2014/04/10 20:24:33, nweiz wrote:
> There's a cleaner way to do this. See [gitPackageRevisionCacheDir] in
> pub/test/descriptor.dart.

Maybe I'm missing something, but I don't think that helps. To delete stuff, you
need an actual path, not a descriptor.

https://codereview.chromium.org/228703006/diff/1/sdk/lib/_internal/pub/test/c...
sdk/lib/_internal/pub/test/cache/repair/reinstalls_git_packages_test.dart:48: //
TODO(bob): Output for git?
On 2014/04/10 20:24:33, nweiz wrote:
> I don't understand this TODO.

Oops. Leftover note to myself. "TODO(rnystrom)" is todos that get checked in,
"TODO(bob)" is stuff I intend to fix before I send it out for review.

Removed.

https://codereview.chromium.org/228703006/diff/1/sdk/lib/_internal/pub/test/c...
sdk/lib/_internal/pub/test/cache/repair/reinstalls_git_packages_test.dart:57:
var fooLibs = fooDirs.map((dir) {
On 2014/04/10 20:24:33, nweiz wrote:
> Rather than doing contortions here to validate the cache directory, why not
just
> validate the packages dir?

It only has the one version of foo, and I want to ensure both commits in the
cache were fixed.

[nweiz, 2014-04-15 00:59:26]

lgtm

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

https://codereview.chromium.org/228703006/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/source/git.dart:157: log.error("Failed to reset
${log.bold(package.name)} "
We should probably only have this nice handling for GitExceptions. Unexpected
exceptions should be reported as such.

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

https://codereview.chromium.org/228703006/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/source/hosted.dart:126: return _download(url,
package.name, package.version, package.dir).then((_) {
Long line.

https://codereview.chromium.org/228703006/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/source/hosted.dart:131: "${package.version} from
$url. Error:\n$error");
I'd omit "from $url" if it's pub.dartlang.org.

[Bob Nystrom, 2014-04-16 17:04:17]

Committed patchset #2 manually as r35111 (presubmit successful).

[Bob Nystrom, 2014-04-16 17:07:44]

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

https://codereview.chromium.org/228703006/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/source/git.dart:157: log.error("Failed to reset
${log.bold(package.name)} "
On 2014/04/15 00:59:26, nweiz wrote:
> We should probably only have this nice handling for GitExceptions. Unexpected
> exceptions should be reported as such.

Done.

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

https://codereview.chromium.org/228703006/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/source/hosted.dart:126: return _download(url,
package.name, package.version, package.dir).then((_) {
On 2014/04/15 00:59:26, nweiz wrote:
> Long line.

Done.

https://codereview.chromium.org/228703006/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/source/hosted.dart:131: "${package.version} from
$url. Error:\n$error");
On 2014/04/15 00:59:26, nweiz wrote:
> I'd omit "from $url" if it's http://pub.dartlang.org.

Done.