Issue 12079112: Make a bunch of stuff in pub synchronous.

Issue 12079112: Make a bunch of stuff in pub synchronous. (Closed)

Created:
7 years, 10 months ago by Bob Nystrom

Modified:
7 years, 10 months ago

Reviewers:
nweiz

CC:
reviews_dartlang.org

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

More Reviews

Description

Make a bunch of stuff in pub synchronous. Committed: https://code.google.com/p/dart/source/detail?r=18019

Patch Set 3 : Fix after merging with latest. #

Patch Set 4 : Fix after merge. #

Created: 7 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+369 lines, -463 lines)			Patch
M	utils/pub/command_lish.dart	View	1 2	3 chunks	+12 lines, -25 lines	0 comments	Download
M	utils/pub/command_uploader.dart	View		1 chunk	+2 lines, -3 lines	0 comments	Download
M	utils/pub/entrypoint.dart	View	1 2	10 chunks	+42 lines, -55 lines	0 comments	Download
M	utils/pub/error_group.dart	View	1 2	2 chunks	+3 lines, -1 line	0 comments	Download
M	utils/pub/git_source.dart	View	1 2	3 chunks	+12 lines, -14 lines	0 comments	Download
M	utils/pub/io.dart	View	1 2	9 chunks	+50 lines, -118 lines	0 comments	Download
M	utils/pub/oauth2.dart	View	1 2	5 chunks	+33 lines, -38 lines	0 comments	Download
M	utils/pub/package.dart	View	1 2	2 chunks	+24 lines, -25 lines	0 comments	Download
M	utils/pub/pub.dart	View	1 2	1 chunk	+23 lines, -32 lines	0 comments	Download
M	utils/pub/sdk_source.dart	View	1	3 chunks	+10 lines, -9 lines	0 comments	Download
M	utils/pub/source.dart	View	1 2	2 chunks	+10 lines, -8 lines	0 comments	Download
M	utils/pub/system_cache.dart	View	1 2	3 chunks	+6 lines, -5 lines	0 comments	Download
M	utils/pub/utils.dart	View	1 2	1 chunk	+10 lines, -0 lines	0 comments	Download
M	utils/pub/validator/compiled_dartdoc.dart	View	1	1 chunk	+12 lines, -13 lines	0 comments	Download
M	utils/pub/validator/dependency.dart	View		2 chunks	+15 lines, -16 lines	0 comments	Download
M	utils/pub/validator/directory.dart	View		2 chunks	+22 lines, -23 lines	0 comments	Download
M	utils/pub/validator/lib.dart	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
M	utils/pub/validator/name.dart	View	1	2 chunks	+3 lines, -2 lines	0 comments	Download
M	utils/tests/pub/io_test.dart	View		6 chunks	+45 lines, -38 lines	0 comments	Download
M	utils/tests/pub/test_pub.dart	View	1 2	7 chunks	+33 lines, -36 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

nweiz

https://codereview.chromium.org/12079112/diff/1/utils/pub/command_lish.dart File utils/pub/command_lish.dart (right): https://codereview.chromium.org/12079112/diff/1/utils/pub/command_lish.dart#newcode119 utils/pub/command_lish.dart:119: return git.isInstalled.then((results) { "results" -> "gitInstalled" It's worrisome that ...

7 years, 10 months ago (2013-02-01 02:05:55 UTC) #2

Bob Nystrom

Thanks! https://codereview.chromium.org/12079112/diff/1/utils/pub/command_lish.dart File utils/pub/command_lish.dart (right): https://codereview.chromium.org/12079112/diff/1/utils/pub/command_lish.dart#newcode119 utils/pub/command_lish.dart:119: return git.isInstalled.then((results) { On 2013/02/01 02:05:55, nweiz wrote: ...

7 years, 10 months ago (2013-02-01 23:17:21 UTC) #3

Thanks!

https://codereview.chromium.org/12079112/diff/1/utils/pub/command_lish.dart
File utils/pub/command_lish.dart (right):

https://codereview.chromium.org/12079112/diff/1/utils/pub/command_lish.dart#n...
utils/pub/command_lish.dart:119: return git.isInstalled.then((results) {
On 2013/02/01 02:05:55, nweiz wrote:
> "results" -> "gitInstalled"
> 
> It's worrisome that this didn't show up in our tests.

It did. :( pub_lish_test is flaky so you don't see it when it fails. :(

I'll get it passing locally and then try to get that status fixed soon.

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

https://codereview.chromium.org/12079112/diff/1/utils/pub/entrypoint.dart#new...
utils/pub/entrypoint.dart:42: final _installs = new Map<PackageId,
Future<PackageId>>();
On 2013/02/01 02:05:55, nweiz wrote:
> Can't we make this a Map literal, or are there still weird restrictions on
> those?

Map literals can only have string keys. Seems a bit silly to have that
restriction even for empty maps, but <shrug>.

https://codereview.chromium.org/12079112/diff/1/utils/pub/entrypoint.dart#new...
utils/pub/entrypoint.dart:99: // Make sure all errors propagate through future.
On 2013/02/01 02:05:55, nweiz wrote:
> I don't want to have to leave this comment on every use of defer(). We should
> just mention this use case in the documentation of defer() itself.

Done.

https://codereview.chromium.org/12079112/diff/1/utils/pub/entrypoint.dart#new...
utils/pub/entrypoint.dart:220: if (basename(file) == 'packages' ||
!dirExists(file)) return [];
On 2013/02/01 02:05:55, nweiz wrote:
> Moving the "packages" check in here changes the behavior so that "packages"
> directories (although not their subfiles) are returned.

Fixed.

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

https://codereview.chromium.org/12079112/diff/1/utils/pub/io.dart#newcode79
utils/pub/io.dart:79: return file;
On 2013/02/01 02:05:55, nweiz wrote:
> "return file..writeAsStringSync(contents)"? Is that too cute?

Eh, I'll try it. Let's see if it grows on us.

https://codereview.chromium.org/12079112/diff/1/utils/pub/oauth2.dart
File utils/pub/oauth2.dart (right):

https://codereview.chromium.org/12079112/diff/1/utils/pub/oauth2.dart#newcode115
utils/pub/oauth2.dart:115: if (_credentials != null) return _credentials;
On 2013/02/01 02:05:55, nweiz wrote:
> Why no more logging?

Two reasons:

1. The logging is most helpful when asynchrony has made us lose state. The more
sync we have, the more useful the callstack is. So I think we can ditch some of
the logging.

2. The current logs seem a bit more verbose than is useful. I littered a bunch
of logging in, but it needs some tuning, so I figured I'd dial it back a bit.

https://codereview.chromium.org/12079112/diff/1/utils/pub/oauth2.dart#newcode119
utils/pub/oauth2.dart:119: if (!credentialsExist) return;
On 2013/02/01 02:05:55, nweiz wrote:
> Style nit: merge this and the previous line.

Done.

https://codereview.chromium.org/12079112/diff/1/utils/pub/oauth2.dart#newcode122
utils/pub/oauth2.dart:122: readTextFile(_credentialsFile(cache)));
On 2013/02/01 02:05:55, nweiz wrote:
> "_credentialsFile(cache)" -> "path"

Done.

https://codereview.chromium.org/12079112/diff/1/utils/pub/package.dart
File utils/pub/package.dart (right):

https://codereview.chromium.org/12079112/diff/1/utils/pub/package.dart#newcode39
utils/pub/package.dart:39: factory Package(String name, String packageDir,
SourceRegistry sources) {
On 2013/02/01 02:05:55, nweiz wrote:
> It may still be correct to call this "Package.load", to indicate that it's
doing
> IO, even if it's not async anymore.

I'm personally OK with that being implied by the fact that it takes a path in
its constructor. I think as we use sync IO more, we don't need to treat it as
special.

https://codereview.chromium.org/12079112/diff/1/utils/pub/pub.dart
File utils/pub/pub.dart (right):

https://codereview.chromium.org/12079112/diff/1/utils/pub/pub.dart#newcode241
utils/pub/pub.dart:241: future = defer(() => new Entrypoint(path.current,
cache));
On 2013/02/01 02:05:55, nweiz wrote:
> I think this slightly arcane control flow is an artifact of pre-libv2 APIs.
> Suggest:
> 
>     defer(() {
>       if (requiresEntrypoint) {
>         // TODO(rnystrom): Will eventually need better logic to walk up
>         // subdirectories until we hit one that looks package-like. For now,
>         // just assume the cwd is it.
>         this.entrypoint = Entrypoint.load(path.current, cache);
>       }
>       try {
>         var commandFuture = onRun();
>         if (commandFuture == null) return true;
> 
>         return commandFuture;
>       } catch (error, trace) {
>         handleError(error, trace);
>       }
>     }).then((_) => cache_.deleteTempDir()).catchError((asyncError) {
>       var e = asyncError.error;
>       if (e is PubspecNotFoundException && e.name == null) {
>         e = 'Could not find a file named "pubspec.yaml" in the directory '
>           '${path.current}.';
>       } else if (e is PubspecHasNoNameException && e.name == null) {
>         e = 'pubspec.yaml is missing the required "name" field (e.g. "name: '
>           '${basename(path.current)}").';
>       }
> 
>       handleError(e, asyncError.stackTrace);
>     }).then((_) {
>       // Explicitly exit on success to ensure that any dangling dart:io
handles
>       // don't cause the process to never terminate.
>       exit(0);
>     });

Beautiful. Tweaked it a bit more, but that's great.

https://codereview.chromium.org/12079112/diff/1/utils/pub/utils.dart
File utils/pub/utils.dart (right):

https://codereview.chromium.org/12079112/diff/1/utils/pub/utils.dart#newcode108
utils/pub/utils.dart:108: /// to the result of [callback].
On 2013/02/01 02:05:55, nweiz wrote:
> Move all those comments about error-capturing in here.

Done.

lgtm

Expand Messages | Collapse Messages