Change lookup path for Dart native extensions shared libraries.

Change lookup path for Dart native extensions shared libraries.


BUG=
TEST=

Committed: https://code.google.com/p/dart/source/detail?r=5524

[Bill Hesse, 2012-03-13 17:53:29]

Still needs ports to Mac and Windows, and cleanup.

[Mads Ager (google), 2012-03-14 10:29:47]

https://chromiumcodereview.appspot.com/9694045/diff/1/runtime/bin/extensions.cc
File runtime/bin/extensions.cc (right):

https://chromiumcodereview.appspot.com/9694045/diff/1/runtime/bin/extensions....
runtime/bin/extensions.cc:20: if (strchr(library_name, '/') != library_name &&
Is this because the extension url has been fully expanded before we get here? In
that case, could you add a comment about that. Please also update the error
message below which makes no sense now.

https://chromiumcodereview.appspot.com/9694045/diff/1/runtime/bin/extensions....
runtime/bin/extensions.cc:30: strrchr(library_name, File::PathSeparator()[0]);
If you are using PathSeparator here, couldn't you use that above as well instead
of checking for both '\\' and '/'?

https://chromiumcodereview.appspot.com/9694045/diff/1/runtime/bin/extensions_...
File runtime/bin/extensions_linux.cc (right):

https://chromiumcodereview.appspot.com/9694045/diff/1/runtime/bin/extensions_...
runtime/bin/extensions_linux.cc:8: void* Extensions::LoadExtensionLibrary(const
char* library_path) {
The splitting up of the path into path and extension name should be done in
platform independent code. Then you can pass two pointers to here?

https://chromiumcodereview.appspot.com/9694045/diff/1/runtime/bin/extensions_...
runtime/bin/extensions_linux.cc:10: char* mutable_library_path =
Concatenate(library_path_strings);
This is just a strdup?

https://chromiumcodereview.appspot.com/9694045/diff/1/tests/standalone/src/io...
File tests/standalone/src/io/test_extension.dart (right):

https://chromiumcodereview.appspot.com/9694045/diff/1/tests/standalone/src/io...
tests/standalone/src/io/test_extension.dart:7:
#import("dart-ext:/usr/local/google/home/whesse/dart/test_extension");
Change before commit.

[Bill Hesse, 2012-03-14 17:15:04]

Comments addressed.

https://chromiumcodereview.appspot.com/9694045/diff/1/runtime/bin/extensions.cc
File runtime/bin/extensions.cc (right):

https://chromiumcodereview.appspot.com/9694045/diff/1/runtime/bin/extensions....
runtime/bin/extensions.cc:20: if (strchr(library_name, '/') != library_name &&
Eliminated.  We now allow paths.  Both absolute and relative paths should work,
but we will only be called with absolute paths.

On 2012/03/14 10:29:47, Mads Ager wrote:
> Is this because the extension url has been fully expanded before we get here?
In
> that case, could you add a comment about that. Please also update the error
> message below which makes no sense now.

https://chromiumcodereview.appspot.com/9694045/diff/1/runtime/bin/extensions....
runtime/bin/extensions.cc:30: strrchr(library_name, File::PathSeparator()[0]);
On 2012/03/14 10:29:47, Mads Ager wrote:
> If you are using PathSeparator here, couldn't you use that above as well
instead
> of checking for both '\\' and '/'?

Done.

https://chromiumcodereview.appspot.com/9694045/diff/1/runtime/bin/extensions_...
File runtime/bin/extensions_linux.cc (right):

https://chromiumcodereview.appspot.com/9694045/diff/1/runtime/bin/extensions_...
runtime/bin/extensions_linux.cc:8: void* Extensions::LoadExtensionLibrary(const
char* library_path) {
On 2012/03/14 10:29:47, Mads Ager wrote:
> The splitting up of the path into path and extension name should be done in
> platform independent code. Then you can pass two pointers to here?

Done.

https://chromiumcodereview.appspot.com/9694045/diff/1/runtime/bin/extensions_...
runtime/bin/extensions_linux.cc:10: char* mutable_library_path =
Concatenate(library_path_strings);
This is moved to the platform-independent code, and it is a strdup to remove
constness.
On 2012/03/14 10:29:47, Mads Ager wrote:
> This is just a strdup?

[Mads Ager (google), 2012-03-15 08:44:09]

https://chromiumcodereview.appspot.com/9694045/diff/9001/runtime/bin/extensio...
File runtime/bin/extensions.cc (right):

https://chromiumcodereview.appspot.com/9694045/diff/9001/runtime/bin/extensio...
runtime/bin/extensions.cc:22: char* library_path =
Concatenate(path_component_strings);
Instead of these two lines, please use strdup(path_component). You are just
duplicating a string and not concatenating multiple strings. :)

https://chromiumcodereview.appspot.com/9694045/diff/9001/runtime/bin/extensio...
File runtime/bin/extensions_linux.cc (right):

https://chromiumcodereview.appspot.com/9694045/diff/9001/runtime/bin/extensio...
runtime/bin/extensions_linux.cc:10: const char* strings[5] = { library_path,
"/lib",
Does this work if library_path is the empty string?

[Bill Hesse, 2012-03-15 13:08:54]

Comments addressed, tested on platforms with all types of library paths.

https://chromiumcodereview.appspot.com/9694045/diff/9001/runtime/bin/extensio...
File runtime/bin/extensions.cc (right):

https://chromiumcodereview.appspot.com/9694045/diff/9001/runtime/bin/extensio...
runtime/bin/extensions.cc:22: char* library_path =
Concatenate(path_component_strings);
On 2012/03/15 08:44:09, Mads Ager wrote:
> Instead of these two lines, please use strdup(path_component). You are just
> duplicating a string and not concatenating multiple strings. :)

Done.

https://chromiumcodereview.appspot.com/9694045/diff/9001/runtime/bin/extensio...
File runtime/bin/extensions_linux.cc (right):

https://chromiumcodereview.appspot.com/9694045/diff/9001/runtime/bin/extensio...
runtime/bin/extensions_linux.cc:10: const char* strings[5] = { library_path,
"/lib",
The library_path should always be an absolute file path.  This is checked in
LoadExtension in extensions.cc.

On 2012/03/15 08:44:09, Mads Ager wrote:
> Does this work if library_path is the empty string?

[Mads Ager (google), 2012-03-15 14:14:47]

LGTM

This should make it possible to build and run the test extension automatically
soon, right?

[Bill Hesse, 2012-03-15 15:47:44]

The only step missing is copying the library to tests/standalone/src/io. 
Everything else works already.
Then the tests would run, and pass.  But this puts a changed file into
tests/standaolne/src/io.

This is a violation of the idea that all changed code should be in the build
directory, so we could alternatively make the whole thing a generated test, that
copies everything to a test directory.

On 2012/03/15 14:14:47, Mads Ager wrote:
> LGTM
> 
> This should make it possible to build and run the test extension automatically
> soon, right?

[Ivan Posva, 2012-03-15 16:18:20]

https://chromiumcodereview.appspot.com/9694045/diff/7011/runtime/bin/builtin....
File runtime/bin/builtin.dart (right):

https://chromiumcodereview.appspot.com/9694045/diff/7011/runtime/bin/builtin....
runtime/bin/builtin.dart:72: var uri = new Uri.fromString(userString);
Please add comments why dart-ext URIs need special handling.

https://chromiumcodereview.appspot.com/9694045/diff/7011/runtime/bin/builtin....
runtime/bin/builtin.dart:85: String filePathFromUri(String userUri) {
This function essentially does what you are doing manually in C++ under ifdef.

https://chromiumcodereview.appspot.com/9694045/diff/7011/runtime/bin/extensio...
File runtime/bin/extensions.cc (right):

https://chromiumcodereview.appspot.com/9694045/diff/7011/runtime/bin/extensio...
runtime/bin/extensions.cc:21: #ifdef TARGET_OS_WINDOWS
Please no ifdefs, they are generally a sign of bad abstractions.

https://chromiumcodereview.appspot.com/9694045/diff/7011/runtime/bin/extensio...
File runtime/bin/extensions_linux.cc (right):

https://chromiumcodereview.appspot.com/9694045/diff/7011/runtime/bin/extensio...
runtime/bin/extensions_linux.cc:10: const char* strings[5] = { library_path,
"/lib",
Just leave out the size. You are NULL terminating this array anyway.

[Bill Hesse, 2012-03-15 16:24:34]

Sorry I didn't see these for committing - I will fix these tomorrow PM in MTV,
or Monday.  I'm leaving for MTV in a few hours.


On 2012/03/15 16:18:20, Ivan Posva wrote:
>
https://chromiumcodereview.appspot.com/9694045/diff/7011/runtime/bin/builtin....
> File runtime/bin/builtin.dart (right):
> 
> 
https://chromiumcodereview.appspot.com/9694045/diff/7011/runtime/bin/builtin....
> runtime/bin/builtin.dart:72: var uri = new Uri.fromString(userString);
> Please add comments why dart-ext URIs need special handling.
> 
>
https://chromiumcodereview.appspot.com/9694045/diff/7011/runtime/bin/builtin....
> runtime/bin/builtin.dart:85: String filePathFromUri(String userUri) {
> This function essentially does what you are doing manually in C++ under ifdef.
> 
>
https://chromiumcodereview.appspot.com/9694045/diff/7011/runtime/bin/extensio...
> File runtime/bin/extensions.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9694045/diff/7011/runtime/bin/extensio...
> runtime/bin/extensions.cc:21: #ifdef TARGET_OS_WINDOWS
> Please no ifdefs, they are generally a sign of bad abstractions.
> 
>
https://chromiumcodereview.appspot.com/9694045/diff/7011/runtime/bin/extensio...
> File runtime/bin/extensions_linux.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9694045/diff/7011/runtime/bin/extensio...
> runtime/bin/extensions_linux.cc:10: const char* strings[5] = { library_path,
> "/lib",
> Just leave out the size. You are NULL terminating this array anyway.

[Bill Hesse, 2012-04-17 14:46:31]

Comments addressed in changelist
https://chromiumcodereview.appspot.com/10103031/

https://chromiumcodereview.appspot.com/9694045/diff/7011/runtime/bin/builtin....
File runtime/bin/builtin.dart (right):

https://chromiumcodereview.appspot.com/9694045/diff/7011/runtime/bin/builtin....
runtime/bin/builtin.dart:72: var uri = new Uri.fromString(userString);
On 2012/03/15 16:18:20, Ivan Posva wrote:
> Please add comments why dart-ext URIs need special handling.

This special function has been dropped, and its functionality merged into
_resolveUri.

https://chromiumcodereview.appspot.com/9694045/diff/7011/runtime/bin/builtin....
runtime/bin/builtin.dart:85: String filePathFromUri(String userUri) {
On 2012/03/15 16:18:20, Ivan Posva wrote:
> This function essentially does what you are doing manually in C++ under ifdef.

Yes, and with a refactoring, the existing call of this function does the right
thing for the extensions case as well.

https://chromiumcodereview.appspot.com/9694045/diff/7011/runtime/bin/extensio...
File runtime/bin/extensions.cc (right):

https://chromiumcodereview.appspot.com/9694045/diff/7011/runtime/bin/extensio...
runtime/bin/extensions.cc:21: #ifdef TARGET_OS_WINDOWS
On 2012/03/15 16:18:20, Ivan Posva wrote:
> Please no ifdefs, they are generally a sign of bad abstractions.

Done.

https://chromiumcodereview.appspot.com/9694045/diff/7011/runtime/bin/extensio...
File runtime/bin/extensions_linux.cc (right):

https://chromiumcodereview.appspot.com/9694045/diff/7011/runtime/bin/extensio...
runtime/bin/extensions_linux.cc:10: const char* strings[5] = { library_path,
"/lib",
On 2012/03/15 16:18:20, Ivan Posva wrote:
> Just leave out the size. You are NULL terminating this array anyway.

Done.