fix for bin/devc relative path

fix for bin/devc relative path

R=vsm@google.com

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

[Jennifer Messerly, 2015-07-20 16:02:03]

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

[Jennifer Messerly, 2015-07-20 16:02:04]

fixes some issues with
https://github.com/dart-lang/dev_compiler/commit/e260ddaa284a9b6ac054a1469689...
around handling of relative paths, and input base directory.

These changes are used more heavily in another CL I'm working on, but I wanted
to send out since you're hitting it too :)

[vsm, 2015-07-20 16:11:16]

https://codereview.chromium.org/1243693002/diff/1/lib/src/compiler.dart
File lib/src/compiler.dart (right):

https://codereview.chromium.org/1243693002/diff/1/lib/src/compiler.dart#newco...
lib/src/compiler.dart:211:
df.append(html_codegen.libraryInclude(runtimeFileOutput(file)));
I think you need to make a relative path wrt *from* here as well.

[Jennifer Messerly, 2015-07-20 16:32:31]

thanks!

https://codereview.chromium.org/1243693002/diff/1/lib/src/compiler.dart
File lib/src/compiler.dart (right):

https://codereview.chromium.org/1243693002/diff/1/lib/src/compiler.dart#newco...
lib/src/compiler.dart:211:
df.append(html_codegen.libraryInclude(runtimeFileOutput(file)));
On 2015/07/20 16:11:16, vsm wrote:
> I think you need to make a relative path wrt *from* here as well.

It's already a relative path. The (poorly named) runtimeFileOutput just prepends
"dev_compiler/runtime". So the path ends up as
"dev_compiler/runtime/dart/core.js" for example. Which results in the HTML
saying `<script src="dev_compiler/runtime/dart/core.js">` which is exactly what
we want. Then above, all of the runtime files are copied into the "htmlOutDir"
(line 191), so you get a structure like:

sunflower/
- sunflower.html
- dev_compiler/
- - runtime/
- - - dart/
- - - - core.js

The scripts match up with the HTML file's relative path.

This does mean multiple every HTML file gets a copy of the runtime next to it.
This roughly matches what we used to do (and still do in server mode), for
better or worse. I basically kept it because it matches what server does. Though
I agree, it's not right longer term.

For demo purposes, could you have all of your HTML files in the same directory?
Alternatively, would "../" be okay in the HTML file, if the input base directory
was above the HTML? In that case, I'd probably have it output the runtime files
not as part of compileHtml, but as part of `run`. The invariant would be one
batch compile == one runtime. If you want me to take that route, I'd prefer
doing it in a follow up, as it might cause collateral test damage :)

Anyway, I'm happy to continue tweaking all of this stuff. It's just hard to
think about the right thing when we still have our server. I feel like it has
taken a lot of energy to try and maintain the system, when we know ultimately
it's not the right direction w.r.t. analyzer integration. Not sure the smartest
approach at this point. I'm starting to think if we had a very simple file
watcher (perhaps borrowing some code from analysis_server), that could get us to
a saner system in the meantime.

[vsm, 2015-07-20 20:46:57]

On 2015/07/20 16:32:31, John Messerly wrote:
> thanks!
> 
> https://codereview.chromium.org/1243693002/diff/1/lib/src/compiler.dart
> File lib/src/compiler.dart (right):
> 
>
https://codereview.chromium.org/1243693002/diff/1/lib/src/compiler.dart#newco...
> lib/src/compiler.dart:211:
> df.append(html_codegen.libraryInclude(runtimeFileOutput(file)));
> On 2015/07/20 16:11:16, vsm wrote:
> > I think you need to make a relative path wrt *from* here as well.
> 
> It's already a relative path. The (poorly named) runtimeFileOutput just
prepends
> "dev_compiler/runtime". So the path ends up as
> "dev_compiler/runtime/dart/core.js" for example. Which results in the HTML
> saying `<script src="dev_compiler/runtime/dart/core.js">` which is exactly
what
> we want. Then above, all of the runtime files are copied into the "htmlOutDir"
> (line 191), so you get a structure like:
> 
> sunflower/
> - sunflower.html
> - dev_compiler/
> - - runtime/
> - - - dart/
> - - - - core.js
> 
> The scripts match up with the HTML file's relative path.
> 
> This does mean multiple every HTML file gets a copy of the runtime next to it.
> This roughly matches what we used to do (and still do in server mode), for
> better or worse. I basically kept it because it matches what server does.
Though
> I agree, it's not right longer term.
> 

Hmm, if I have:

src/
- hello_world
  - index.html
  - index.dart
- todo
  - index.html
  - index.dart
  - todo.dart
  - css/

we seem to already generate:

- dev_compiler/...
- angular/...
- hello_world
  - index.html
  - index.js
...

I.e., one shared copy.  If I fix the links in the html files (i.e., add similar
relative path logic in the gen), everything seems to work.  

FWIW, I'm running in batch mode and serving separately - just to show multiple
apps sharing code.

> For demo purposes, could you have all of your HTML files in the same
directory?
> Alternatively, would "../" be okay in the HTML file, if the input base
directory
> was above the HTML? In that case, I'd probably have it output the runtime
files
> not as part of compileHtml, but as part of `run`. The invariant would be one
> batch compile == one runtime. If you want me to take that route, I'd prefer
> doing it in a follow up, as it might cause collateral test damage :)
> 
> Anyway, I'm happy to continue tweaking all of this stuff. It's just hard to
> think about the right thing when we still have our server. I feel like it has
> taken a lot of energy to try and maintain the system, when we know ultimately
> it's not the right direction w.r.t. analyzer integration. Not sure the
smartest
> approach at this point. I'm starting to think if we had a very simple file
> watcher (perhaps borrowing some code from analysis_server), that could get us
to
> a saner system in the meantime.

[Jennifer Messerly, 2015-07-21 21:13:49]

PTAL. I went ahead and did the follow up work, to treat runtime files at the
output of compiling dart:core (which should always be included, implicitly if
it's not explicitly imported).

[Jennifer Messerly, 2015-07-23 16:33:25]

Friendly ping, I think you'll like the new version! :)

[vsm, 2015-07-23 16:42:45]

lgtm!

https://codereview.chromium.org/1243693002/diff/40001/lib/src/compiler.dart
File lib/src/compiler.dart (right):

https://codereview.chromium.org/1243693002/diff/40001/lib/src/compiler.dart#n...
lib/src/compiler.dart:158: static const String _propertyName =
'dev_compiler.BatchCompiler.isCompiled';
Perhaps rename _propertyName to _isCompiled?

https://codereview.chromium.org/1243693002/diff/40001/lib/src/compiler.dart#n...
lib/src/compiler.dart:223: for (var file in defaultRuntimeFiles) {
Will this pick up dart:mirrors?  I think core doesn't actually depend on that.

[Jennifer Messerly, 2015-07-23 16:50:35]

https://codereview.chromium.org/1243693002/diff/40001/lib/src/compiler.dart
File lib/src/compiler.dart (right):

https://codereview.chromium.org/1243693002/diff/40001/lib/src/compiler.dart#n...
lib/src/compiler.dart:158: static const String _propertyName =
'dev_compiler.BatchCompiler.isCompiled';
On 2015/07/23 16:42:45, vsm wrote:
> Perhaps rename _propertyName to _isCompiled?

_propertyName is gone after I rebase against master.

https://codereview.chromium.org/1243693002/diff/40001/lib/src/compiler.dart#n...
lib/src/compiler.dart:223: for (var file in defaultRuntimeFiles) {
On 2015/07/23 16:42:45, vsm wrote:
> Will this pick up dart:mirrors?  I think core doesn't actually depend on that.

dart:mirrors depends on dart:core, so we always hit core eventually, at which
point we make the whole SDK available (everything hard coded in
defaultRuntimeFiles), which includes mirrors.

It's a good observation, though: we treat the whole SDK as a unit, which is no
longer true, since dart:mirrors and dart:js aren't part of the ginormous library
cycle that is core.

that's what my TODO was trying to explain, though not sure it captured the issue
well. I'll add another comment down by defaultRuntimeFiles.

We shouldn't really need these special cases at all: instead, including an SDK
lib should cause it to be "compiled" (which should be an instant cache hit,
assuming incremental works). But right now, that's not how things work.

[Jennifer Messerly, 2015-07-23 16:55:25]

Committed patchset #4 (id:60001) manually as
0b0f07faea680f4f8b3ffaa8854f82be4febc960 (presubmit successful).