Add a "pub deploy" command.

Add a "pub deploy" command.

BUG=6006
R=amouravski@google.com

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

[Andrei Mouravski, 2013-05-10 16:19:57]

Will review more later today after a few errands.

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

https://codereview.chromium.org/14914007/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:227: /// Returns a list of files
that are considered to be part of this package. If
Add a note that these paths are relative to the working directory.

https://codereview.chromium.org/14914007/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:227: /// Returns a list of files
that are considered to be part of this package. If
Doc comment style is to have the first line be its own paragraph.
http://www.dartlang.org/articles/doc-comment-guidelines/#first-paragraph-a-on...

https://codereview.chromium.org/14914007/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:232: Future<List<String>>
packageFiles({String beneath}) {
We've talked briefly about a "pub" Pub package, and I want to bring that up
again, because I think specifically this method would be extremely useful in
many different tooling cases where you really want to just get the list of files
in a package.

I'd very much prefer to have one good implementation from the pub team than a
dozen bad implementations that go stale when Pub changes, etc.

If all you do is make this one method available, then I'll be happy. :]

https://codereview.chromium.org/14914007/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:232: Future<List<String>>
packageFiles({String beneath}) {
Also, how about a test for this?
Might be valuable to write a helper function to run that long git command and
test it separately and make this function smaller.

https://codereview.chromium.org/14914007/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:249: }).then((files) {
Why not
.then((files) => files.retainWhere((file) {
...
}));

[nweiz, 2013-05-10 19:12:55]

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

https://codereview.chromium.org/14914007/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:227: /// Returns a list of files
that are considered to be part of this package. If
On 2013/05/10 16:19:57, Andrei Mouravski wrote:
> Add a note that these paths are relative to the working directory.

All relative paths are implicitly relative to the working directory unless
specified otherwise. Also, there's no guarantee that this will return relative
paths.

https://codereview.chromium.org/14914007/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:227: /// Returns a list of files
that are considered to be part of this package. If
On 2013/05/10 16:19:57, Andrei Mouravski wrote:
> Doc comment style is to have the first line be its own paragraph.
>
http://www.dartlang.org/articles/doc-comment-guidelines/#first-paragraph-a-on...

Huh, I hadn't seen that before. That's not a style we follow generally in pub,
but I can certainly start transitioning to it. I've filed issue 10582 to convert
the docs more generally.

https://codereview.chromium.org/14914007/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:232: Future<List<String>>
packageFiles({String beneath}) {
On 2013/05/10 16:19:57, Andrei Mouravski wrote:
> We've talked briefly about a "pub" Pub package, and I want to bring that up
> again, because I think specifically this method would be extremely useful in
> many different tooling cases where you really want to just get the list of
files
> in a package.
> 
> I'd very much prefer to have one good implementation from the pub team than a
> dozen bad implementations that go stale when Pub changes, etc.
> 
> If all you do is make this one method available, then I'll be happy. :]

We're not ready yet to start exposing pub as a package. When we do, though, I'm
pretty confident that Entrypoint will be part of the public API.

https://codereview.chromium.org/14914007/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:232: Future<List<String>>
packageFiles({String beneath}) {
On 2013/05/10 16:19:57, Andrei Mouravski wrote:
> Also, how about a test for this?
> Might be valuable to write a helper function to run that long git command and
> test it separately and make this function smaller.

We generally don't write unit tests for pub, since there's no public API surface
and the internal APIs change frequently and have many interdependencies that
make them difficult to test. I think the integration tests adequately exercise
this code.

https://codereview.chromium.org/14914007/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:249: }).then((files) {
On 2013/05/10 16:19:57, Andrei Mouravski wrote:
> Why not
> .then((files) => files.retainWhere((file) {
> ...
> }));

I believe it's not Dart style to start multiple blocks on the same line. At
least, Bob tends to complain when I do it.

[Andrei Mouravski, 2013-05-10 21:25:01]

Some more to keep you going.

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

https://codereview.chromium.org/14914007/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:227: /// Returns a list of files
that are considered to be part of this package. If
On 2013/05/10 19:12:55, nweiz wrote:
> On 2013/05/10 16:19:57, Andrei Mouravski wrote:
> > Doc comment style is to have the first line be its own paragraph.
> >
>
http://www.dartlang.org/articles/doc-comment-guidelines/#first-paragraph-a-on...
> 
> Huh, I hadn't seen that before. That's not a style we follow generally in pub,
> but I can certainly start transitioning to it. I've filed issue 10582 to
convert
> the docs more generally.

I noticed that! It's okay, consistency is often more important, but if you're
writing a new file, use the style guide format, but if you're editing a lot of
existing code, use the old style to not confuse the CL.

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/command_deploy.dart (right):

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/command_deploy.dart:28: // TODO(nweiz): make these
configurable.
You should just do this now:

String _source = 'web';
void set source(String s) { _source = s }

Maybe add some path handling, too.

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/command_deploy.dart:91: log.warning(e.message);
Do you really want to log a warning here? Isn't this going to be triggered on
every non-main() dart file?

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/command_deploy.dart:101: /// the deploy command.
Add a note as to _why_ you need this.

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/command_deploy.dart:106: _entrypoints.map((file)
=> path.relative(file).length))
Why is this indented so far?

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/command_deploy.dart:120: /// If this package
depends non-transitively on the `browser` package, this
Is there any way to use a CDN's browser?

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/dart.dart (right):

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/dart.dart:5: library dart;
Could you write library level documentation? So people know what this library is
all about?

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/dart.dart:12: import 'package:pathos/path.dart' as
path;
How about a newline after packages.

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/dart.dart:13: import
'../../../compiler/compiler.dart' as api;
Let's call this compiler since "api" is not descriptive.
Or "dart2js".

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/dart.dart:32: Future<String> compile(String
entrypoint, {String packageRoot,
Is there a reason you can't just use the straight dart2js API for this? If it's
not good enough, that's feedback you should provide so they can fix it.

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/dart.dart:36: var options =
<String>['--categories=Client,Server', '--minify'];
What about logging? Where does that go?

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/dart.dart:56: /// directory in the repo and to the
root of the compiled SDK.
"in the repo" as in, "in the Dart SDK repository"?

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/dart.dart:64: // TODO(nweiz): this misses the case
where a Dart file doesn't contain main(),
What if the main is in an HTML file?

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/dart.dart:64: // TODO(nweiz): this misses the case
where a Dart file doesn't contain main(),
Is this a TODO on you because you don't want to write that logic now or is it on
dart2js because it's hard to get that information?

[Andrei Mouravski, 2013-05-10 21:56:34]

Here you go. Fix and commit this puppy.


That came out weird.

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/command_deploy.dart (right):

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/command_deploy.dart:21: /// Handles the `deploy`
pub command.
Probably should mention what exactly it does and maybe how to use it?

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/io.dart (right):

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/io.dart:190: /// Any existing files at those paths
will be overwritten.
What about a no-clobber option for deploy? Or a dry-run option? Or verbose
that'll log that a file was clobbered?

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/io.dart:191: void copyFiles(Iterable<String>
files, String baseDir, String destination) {
Is there a reason why dart:io doesn't have this functionality?

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/utils.dart (right):

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/utils.dart:390: Uri pathToFileUri(String
pathString) {
Doesn't this code exist elsewhere?

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/utils.dart:404: /// something else since those
aren't supported.
Does this actually do anything but return the string passed in?

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
File
sdk/lib/_internal/pub/test/deploy/compiles_dart_entrypoints_to_dart_and_js.dart
(right):

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/deploy/compiles_dart_entrypoints_to_dart_and_js.dart:14:
d.dir(appPath, [
Do you even need the prefix?

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/deploy/compiles_dart_entrypoints_to_dart_and_js.dart:19:
d.file('subfile.dart', 'void main() => print("subhello");')
I think the subfile should print "ping."

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
File sdk/lib/_internal/pub/test/deploy/copies_dart_js_next_to_entrypoints.dart
(right):

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/deploy/copies_dart_js_next_to_entrypoints.dart:44:
output: new RegExp(r"Dependencies installed!$"));
I think this would look better lined up at the paren:
...(args: ...
    output: ...

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
File sdk/lib/_internal/pub/test/deploy/copies_non_dart_files_to_deploy.dart
(right):

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/deploy/copies_non_dart_files_to_deploy.dart:15: // a
browser dependency with no entrypoints shouldn't cause dart.js to be
Capital letter; period.

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/deploy/copies_non_dart_files_to_deploy.dart:19:
d.file('file.txt', 'contents'),
Add another file that's a lib-type dart file. No main, that is.

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/deploy/copies_non_dart_files_to_deploy.dart:21:
d.file('subfile.txt', 'subcontents')
Also, maybe another kind of file? .css? .png? .js?

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/deploy/copies_non_dart_files_to_deploy.dart:29:
Copying   web/ => deploy/
You don't print all the individual files being copied?

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
File sdk/lib/_internal/pub/test/deploy/ignores_non_entrypoint_dart_files.dart
(right):

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/deploy/ignores_non_entrypoint_dart_files.dart:19:
d.file('file3.dart', 'class Foo { void main() => print("hello"); }')
How about a regular file that's nothing fancy? "var foo;"

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
File sdk/lib/_internal/pub/test/deploy/with_no_web_directory.dart (right):

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/deploy/with_no_web_directory.dart:12:
d.appDir([]).create();
Add a different folder in that looks like web but isn't, maybe?

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
File sdk/lib/_internal/pub/test/test_pub.dart (right):

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/test_pub.dart:372: // TODO(nweiz): remove this when
issue 9294 is fixed.
Which "this".

[Andrei Mouravski, 2013-05-10 22:25:01]

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/command_deploy.dart (right):

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/command_deploy.dart:101: /// the deploy command.
Please document that this should be run after _findEntryPoints

[nweiz, 2013-05-13 21:59:44]

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/command_deploy.dart (right):

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/command_deploy.dart:28: // TODO(nweiz): make these
configurable.
On 2013/05/10 21:25:01, Andrei Mouravski wrote:
> You should just do this now:
> 
> String _source = 'web';
> void set source(String s) { _source = s }
> 
> Maybe add some path handling, too.

That's not what I meant "by configurable". Eventually these getters will return
results based on the command options, but that's out of scope for this CL.

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/command_deploy.dart:91: log.warning(e.message);
On 2013/05/10 21:25:01, Andrei Mouravski wrote:
> Do you really want to log a warning here? Isn't this going to be triggered on
> every non-main() dart file?

No. parseDartFile only throws FormatExceptions for actual parse errors (although
this reminds me that it actually throws AnalyzerErrorGroup now; fixed).

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/command_deploy.dart:101: /// the deploy command.
On 2013/05/10 21:25:01, Andrei Mouravski wrote:
> Add a note as to _why_ you need this.

Done.

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/command_deploy.dart:106: _entrypoints.map((file)
=> path.relative(file).length))
On 2013/05/10 21:25:01, Andrei Mouravski wrote:
> Why is this indented so far?

To visually distinguish it from the call to [add] on the next line.

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/command_deploy.dart:120: /// If this package
depends non-transitively on the `browser` package, this
On 2013/05/10 21:25:01, Andrei Mouravski wrote:
> Is there any way to use a CDN's browser?

Users are welcome to do that, but if they depend on the browser package
presumably they're using that package's dart.js, so we should make that work.

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/dart.dart (right):

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/dart.dart:5: library dart;
On 2013/05/10 21:25:01, Andrei Mouravski wrote:
> Could you write library level documentation? So people know what this library
is
> all about?

Done.

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/dart.dart:12: import 'package:pathos/path.dart' as
path;
On 2013/05/10 21:25:01, Andrei Mouravski wrote:
> How about a newline after packages.

We separate imports into three groups: internal "dart:" libraries, third-party
libraries, and libraries from the same package. The compiler libraries are
third-party, even if they can't be imported using "package:".

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/dart.dart:13: import
'../../../compiler/compiler.dart' as api;
On 2013/05/10 21:25:01, Andrei Mouravski wrote:
> Let's call this compiler since "api" is not descriptive.
> Or "dart2js".

Done.

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/dart.dart:32: Future<String> compile(String
entrypoint, {String packageRoot,
On 2013/05/10 21:25:01, Andrei Mouravski wrote:
> Is there a reason you can't just use the straight dart2js API for this? If
it's
> not good enough, that's feedback you should provide so they can fix it.

Everything this method does is a reason we can't use the straight dart2js API.
To name a few, it:

* requires the use of wrapped URI objects rather than plain paths (and if they
took paths, it would be wrapped in dart:io's Path class)
* specifies options as command-line strings
* doesn't default to useful options
* sometimes throws errors, sometimes just returns null

and so forth.

If I spent the time filing API design issues against every API I used, I'd never
have time to write actual code. I'm happy to file issues against actual bugs,
but for API design I'd rather just wrap it and move on with my work.

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/dart.dart:36: var options =
<String>['--categories=Client,Server', '--minify'];
On 2013/05/10 21:25:01, Andrei Mouravski wrote:
> What about logging? Where does that go?

I don't understand what you're asking. What does this line have to do with
logging? Are you referring to dart2js's logging or pub's logging?

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/dart.dart:56: /// directory in the repo and to the
root of the compiled SDK.
On 2013/05/10 21:25:01, Andrei Mouravski wrote:
> "in the repo" as in, "in the Dart SDK repository"?

In the Dart repository.

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/dart.dart:64: // TODO(nweiz): this misses the case
where a Dart file doesn't contain main(),
On 2013/05/10 21:25:01, Andrei Mouravski wrote:
> What if the main is in an HTML file?

That's going to have to be an unsupported use case, at least until dart2js
supports compiling HTML files.

https://codereview.chromium.org/14914007/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/dart.dart:64: // TODO(nweiz): this misses the case
where a Dart file doesn't contain main(),
On 2013/05/10 21:25:01, Andrei Mouravski wrote:
> Is this a TODO on you because you don't want to write that logic now or is it
on
> dart2js because it's hard to get that information?

The TODO is on me. It's possible to implement this in terms of the APIs the
analyzer exposes, but it's a very low priority since it would be difficult and I
don't think anyone will actually do this.

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/command_deploy.dart (right):

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/command_deploy.dart:21: /// Handles the `deploy`
pub command.
On 2013/05/10 21:56:34, Andrei Mouravski wrote:
> Probably should mention what exactly it does and maybe how to use it?

This isn't the place for that. We'll add full documentation to the website.

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/command_deploy.dart:101: /// the deploy command.
On 2013/05/10 22:25:02, Andrei Mouravski wrote:
> Please document that this should be run after _findEntryPoints

Done.

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/io.dart (right):

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/io.dart:190: /// Any existing files at those paths
will be overwritten.
On 2013/05/10 21:56:34, Andrei Mouravski wrote:
> What about a no-clobber option for deploy?

I don't think this would be useful. "deploy" is effectively a build step; users
shouldn't assume that the results of a build step will be safe from clobbering
when another build step will be run.

> Or a dry-run option?

This is something we could do, I suppose, but for the time being I think it's
pretty clear what's going to happen when you run deploy.

> Or verbose that'll log that a file was clobbered?

The existing --verbose option will mention that the "deploy" directory is
getting deleted. We don't have any more fine-grained information than that,
since the directory is deleted all at once.

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/io.dart:191: void copyFiles(Iterable<String>
files, String baseDir, String destination) {
On 2013/05/10 21:56:34, Andrei Mouravski wrote:
> Is there a reason why dart:io doesn't have this functionality?

I'm not really the person to ask that question, but at a guess it's because it's
reasonably easy to write manually.

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/utils.dart (right):

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/utils.dart:390: Uri pathToFileUri(String
pathString) {
On 2013/05/10 21:56:34, Andrei Mouravski wrote:
> Doesn't this code exist elsewhere?

Yes, as do many small utility functions. I feel like we've been over this
before.

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/utils.dart:404: /// something else since those
aren't supported.
On 2013/05/10 21:56:34, Andrei Mouravski wrote:
> Does this actually do anything but return the string passed in?

It chooses one of two strings that are passed in.

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
File
sdk/lib/_internal/pub/test/deploy/compiles_dart_entrypoints_to_dart_and_js.dart
(right):

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/deploy/compiles_dart_entrypoints_to_dart_and_js.dart:14:
d.dir(appPath, [
On 2013/05/10 21:56:34, Andrei Mouravski wrote:
> Do you even need the prefix?

You mean "d"? The recommended way of using the descriptor library is with a
prefix, so as to avoid name conflicts.

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/deploy/compiles_dart_entrypoints_to_dart_and_js.dart:19:
d.file('subfile.dart', 'void main() => print("subhello");')
On 2013/05/10 21:56:34, Andrei Mouravski wrote:
> I think the subfile should print "ping."

Haha, done.

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
File sdk/lib/_internal/pub/test/deploy/copies_dart_js_next_to_entrypoints.dart
(right):

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/deploy/copies_dart_js_next_to_entrypoints.dart:44:
output: new RegExp(r"Dependencies installed!$"));
On 2013/05/10 21:56:34, Andrei Mouravski wrote:
> I think this would look better lined up at the paren:
> ...(args: ...
>     output: ...

At this point I'd rather keep it consistent with every other use of schedulePub.

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
File sdk/lib/_internal/pub/test/deploy/copies_non_dart_files_to_deploy.dart
(right):

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/deploy/copies_non_dart_files_to_deploy.dart:15: // a
browser dependency with no entrypoints shouldn't cause dart.js to be
On 2013/05/10 21:56:34, Andrei Mouravski wrote:
> Capital letter; period.

Done.

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/deploy/copies_non_dart_files_to_deploy.dart:19:
d.file('file.txt', 'contents'),
On 2013/05/10 21:56:34, Andrei Mouravski wrote:
> Add another file that's a lib-type dart file. No main, that is.

There's another test that addresses these files.

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/deploy/copies_non_dart_files_to_deploy.dart:21:
d.file('subfile.txt', 'subcontents')
On 2013/05/10 21:56:34, Andrei Mouravski wrote:
> Also, maybe another kind of file? .css? .png? .js?

I don't think that's necessary. There's no reason the extension would affect the
behavior.

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/deploy/copies_non_dart_files_to_deploy.dart:29:
Copying   web/ => deploy/
On 2013/05/10 21:56:34, Andrei Mouravski wrote:
> You don't print all the individual files being copied?

No. The output is more to tell users what's going on for long-running deploy
steps than to give a comprehensive description of everything that was done.

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
File sdk/lib/_internal/pub/test/deploy/ignores_non_entrypoint_dart_files.dart
(right):

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/deploy/ignores_non_entrypoint_dart_files.dart:19:
d.file('file3.dart', 'class Foo { void main() => print("hello"); }')
On 2013/05/10 21:56:34, Andrei Mouravski wrote:
> How about a regular file that's nothing fancy? "var foo;"

Done.

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
File sdk/lib/_internal/pub/test/deploy/with_no_web_directory.dart (right):

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/deploy/with_no_web_directory.dart:12:
d.appDir([]).create();
On 2013/05/10 21:56:34, Andrei Mouravski wrote:
> Add a different folder in that looks like web but isn't, maybe?

What does "looks like web" mean? I don't think it's likely that we'll start
accidentally deploying from a directory named "wep".

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
File sdk/lib/_internal/pub/test/test_pub.dart (right):

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/test_pub.dart:372: // TODO(nweiz): remove this when
issue 9294 is fixed.
On 2013/05/10 21:56:34, Andrei Mouravski wrote:
> Which "this".

The line immediately below.

[Andrei Mouravski, 2013-05-13 22:40:32]

lgtm

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/command_deploy.dart (right):

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/command_deploy.dart:21: /// Handles the `deploy`
pub command.
You don't have to add much, and it might be valuable to put it into the library
docs:
/// The deploy command compiles all Dart entrypoints in /web  /// into /deploy
and copies all other files as well.
/// Blah blah blah won't work if you have foo bar baz.

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/utils.dart (right):

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/utils.dart:404: /// something else since those
aren't supported.
I figured that out, but it's not described very clearly. Not sure how to make it
better...

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
File sdk/lib/_internal/pub/test/deploy/copies_non_dart_files_to_deploy.dart
(right):

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/deploy/copies_non_dart_files_to_deploy.dart:21:
d.file('subfile.txt', 'subcontents')
And wouldn't it be nice to have a test that proves that in perpetuity? :P

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/deploy/copies_non_dart_files_to_deploy.dart:29:
Copying   web/ => deploy/
But verbose will print all files that are being printed, right?

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
File sdk/lib/_internal/pub/test/deploy/with_no_web_directory.dart (right):

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/deploy/with_no_web_directory.dart:12:
d.appDir([]).create();
I have no idea what I was talking about. x_x

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

https://codereview.chromium.org/14914007/diff/30001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:233: Future<List<String>>
packageFiles({String beneath}) {
Do you have a test for the .gitignore bit?

[nweiz, 2013-05-13 23:31:06]

Committed patchset #4 manually as r22656 (presubmit successful).

[nweiz, 2013-05-13 23:32:58]

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/command_deploy.dart (right):

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/command_deploy.dart:21: /// Handles the `deploy`
pub command.
On 2013/05/13 22:40:33, Andrei Mouravski wrote:
> You don't have to add much, and it might be valuable to put it into the
library
> docs:
> /// The deploy command compiles all Dart entrypoints in /web  /// into /deploy
> and copies all other files as well.
> /// Blah blah blah won't work if you have foo bar baz. 

The class itself includes as much information as "pub help deploy". If more
information needs to be made available, it should be done in a user-visible way
as well.

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
File sdk/lib/_internal/pub/test/deploy/copies_non_dart_files_to_deploy.dart
(right):

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/deploy/copies_non_dart_files_to_deploy.dart:21:
d.file('subfile.txt', 'subcontents')
On 2013/05/13 22:40:33, Andrei Mouravski wrote:
> And wouldn't it be nice to have a test that proves that in perpetuity? :P

It's not possible to add tests for all possible permutations of behavior, nor
would it be a good idea.

https://codereview.chromium.org/14914007/diff/18002/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/deploy/copies_non_dart_files_to_deploy.dart:29:
Copying   web/ => deploy/
On 2013/05/13 22:40:33, Andrei Mouravski wrote:
> But verbose will print all files that are being printed, right?

Do you mean "being copied"? Yes, it will.

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

https://codereview.chromium.org/14914007/diff/30001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:233: Future<List<String>>
packageFiles({String beneath}) {
On 2013/05/13 22:40:33, Andrei Mouravski wrote:
> Do you have a test for the .gitignore bit?

Actually, no; prior to this, we only ran this code for "pub lish", and we were
unable to verify it there for tangential reasons. This would be a good candidate
for a unit test.

I'll add a TODO for this.