simplify computing of runtime file locations

simplify computing of runtime file location

R=vsm@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/d8d19c94fd67b70f30d4a83d2f9ec4971794f7de

[Jennifer Messerly, 2015-07-14 22:58:00]

jmesserly@google.com changed reviewers:
+ vsm@google.com

[Jennifer Messerly, 2015-07-14 22:58:01]

just something I came across working on other refactorings.

[vsm, 2015-07-14 23:52:30]

lgtm with question

https://codereview.chromium.org/1223113005/diff/1/lib/src/options.dart
File lib/src/options.dart (left):

https://codereview.chromium.org/1223113005/diff/1/lib/src/options.dart#oldcod...
lib/src/options.dart:266: /// activated via `pub global activate`.
Will this still work in the `pub global activate` case?

[Jennifer Messerly, 2015-07-15 00:29:41]

Yeah, I should've explained that better. Doh!

btw, I simplified further, mirrors aren't needed in new version.

https://codereview.chromium.org/1223113005/diff/1/lib/src/options.dart
File lib/src/options.dart (left):

https://codereview.chromium.org/1223113005/diff/1/lib/src/options.dart#oldcod...
lib/src/options.dart:266: /// activated via `pub global activate`.
On 2015/07/14 23:52:30, vsm wrote:
> Will this still work in the `pub global activate` case?

It should, but folks will need to add an explicit dependency on dev_compiler
package if they want to use --server. Because in a snapshot I don't think we can
get access to our resources ... 

The code below depends on all kinds of `pub` implementation details. It's
guessing that if we're running from a snapshot, maybe `pub` created the
snapshot, and hey let's check for git source and all this stuff to replicate
what Pub does. That's rather fragile logic.

The new code just says "hey look for package:dev_compiler/runtime" ... seemed
easy enough for now.

Is that good enough for now?

Longer term we have so many options. If we did want to emit our runtime .js
files when compiling HTML files, instead of having the user import them, and we
want it to use a pub global install, and we don't want to recompile against the
actual Dart SDK you're using, then we probably want something more like
https://api.dartlang.org/apidocs/channels/dev/dartdoc-viewer/dart:core.Resource

But I guess, it seems like we're in the mode now where we're more about clean
analyzer integration, and I don't see how we can easily do that with code of
this nature...

[vsm, 2015-07-15 00:45:29]

https://codereview.chromium.org/1223113005/diff/1/lib/src/options.dart
File lib/src/options.dart (left):

https://codereview.chromium.org/1223113005/diff/1/lib/src/options.dart#oldcod...
lib/src/options.dart:266: /// activated via `pub global activate`.
On 2015/07/15 00:29:41, John Messerly wrote:
> On 2015/07/14 23:52:30, vsm wrote:
> > Will this still work in the `pub global activate` case?
> 
> It should, but folks will need to add an explicit dependency on dev_compiler
> package if they want to use --server. Because in a snapshot I don't think we
can
> get access to our resources ... 

Hmm, how do you add an explicit dep in this case?  E.g., our current usage doc:

https://github.com/dart-lang/dev_compiler/blob/master/USAGE.md

Will that still work?  

> 
> The code below depends on all kinds of `pub` implementation details. It's
> guessing that if we're running from a snapshot, maybe `pub` created the
> snapshot, and hey let's check for git source and all this stuff to replicate
> what Pub does. That's rather fragile logic.
> 
> The new code just says "hey look for package:dev_compiler/runtime" ... seemed
> easy enough for now.
> 
> Is that good enough for now?
> 
> Longer term we have so many options. If we did want to emit our runtime .js
> files when compiling HTML files, instead of having the user import them, and
we
> want it to use a pub global install, and we don't want to recompile against
the
> actual Dart SDK you're using, then we probably want something more like
>
https://api.dartlang.org/apidocs/channels/dev/dartdoc-viewer/dart:core.Resource
> 
> But I guess, it seems like we're in the mode now where we're more about clean
> analyzer integration, and I don't see how we can easily do that with code of
> this nature...

[Jennifer Messerly, 2015-07-15 15:30:43]

https://codereview.chromium.org/1223113005/diff/1/lib/src/options.dart
File lib/src/options.dart (left):

https://codereview.chromium.org/1223113005/diff/1/lib/src/options.dart#oldcod...
lib/src/options.dart:266: /// activated via `pub global activate`.
On 2015/07/15 00:45:29, vsm wrote:
> On 2015/07/15 00:29:41, John Messerly wrote:
> > On 2015/07/14 23:52:30, vsm wrote:
> > > Will this still work in the `pub global activate` case?
> > 
> > It should, but folks will need to add an explicit dependency on dev_compiler
> > package if they want to use --server. Because in a snapshot I don't think we
> can
> > get access to our resources ... 
> 
> Hmm, how do you add an explicit dep in this case?  E.g., our current usage
doc:
> 
> https://github.com/dart-lang/dev_compiler/blob/master/USAGE.md
> 
> Will that still work?  
> 
> > 
> > The code below depends on all kinds of `pub` implementation details. It's
> > guessing that if we're running from a snapshot, maybe `pub` created the
> > snapshot, and hey let's check for git source and all this stuff to replicate
> > what Pub does. That's rather fragile logic.
> > 
> > The new code just says "hey look for package:dev_compiler/runtime" ...
seemed
> > easy enough for now.
> > 
> > Is that good enough for now?
> > 
> > Longer term we have so many options. If we did want to emit our runtime .js
> > files when compiling HTML files, instead of having the user import them, and
> we
> > want it to use a pub global install, and we don't want to recompile against
> the
> > actual Dart SDK you're using, then we probably want something more like
> >
>
https://api.dartlang.org/apidocs/channels/dev/dartdoc-viewer/dart:core.Resource
> > 
> > But I guess, it seems like we're in the mode now where we're more about
clean
> > analyzer integration, and I don't see how we can easily do that with code of
> > this nature...
> 

For checking/compiling: yes

For running --server: you'll need to add a "dev_compiler" to your pubspec
dev_dependencies, that gets you access to the runtime files.

This really boils down to "how do we distribute our precompiled SDK JS files?"
Right now, we use pub implementation details to guess: "hey if we're running our
compiled binary, probably our original sources are still somewhere in Pub's
cache" ... I mean, it does work in practice, but it's fragile w.r.t. the
refactoring we need to do. A package is the logical place to look for runtime
files.

Here's another proposal ... I can see how Phil feels about moving this code in
the "cli_util" package. Then at some point, replace it with the Resources class
that Lasse is implementing. At least that moves this Resources workaround away
from our compiler code. 

The downside is, I still don't see how this code will work correctly in an
analyzer_cli/analysis_server environment. In the short term, I think we're
better off asking users to add the dev dependency...

[Jennifer Messerly, 2015-07-15 16:39:18]

After further discussion, it sounds like maybe we could do a prebuild step that
captures the JS resources in .dart files so they're available in the snapshot,
regardless of entry point.

downside: we may need to do this for the tool/generated_sdk sources too, given:
https://github.com/dart-lang/dev_compiler/issues/257 ...

For now I can revert the runtimeDir change, but keep the other changes, if that
sounds good.

[Jennifer Messerly, 2015-07-15 16:46:37]

PTAL

[vsm, 2015-07-15 16:56:56]

lgtm

[Jennifer Messerly, 2015-07-15 16:58:28]

Committed patchset #3 (id:40001) manually as
d8d19c94fd67b70f30d4a83d2f9ec4971794f7de (presubmit successful).