Install before starting the server if the lockfile is out of date.

Install before starting the server if the lockfile is out of date.

R=nweiz@google.com

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

[nweiz, 2013-07-30 19:34:36]

https://codereview.chromium.org/21147002/diff/3001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/command/serve.dart (right):

https://codereview.chromium.org/21147002/diff/3001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/command/serve.dart:44: log.message("Dependencies
have changed, installing...");
"have changed since the pubspec.lock was last generated"

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

https://codereview.chromium.org/21147002/diff/3001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:162: /// what's in there.
Doesn't this return `false` in those cases?

https://codereview.chromium.org/21147002/diff/3001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:175: locked.description)) return
false;
Style nit: I think if the if clause goes over one line, the return should be on
its own line in a block.

https://codereview.chromium.org/21147002/diff/3001/sdk/lib/_internal/pub/test...
File
sdk/lib/_internal/pub/test/serve/installs_first_if_dependency_added_test.dart
(right):

https://codereview.chromium.org/21147002/diff/3001/sdk/lib/_internal/pub/test...
sdk/lib/_internal/pub/test/serve/installs_first_if_dependency_added_test.dart:16:
integration("installs first if a dependency is not in the lock file", () {
Test that it doesn't install if a dependency is removed.

https://codereview.chromium.org/21147002/diff/3001/sdk/lib/_internal/pub/test...
sdk/lib/_internal/pub/test/serve/installs_first_if_dependency_added_test.dart:29:
}))
I don't like coupling the tests to the specific lockfile format. I'd rather run
pub install, then modify the pubspec, like you would if you were testing this
manually.

https://codereview.chromium.org/21147002/diff/3001/sdk/lib/_internal/pub/test...
File
sdk/lib/_internal/pub/test/serve/installs_first_if_dependency_version_changed_test.dart
(right):

https://codereview.chromium.org/21147002/diff/3001/sdk/lib/_internal/pub/test...
sdk/lib/_internal/pub/test/serve/installs_first_if_dependency_version_changed_test.dart:16:
integration("installs first if a dependency's version doesn't match the one "
In the same vein as this, test that if the version changes to a constraint that
still matches the locked version it doesn't reinstall.

https://codereview.chromium.org/21147002/diff/3001/sdk/lib/_internal/pub/test...
File sdk/lib/_internal/pub/test/serve/utils.dart (right):

https://codereview.chromium.org/21147002/diff/3001/sdk/lib/_internal/pub/test...
sdk/lib/_internal/pub/test/serve/utils.dart:25: /// If [shouldInstallFirst] is
`true`, then validates that pub install is run
Nit: remove "then"

https://codereview.chromium.org/21147002/diff/3001/sdk/lib/_internal/pub/test...
sdk/lib/_internal/pub/test/serve/utils.dart:45: });
Since [nextLine] is scheduled internally, you can write this without manually
chaining futures:

    if (shouldInstallFirst) {
      expect(_pubServer.nextLine(),
          completion(startsWith("Dependencies have changed")));
      expect(_pubServer.nextLine(),
          completion(startsWith("Resolving dependencies...")));
      expect(_pubServer.nextLine(),
          completion(startsWith("Dependencies installed!")));
    }
    expect(_pubServer.nextLine().then(_parsePort), completes);

[Bob Nystrom, 2013-07-30 21:35:32]

Thanks!

https://codereview.chromium.org/21147002/diff/3001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/command/serve.dart (right):

https://codereview.chromium.org/21147002/diff/3001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/command/serve.dart:44: log.message("Dependencies
have changed, installing...");
On 2013/07/30 19:34:36, nweiz wrote:
> "have changed since the pubspec.lock was last generated"

I think it's better to not mention the lock file here. That's sort of
implementation-detailey.

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

https://codereview.chromium.org/21147002/diff/3001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:162: /// what's in there.
On 2013/07/30 19:34:36, nweiz wrote:
> Doesn't this return `false` in those cases?

Fixed. Originally wrote the method to be isLockFileOutOfDate() and forgot to
update the docs.

https://codereview.chromium.org/21147002/diff/3001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:175: locked.description)) return
false;
On 2013/07/30 19:34:36, nweiz wrote:
> Style nit: I think if the if clause goes over one line, the return should be
on
> its own line in a block.

Done.

https://codereview.chromium.org/21147002/diff/3001/sdk/lib/_internal/pub/test...
File
sdk/lib/_internal/pub/test/serve/installs_first_if_dependency_added_test.dart
(right):

https://codereview.chromium.org/21147002/diff/3001/sdk/lib/_internal/pub/test...
sdk/lib/_internal/pub/test/serve/installs_first_if_dependency_added_test.dart:16:
integration("installs first if a dependency is not in the lock file", () {
On 2013/07/30 19:34:36, nweiz wrote:
> Test that it doesn't install if a dependency is removed.

Done.

https://codereview.chromium.org/21147002/diff/3001/sdk/lib/_internal/pub/test...
sdk/lib/_internal/pub/test/serve/installs_first_if_dependency_added_test.dart:29:
}))
On 2013/07/30 19:34:36, nweiz wrote:
> I don't like coupling the tests to the specific lockfile format. I'd rather
run
> pub install, then modify the pubspec, like you would if you were testing this
> manually.

Done.

https://codereview.chromium.org/21147002/diff/3001/sdk/lib/_internal/pub/test...
File
sdk/lib/_internal/pub/test/serve/installs_first_if_dependency_version_changed_test.dart
(right):

https://codereview.chromium.org/21147002/diff/3001/sdk/lib/_internal/pub/test...
sdk/lib/_internal/pub/test/serve/installs_first_if_dependency_version_changed_test.dart:16:
integration("installs first if a dependency's version doesn't match the one "
On 2013/07/30 19:34:36, nweiz wrote:
> In the same vein as this, test that if the version changes to a constraint
that
> still matches the locked version it doesn't reinstall.

Done.

https://codereview.chromium.org/21147002/diff/3001/sdk/lib/_internal/pub/test...
File sdk/lib/_internal/pub/test/serve/utils.dart (right):

https://codereview.chromium.org/21147002/diff/3001/sdk/lib/_internal/pub/test...
sdk/lib/_internal/pub/test/serve/utils.dart:25: /// If [shouldInstallFirst] is
`true`, then validates that pub install is run
On 2013/07/30 19:34:36, nweiz wrote:
> Nit: remove "then"

Done.

https://codereview.chromium.org/21147002/diff/3001/sdk/lib/_internal/pub/test...
sdk/lib/_internal/pub/test/serve/utils.dart:45: });
On 2013/07/30 19:34:36, nweiz wrote:
> Since [nextLine] is scheduled internally, you can write this without manually
> chaining futures:
> 
>     if (shouldInstallFirst) {
>       expect(_pubServer.nextLine(),
>           completion(startsWith("Dependencies have changed")));
>       expect(_pubServer.nextLine(),
>           completion(startsWith("Resolving dependencies...")));
>       expect(_pubServer.nextLine(),
>           completion(startsWith("Dependencies installed!")));
>     }
>     expect(_pubServer.nextLine().then(_parsePort), completes);

Done.

[nweiz, 2013-07-30 21:46:58]

lgtm

[Bob Nystrom, 2013-07-30 22:25:59]

Committed patchset #3 manually as r25648 (presubmit successful).