Don't recompile dependency executables that haven't changed.

Don't recompile dependency executables that haven't changed.

R=rnystrom@google.com
BUG=20482
Committed: https://code.google.com/p/dart/source/detail?r=39441

[Bob Nystrom, 2014-08-20 20:33:00]

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

https://codereview.chromium.org/491993002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:164: if (!sdkMatches) changed =
null;
Document this.

https://codereview.chromium.org/491993002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:206: List<AssetId>
_executablesForPackage(PackageGraph graph, String packageName,
_changedExecutablesForPackage()?

https://codereview.chromium.org/491993002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:241: if (!executablesExist) return
executablesExist;
return executables;

Is there a reason to do every() here? Why not just compile the ones that are
missing? (I understand that in practice they will be all present or all missing,
but I don't know why the code relies on that.)

Oh, now I see. You delete the entire directory if any executable needs to be
recompiled. Document that.

https://codereview.chromium.org/491993002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:246: }
I'd move this above the previous check.

https://codereview.chromium.org/491993002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:252: Future
_precompileExecutablesForPackage(
Why no doc comment?

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

https://codereview.chromium.org/491993002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/solver/version_solver.dart:80: /// This includes
packages that were added or removed.
What about ones that were upgraded, downgraded, or changed source or
description?

https://codereview.chromium.org/491993002/diff/20001/sdk/lib/_internal/pub/te...
File
sdk/lib/_internal/pub/test/snapshot/doesnt_resnapshot_when_a_dependency_is_unchanged_test.dart
(right):

https://codereview.chromium.org/491993002/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/snapshot/doesnt_resnapshot_when_a_dependency_is_unchanged_test.dart:16:
"have changed", () {
Test that it *does* recreate the snapshot when the package has changed, or a
transitive dependency of it.

[nweiz, 2014-08-20 20:59:35]

Code review changes

[nweiz, 2014-08-20 20:59:40]

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

https://codereview.chromium.org/491993002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:164: if (!sdkMatches) changed =
null;
On 2014/08/20 20:33:00, Bob Nystrom wrote:
> Document this.

Done.

https://codereview.chromium.org/491993002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:206: List<AssetId>
_executablesForPackage(PackageGraph graph, String packageName,
On 2014/08/20 20:32:59, Bob Nystrom wrote:
> _changedExecutablesForPackage()?

I don't like that that name implies that the only sort of filtering this is
doing is for which executables changed. It also filters out executables for
mutable packages.

https://codereview.chromium.org/491993002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:241: if (!executablesExist) return
executablesExist;
On 2014/08/20 20:32:59, Bob Nystrom wrote:
> return executables;

Done.

> Is there a reason to do every() here? Why not just compile the ones that are
> missing? (I understand that in practice they will be all present or all
missing,
> but I don't know why the code relies on that.)
> 
> Oh, now I see. You delete the entire directory if any executable needs to be
> recompiled. Document that.

Done.

https://codereview.chromium.org/491993002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:246: }
On 2014/08/20 20:32:59, Bob Nystrom wrote:
> I'd move this above the previous check.

Done.

https://codereview.chromium.org/491993002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:252: Future
_precompileExecutablesForPackage(
On 2014/08/20 20:32:59, Bob Nystrom wrote:
> Why no doc comment?

Merge error.

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

https://codereview.chromium.org/491993002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/solver/version_solver.dart:80: /// This includes
packages that were added or removed.
On 2014/08/20 20:33:00, Bob Nystrom wrote:
> What about ones that were upgraded, downgraded, or changed source or
> description?

I explicitly called out "added or removed" because I thought it may not be
obvious that that was considered "changed". It seems clear to me that upgrading,
downgrading, or changing source or description constitutes a change.

https://codereview.chromium.org/491993002/diff/20001/sdk/lib/_internal/pub/te...
File
sdk/lib/_internal/pub/test/snapshot/doesnt_resnapshot_when_a_dependency_is_unchanged_test.dart
(right):

https://codereview.chromium.org/491993002/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/snapshot/doesnt_resnapshot_when_a_dependency_is_unchanged_test.dart:16:
"have changed", () {
On 2014/08/20 20:33:00, Bob Nystrom wrote:
> Test that it *does* recreate the snapshot when the package has changed, or a
> transitive dependency of it.

This is already covered by existing tests.

[Bob Nystrom, 2014-08-20 21:40:52]

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

https://codereview.chromium.org/491993002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/solver/version_solver.dart:80: /// This includes
packages that were added or removed.
On 2014/08/20 20:59:40, nweiz wrote:
> On 2014/08/20 20:33:00, Bob Nystrom wrote:
> > What about ones that were upgraded, downgraded, or changed source or
> > description?
> 
> I explicitly called out "added or removed" because I thought it may not be
> obvious that that was considered "changed". It seems clear to me that
upgrading,
> downgrading, or changing source or description constitutes a change.

Right, on first read, I didn't realize this covered those cases. You're relying
on PackageId.== to tell this which, unfortunately, isn't reliable. It doesn't
cover differences in descriptions. (That operator is mainly used so PackageIds
can be keys in maps where we don't expect to see multiple keys for a package
with the same name/version/source.)

Take a look at SolveReport._descriptionsEqual() to see what you'll probably need
to do here.

[nweiz, 2014-08-20 23:53:40]

Code review changes

[nweiz, 2014-08-20 23:53:44]

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

https://codereview.chromium.org/491993002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/solver/version_solver.dart:80: /// This includes
packages that were added or removed.
On 2014/08/20 21:40:51, Bob Nystrom wrote:
> On 2014/08/20 20:59:40, nweiz wrote:
> > On 2014/08/20 20:33:00, Bob Nystrom wrote:
> > > What about ones that were upgraded, downgraded, or changed source or
> > > description?
> > 
> > I explicitly called out "added or removed" because I thought it may not be
> > obvious that that was considered "changed". It seems clear to me that
> upgrading,
> > downgrading, or changing source or description constitutes a change.
> 
> Right, on first read, I didn't realize this covered those cases. You're
relying
> on PackageId.== to tell this which, unfortunately, isn't reliable. It doesn't
> cover differences in descriptions. (That operator is mainly used so PackageIds
> can be keys in maps where we don't expect to see multiple keys for a package
> with the same name/version/source.)
> 
> Take a look at SolveReport._descriptionsEqual() to see what you'll probably
need
> to do here.

Done.

[Bob Nystrom, 2014-08-21 00:03:06]

lgtm

[nweiz, 2014-08-21 01:28:51]

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