Support command line parameters to specify a url to file name mapping. This is used during snapsh...

Support command line parameters to specify a url to file name mapping. This is used during snapshot generation so that a URL is mapped to a physical file name.
Committed: https://code.google.com/p/dart/source/detail?r=1550

[siva, 2011-11-12 02:23:51]

PTAL

The usage is as follows:

sivamach[runtime]>more /workspace/asiva/tmp/junk.dart 
#import("dart:dom1");
#import("dart:dom2");

main() {
 func1();
 func2();
 func3();
 func4();
}

::::::::::::::
/workspace/asiva/tmp/junk1.lib
::::::::::::::
#library("dart:dom1");
#source("junk3.dart");
func1() {
 print("func1");
}
::::::::::::::
/workspace/asiva/tmp/junk2.lib
::::::::::::::
#library("dart:dom2");
#source("junk4.dart");
func2() {
 print("func2");
}
::::::::::::::
/workspace/asiva/tmp/junk3.dart
::::::::::::::
func3() {
 print("func3");
}
::::::::::::::
/workspace/asiva/tmp/junk4.dart
::::::::::::::
func4() {
 print("func4");
}


tools/create_snapshot_file.py -v --executable=out/Debug_ia32/gen_snapshot_bin
--output_bin=/tmp/snapshot.bin --input_cc=bin/snapshot_in.cc
--output=/tmp/snapshot.cc --url_mapping=dart:dom1,/workspace/asiva/tmp/junk1.lib
--url_mapping=dart:dom2,/workspace/asiva/tmp/junk2.lib
--script=/workspace/asiva/tmp/junk.dart

[Anton Muhin, 2011-11-14 14:02:13]

http://codereview.chromium.org/8439061/diff/2005/bin/gen_snapshot.cc
File bin/gen_snapshot.cc (right):

http://codereview.chromium.org/8439061/diff/2005/bin/gen_snapshot.cc#newcode123
bin/gen_snapshot.cc:123: Dart_Handle result = Dart_StringToCString(url,
&url_chars);
nit: maybe TODO to switch to API like Dart_StringGet8 later on.

http://codereview.chromium.org/8439061/diff/8001/bin/gen_snapshot.cc
File bin/gen_snapshot.cc (right):

http://codereview.chromium.org/8439061/diff/8001/bin/gen_snapshot.cc#newcode118
bin/gen_snapshot.cc:118: int len = strlen(library_url_chars);
hoist len out of for loop?

http://codereview.chromium.org/8439061/diff/8001/bin/gen_snapshot.cc#newcode120
bin/gen_snapshot.cc:120: (url_name[len] == ',')) {
should we protect from that?  if yes, we may need better switches verification
with better reporting.

http://codereview.chromium.org/8439061/diff/8001/bin/gen_snapshot.cc#newcode122
bin/gen_snapshot.cc:122: if (strlen(url_mapped_name) != 0) {
ditto.

http://codereview.chromium.org/8439061/diff/8001/bin/gen_snapshot.cc#newcode133
bin/gen_snapshot.cc:133: static Dart_Handle CanonicalizeUrl(const char*
library_url_chars,
doesn't we have pretty similar code in process_script.cc?  do we want to
maintain both versions?

http://codereview.chromium.org/8439061/diff/8001/bin/gen_snapshot.cc#newcode193
bin/gen_snapshot.cc:193: url_chars = MapLibraryUrl(url_chars);
If I import mapped lib in mapped lib, won't we have a problem? 

..../dart-for.dart:
#import("dart:bar");

..../dart-bar.dart:
#import("dart:foo");

Let's consider loading of dart-foo.dart when it imports "dart:bar". 
Canonicalization will work fine, but when we come to importing, dart:bar will
get expanded to absolute path (in typical case), and we'll do Dart_LoadLibrary
with _rewritten_ URL hence not loading proper sources of dart:bar.

I think we should separate urls from which we load files (those should be
rewritten) and which are passed back into Dart API (those should be intact).

May we add a test for this case?

http://codereview.chromium.org/8439061/diff/8001/include/dart_api.h
File include/dart_api.h (right):

http://codereview.chromium.org/8439061/diff/8001/include/dart_api.h#newcode259
include/dart_api.h:259: DART_EXPORT bool Dart_Initialize(int argc, const char**
argv,
this will break embedders.  May we postpone this change and do it in a separate
CL, maybe before or after snapshotting stuff.  But that's not big deal if you'd
prefer to keep it as a single CL.

http://codereview.chromium.org/8439061/diff/8001/tools/create_snapshot_file.py
File tools/create_snapshot_file.py (right):

http://codereview.chromium.org/8439061/diff/8001/tools/create_snapshot_file.p...
tools/create_snapshot_file.py:100: script_args = [ ]
nit: I used to [] (w/o space form), but you should know Python style around
better.

[Anton Muhin, 2011-11-14 16:44:05]

http://codereview.chromium.org/8439061/diff/8001/bin/gen_snapshot.cc
File bin/gen_snapshot.cc (right):

http://codereview.chromium.org/8439061/diff/8001/bin/gen_snapshot.cc#newcode193
bin/gen_snapshot.cc:193: url_chars = MapLibraryUrl(url_chars);
Sorry, I now think you handle this case correctly (you pass original urls).  
But, please, double check.

On 2011/11/14 14:02:14, antonmuhin wrote:
> If I import mapped lib in mapped lib, won't we have a problem? 
> 
> ..../dart-for.dart:
> #import("dart:bar");
> 
> ..../dart-bar.dart:
> #import("dart:foo");
> 
> Let's consider loading of dart-foo.dart when it imports "dart:bar". 
> Canonicalization will work fine, but when we come to importing, dart:bar will
> get expanded to absolute path (in typical case), and we'll do Dart_LoadLibrary
> with _rewritten_ URL hence not loading proper sources of dart:bar.
> 
> I think we should separate urls from which we load files (those should be
> rewritten) and which are passed back into Dart API (those should be intact).
> 
> May we add a test for this case?

[siva, 2011-11-14 22:17:09]

Addressed comments.

http://codereview.chromium.org/8439061/diff/8001/bin/gen_snapshot.cc
File bin/gen_snapshot.cc (right):

http://codereview.chromium.org/8439061/diff/8001/bin/gen_snapshot.cc#newcode118
bin/gen_snapshot.cc:118: int len = strlen(library_url_chars);
On 2011/11/14 14:02:14, antonmuhin wrote:
> hoist len out of for loop?

Done.

http://codereview.chromium.org/8439061/diff/8001/bin/gen_snapshot.cc#newcode120
bin/gen_snapshot.cc:120: (url_name[len] == ',')) {
Not sure I understand what we should protect against?

This code just ensures that we only add URLs for which a mapping is specified to
the list.
e.g
--url_mapping=dart:xyz
--url_mapping=dart:xyz,
are not added to the map.

On 2011/11/14 14:02:14, antonmuhin wrote:
> should we protect from that?  if yes, we may need better switches verification
> with better reporting.

http://codereview.chromium.org/8439061/diff/8001/bin/gen_snapshot.cc#newcode122
bin/gen_snapshot.cc:122: if (strlen(url_mapped_name) != 0) {
Ditto.

On 2011/11/14 14:02:14, antonmuhin wrote:
> ditto.

http://codereview.chromium.org/8439061/diff/8001/bin/gen_snapshot.cc#newcode133
bin/gen_snapshot.cc:133: static Dart_Handle CanonicalizeUrl(const char*
library_url_chars,
I have refactored this to avoid the duplication.

On 2011/11/14 14:02:14, antonmuhin wrote:
> doesn't we have pretty similar code in process_script.cc?  do we want to
> maintain both versions?

http://codereview.chromium.org/8439061/diff/8001/bin/gen_snapshot.cc#newcode193
bin/gen_snapshot.cc:193: url_chars = MapLibraryUrl(url_chars);
Yes the canonicalization part just returns the URL "dart:.." as is the mapping
happens only when we load the library/source. The passed in URL will be the
original URL not the mapped one.

I have simplified this to return the mapped URL if one is specified as is
without trying to do any canonicalization because if a mapping is specified on
the command line we should use that.

On 2011/11/14 16:44:05, antonmuhin wrote:
> Sorry, I now think you handle this case correctly (you pass original urls).  
> But, please, double check.
> 
> On 2011/11/14 14:02:14, antonmuhin wrote:
> > If I import mapped lib in mapped lib, won't we have a problem? 
> > 
> > ..../dart-for.dart:
> > #import("dart:bar");
> > 
> > ..../dart-bar.dart:
> > #import("dart:foo");
> > 
> > Let's consider loading of dart-foo.dart when it imports "dart:bar". 
> > Canonicalization will work fine, but when we come to importing, dart:bar
will
> > get expanded to absolute path (in typical case), and we'll do
Dart_LoadLibrary
> > with _rewritten_ URL hence not loading proper sources of dart:bar.
> > 
> > I think we should separate urls from which we load files (those should be
> > rewritten) and which are passed back into Dart API (those should be intact).
> > 
> > May we add a test for this case?
>

http://codereview.chromium.org/8439061/diff/8001/include/dart_api.h
File include/dart_api.h (right):

http://codereview.chromium.org/8439061/diff/8001/include/dart_api.h#newcode259
include/dart_api.h:259: DART_EXPORT bool Dart_Initialize(int argc, const char**
argv,
Agreed, I will do this later.

On 2011/11/14 14:02:14, antonmuhin wrote:
> this will break embedders.  May we postpone this change and do it in a
separate
> CL, maybe before or after snapshotting stuff.  But that's not big deal if
you'd
> prefer to keep it as a single CL.

http://codereview.chromium.org/8439061/diff/8001/tools/create_snapshot_file.py
File tools/create_snapshot_file.py (right):

http://codereview.chromium.org/8439061/diff/8001/tools/create_snapshot_file.p...
tools/create_snapshot_file.py:100: script_args = [ ]
Changed to []

On 2011/11/14 14:02:14, antonmuhin wrote:
> nit: I used to [] (w/o space form), but you should know Python style around
> better.

Done.

[Anton Muhin, 2011-11-15 12:34:32]

Thanks a lot.  The only problem I can see is w/ check mapped_url_chars, except
for that it's LGTM.

http://codereview.chromium.org/8439061/diff/14001/bin/gen_snapshot.cc
File bin/gen_snapshot.cc (right):

http://codereview.chromium.org/8439061/diff/14001/bin/gen_snapshot.cc#newcode135
bin/gen_snapshot.cc:135: // Create the full path based on the including library
and the url.
nit: those comments can probably go away now.

http://codereview.chromium.org/8439061/diff/14001/bin/gen_snapshot.cc#newcode169
bin/gen_snapshot.cc:169: if (mapped_url_chars != NULL) {
it looks like MapLibraryUrl will never return NULL, isn't this check bogus?

http://codereview.chromium.org/8439061/diff/14001/bin/gen_snapshot.cc#newcode171
bin/gen_snapshot.cc:171: return Dart_NewString(mapped_url_chars);
is it ok?  you have kImportTag/kSourceTag, but do nothing here.

[siva, 2011-11-15 21:00:27]

http://codereview.chromium.org/8439061/diff/14001/bin/gen_snapshot.cc
File bin/gen_snapshot.cc (right):

http://codereview.chromium.org/8439061/diff/14001/bin/gen_snapshot.cc#newcode135
bin/gen_snapshot.cc:135: // Create the full path based on the including library
and the url.
On 2011/11/15 12:34:32, antonmuhin wrote:
> nit: those comments can probably go away now.

Done.

http://codereview.chromium.org/8439061/diff/14001/bin/gen_snapshot.cc#newcode169
bin/gen_snapshot.cc:169: if (mapped_url_chars != NULL) {
Good catch, changed MapLibraryUrl to initialize mapped_url_chars to NULL.

On 2011/11/15 12:34:32, antonmuhin wrote:
> it looks like MapLibraryUrl will never return NULL, isn't this check bogus?

http://codereview.chromium.org/8439061/diff/14001/bin/gen_snapshot.cc#newcode171
bin/gen_snapshot.cc:171: return Dart_NewString(mapped_url_chars);
In this case we found a mapping on the command line for the url so we return the
mapping specified on the command line for the URL.
e.g if url_mapping=dart:dom,/myfile/file.dart
is specified we would return "/mfile/file.dart" here.

On 2011/11/15 12:34:32, antonmuhin wrote:
> is it ok?  you have kImportTag/kSourceTag, but do nothing here.

[Anton Muhin, 2011-11-16 12:45:35]

http://codereview.chromium.org/8439061/diff/14001/bin/gen_snapshot.cc
File bin/gen_snapshot.cc (right):

http://codereview.chromium.org/8439061/diff/14001/bin/gen_snapshot.cc#newcode171
bin/gen_snapshot.cc:171: return Dart_NewString(mapped_url_chars);
Looks like you've submitted it, so let me just experiment.  But I am still
somewhat uneasy that for dart:foo we just return the string doing no
LoadScript/LoadLibrary for kImportTag/kSourceTag.

On 2011/11/15 21:00:27, asiva wrote:
> In this case we found a mapping on the command line for the url so we return
the
> mapping specified on the command line for the URL.
> e.g if url_mapping=dart:dom,/myfile/file.dart
> is specified we would return "/mfile/file.dart" here.
> 
> On 2011/11/15 12:34:32, antonmuhin wrote:
> > is it ok?  you have kImportTag/kSourceTag, but do nothing here.
>