Server .packages support.

Server .packages support.

R=brianwilkerson@google.com, paulberry@google.com

Committed: https://github.com/dart-lang/sdk/commit/05d420692980735434252344ef3cd9535296cdf0

[pquitslund, 2015-07-17 17:04:00]

pquitslund@google.com changed reviewers:
+ brianwilkerson@google.com, paulberry@google.com

[pquitslund, 2015-07-17 17:04:00]

WIP/jumping-off point.

[Brian Wilkerson, 2015-07-17 18:02:58]

https://codereview.chromium.org/1242023008/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/context_manager.dart (right):

https://codereview.chromium.org/1242023008/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/context_manager.dart:498: Folder folder, File
pubspecFile, List<_ContextInfo> children) {
Perhaps rename 'pubspecFile' to 'packageSpec' for consistency?

https://codereview.chromium.org/1242023008/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/context_manager.dart:667: if (info.folder.path !=
directoryPath) {
If a .packages file is added to the context's root directory we need to update
the context to use the .packages file to resolve URI's. I think this misses that
case.

https://codereview.chromium.org/1242023008/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/context_manager.dart:671:
.getFile(pathos.join(directoryPath, '.packages')).exists) {
nit: use PACKAGE_SPEC_NAME (here and below)

https://codereview.chromium.org/1242023008/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/context_manager.dart:679:
.getFile(pathos.join(directoryPath, 'pubspec.yaml')).exists) {
nit: use PUBSPEC_NAME (here and below)

https://codereview.chromium.org/1242023008/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/context_manager.dart:740: case ChangeType.MODIFY:
Do we handle changes to the .packages file somewhere else, or are we missing
that case?

https://codereview.chromium.org/1242023008/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/context_manager.dart:808: //TODO(pquitslund):
error-handling?
Yes. If we cannot read the file, I think we should act as if the file doesn't
exist. It would be nice if we could create an error for the file indicating that
it's being ignored.

https://codereview.chromium.org/1242023008/diff/1/pkg/analysis_server/test/co...
File pkg/analysis_server/test/context_manager_test.dart (right):

https://codereview.chromium.org/1242023008/diff/1/pkg/analysis_server/test/co...
pkg/analysis_server/test/context_manager_test.dart:138: return
pumpEventQueue().then((_) {
You can also make this an async method and return Future from it. It would make
the code a little cleaner:

await pumpEventQueue();
expect(...);
...

https://codereview.chromium.org/1242023008/diff/1/pkg/analysis_server/test/co...
pkg/analysis_server/test/context_manager_test.dart:853: String rootPubspec =
'$root/.packages';
nit: rootPubspec --> rootPackagespec?

https://codereview.chromium.org/1242023008/diff/1/pkg/analysis_server/test/co...
pkg/analysis_server/test/context_manager_test.dart:865:
manager.assertContextFiles(root, [rootFile]);
This doesn't actually guarantee that the changes to the .packages file were
picked up, only that we didn't create a new context because of it's addition
(which is also good).

https://codereview.chromium.org/1242023008/diff/1/pkg/analysis_server/test/co...
pkg/analysis_server/test/context_manager_test.dart:910: // add pubspec - ignore,
because is already in a pubspec-based context
Was "pubspec-based" suppose to be ".packages-based"?

[pquitslund, 2015-07-17 19:30:02]

Thank you!

Comments addressed.

I'll follow-up with a flag to programmatically disable the new logic so that we
can integrate.

https://codereview.chromium.org/1242023008/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/context_manager.dart (right):

https://codereview.chromium.org/1242023008/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/context_manager.dart:498: Folder folder, File
pubspecFile, List<_ContextInfo> children) {
On 2015/07/17 18:02:57, Brian Wilkerson wrote:
> Perhaps rename 'pubspecFile' to 'packageSpec' for consistency?

Done.

https://codereview.chromium.org/1242023008/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/context_manager.dart:667: if (info.folder.path !=
directoryPath) {
On 2015/07/17 18:02:57, Brian Wilkerson wrote:
> If a .packages file is added to the context's root directory we need to update
> the context to use the .packages file to resolve URI's. I think this misses
that
> case.

Actually, I think this is handled in _createContext(..) which is called by
_extractContext(..).  The fact remains that this is not well under-test...  I
added a TODO in the tests and hope that we can address it better after Paul's
rethink.

https://codereview.chromium.org/1242023008/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/context_manager.dart:671:
.getFile(pathos.join(directoryPath, '.packages')).exists) {
On 2015/07/17 18:02:58, Brian Wilkerson wrote:
> nit: use PACKAGE_SPEC_NAME (here and below)

Done.

https://codereview.chromium.org/1242023008/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/context_manager.dart:679:
.getFile(pathos.join(directoryPath, 'pubspec.yaml')).exists) {
On 2015/07/17 18:02:57, Brian Wilkerson wrote:
> nit: use PUBSPEC_NAME (here and below)

Done.

https://codereview.chromium.org/1242023008/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/context_manager.dart:740: case ChangeType.MODIFY:
On 2015/07/17 18:02:57, Brian Wilkerson wrote:
> Do we handle changes to the .packages file somewhere else, or are we missing
> that case?

Yes.  That's outside the switch.  (See below.)

Certainly needs a better home! :)

https://codereview.chromium.org/1242023008/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/context_manager.dart:753:
_checkForPackagespecUpdate(path, info, folder);
Here's where we test for .packages changes.

https://codereview.chromium.org/1242023008/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/context_manager.dart:808: //TODO(pquitslund):
error-handling?
On 2015/07/17 18:02:57, Brian Wilkerson wrote:
> Yes. If we cannot read the file, I think we should act as if the file doesn't
> exist. It would be nice if we could create an error for the file indicating
that
> it's being ignored.

Cool.  Added a try/catch and a TODO to create an error down the road.

https://codereview.chromium.org/1242023008/diff/1/pkg/analysis_server/test/co...
File pkg/analysis_server/test/context_manager_test.dart (right):

https://codereview.chromium.org/1242023008/diff/1/pkg/analysis_server/test/co...
pkg/analysis_server/test/context_manager_test.dart:138: return
pumpEventQueue().then((_) {
On 2015/07/17 18:02:58, Brian Wilkerson wrote:
> You can also make this an async method and return Future from it. It would
make
> the code a little cleaner:
> 
> await pumpEventQueue();
> expect(...);
> ...

Acknowledged.

https://codereview.chromium.org/1242023008/diff/1/pkg/analysis_server/test/co...
pkg/analysis_server/test/context_manager_test.dart:853: String rootPubspec =
'$root/.packages';
On 2015/07/17 18:02:58, Brian Wilkerson wrote:
> nit: rootPubspec --> rootPackagespec?

Done.

https://codereview.chromium.org/1242023008/diff/1/pkg/analysis_server/test/co...
pkg/analysis_server/test/context_manager_test.dart:865:
manager.assertContextFiles(root, [rootFile]);
On 2015/07/17 18:02:58, Brian Wilkerson wrote:
> This doesn't actually guarantee that the changes to the .packages file were
> picked up, only that we didn't create a new context because of it's addition
> (which is also good).

Good call!

This may be tricky to test since the source factory creation bits are in
ServerContextManager.  I'll add a TODO in the meantime.

https://codereview.chromium.org/1242023008/diff/1/pkg/analysis_server/test/co...
pkg/analysis_server/test/context_manager_test.dart:910: // add pubspec - ignore,
because is already in a pubspec-based context
On 2015/07/17 18:02:58, Brian Wilkerson wrote:
> Was "pubspec-based" suppose to be ".packages-based"?

Yes!

[pquitslund, 2015-07-17 19:59:00]

New behavior is now hidden behind a flag.  

PTAL and sanity-check?

[Brian Wilkerson, 2015-07-17 20:32:56]

LGTM

[Paul Berry, 2015-07-17 20:49:13]

lgtm

https://codereview.chromium.org/1242023008/diff/90001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/context_manager.dart (right):

https://codereview.chromium.org/1242023008/diff/90001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/context_manager.dart:36: * Temporary flag to hide
WIP .packages support.
It would be nice to mention the DEP number (DEP 5) in this comment.

https://codereview.chromium.org/1242023008/diff/90001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/context_manager.dart:1066: _ContextInfo(Folder
folder, File pubspecFile, this.children, this.packageRoot)
Rename pubspecFile to packageDescriptionFile to be consistent with the rename of
pubspecPath above.

[pquitslund, 2015-07-19 05:18:34]

Committed patchset #7 (id:110001) manually as
05d420692980735434252344ef3cd9535296cdf0 (presubmit successful).

[pquitslund, 2015-07-20 20:50:22]

Forgot to send....

https://codereview.chromium.org/1242023008/diff/90001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/context_manager.dart (right):

https://codereview.chromium.org/1242023008/diff/90001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/context_manager.dart:36: * Temporary flag to hide
WIP .packages support.
On 2015/07/17 20:49:13, Paul Berry wrote:
> It would be nice to mention the DEP number (DEP 5) in this comment.

Done.

https://codereview.chromium.org/1242023008/diff/90001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/context_manager.dart:1066: _ContextInfo(Folder
folder, File pubspecFile, this.children, this.packageRoot)
On 2015/07/17 20:49:13, Paul Berry wrote:
> Rename pubspecFile to packageDescriptionFile to be consistent with the rename
of
> pubspecPath above.

Done.