Make Platform.executable return a fully qualified path the the executable

Make Platform.executable return the fully qualified path of the executable

BUG=http://dartbug.com/16994
R=kustermann@google.com, lrn@google.com

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

[kustermann, 2015-05-20 10:19:56]

lgtm

https://codereview.chromium.org/1145053002/diff/40002/runtime/bin/platform_wi...
File runtime/bin/platform_win.cc (right):

https://codereview.chromium.org/1145053002/diff/40002/runtime/bin/platform_wi...
runtime/bin/platform_win.cc:88: const int kTmpBufferSize = 32768;
Isn't there a much shorter maximum path length?

[Lasse Reichstein Nielsen, 2015-05-20 10:48:43]

lgtm

https://codereview.chromium.org/1145053002/diff/40002/runtime/bin/platform.h
File runtime/bin/platform.h (right):

https://codereview.chromium.org/1145053002/diff/40002/runtime/bin/platform.h#...
runtime/bin/platform.h:46: executable_name_ = executable_name;
Should this set ..._resolved_ to true?

https://codereview.chromium.org/1145053002/diff/40002/runtime/bin/platform.h#...
runtime/bin/platform.h:84: static bool executable_name_resolved_;
Is this really necessary?
As opposed to just checking if executable_name_ is NULL?

The only difference is that we might try resolving more than once if it fails
resolving, which is probably fine.

I'd expect we could cache the result on the Dart side, and only call this
function once per isolate. It it returns null, I don't mind calling it again for
each isolate that cares, since that should rarely happen (right?).

https://codereview.chromium.org/1145053002/diff/40002/runtime/bin/platform_ma...
File runtime/bin/platform_macos.cc (right):

https://codereview.chromium.org/1145053002/diff/40002/runtime/bin/platform_ma...
runtime/bin/platform_macos.cc:85: if (_NSGetExecutablePath(path, &path_size) !=
-1) {
Could "!= -1" be ">= 0".
(I just don't like unnamed constants other than 0 :)

https://codereview.chromium.org/1145053002/diff/40002/runtime/bin/platform_wi...
File runtime/bin/platform_win.cc (right):

https://codereview.chromium.org/1145053002/diff/40002/runtime/bin/platform_wi...
runtime/bin/platform_win.cc:88: const int kTmpBufferSize = 32768;
Not according to https://msdn.microsoft.com/en-us/library/aa365247(VS.85).aspx
You have to do extra work to use long paths, but they can work.

https://codereview.chromium.org/1145053002/diff/40002/tests/standalone/io/pla...
File tests/standalone/io/platform_test.dart (right):

https://codereview.chromium.org/1145053002/diff/40002/tests/standalone/io/pla...
tests/standalone/io/platform_test.dart:31:
Expect.equals(Platform.executable.substring(1, 3), ':\\');
That's slightly optimistic in general, but probably fine for this test.
Windows can use UNC names "\\foo\bar\baz" and long names "\\?\.." and even
combinations.

Can we make a test where the executable is at a UNC-named location?

[Søren Gjesse, 2015-05-20 11:14:25]

https://codereview.chromium.org/1145053002/diff/40002/runtime/bin/platform.h
File runtime/bin/platform.h (right):

https://codereview.chromium.org/1145053002/diff/40002/runtime/bin/platform.h#...
runtime/bin/platform.h:46: executable_name_ = executable_name;
On 2015/05/20 10:48:43, Lasse Reichstein Nielsen wrote:
> Should this set ..._resolved_ to true?

This is being called from main() with the value of argv[0], which was what we
previously used for Platform.executable. This is still the fallback value is
somehow the system calls to find the absolute path fails.

https://codereview.chromium.org/1145053002/diff/40002/runtime/bin/platform.h#...
runtime/bin/platform.h:84: static bool executable_name_resolved_;
On 2015/05/20 10:48:43, Lasse Reichstein Nielsen wrote:
> Is this really necessary?
> As opposed to just checking if executable_name_ is NULL?
> 
> The only difference is that we might try resolving more than once if it fails
> resolving, which is probably fine.
> 
> I'd expect we could cache the result on the Dart side, and only call this
> function once per isolate. It it returns null, I don't mind calling it again
for
> each isolate that cares, since that should rarely happen (right?).

See above. We have a fallback to argv[0].

https://codereview.chromium.org/1145053002/diff/40002/runtime/bin/platform_ma...
File runtime/bin/platform_macos.cc (right):

https://codereview.chromium.org/1145053002/diff/40002/runtime/bin/platform_ma...
runtime/bin/platform_macos.cc:85: if (_NSGetExecutablePath(path, &path_size) !=
-1) {
On 2015/05/20 10:48:43, Lasse Reichstein Nielsen wrote:
> Could "!= -1" be ">= 0".
> (I just don't like unnamed constants other than 0 :)

Changed to "== 0" then. The documentation explicitly says either -1 or 0.

https://codereview.chromium.org/1145053002/diff/40002/runtime/bin/platform_wi...
File runtime/bin/platform_win.cc (right):

https://codereview.chromium.org/1145053002/diff/40002/runtime/bin/platform_wi...
runtime/bin/platform_win.cc:88: const int kTmpBufferSize = 32768;
On 2015/05/20 10:19:56, kustermann wrote:
> Isn't there a much shorter maximum path length?

According to
https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247%28v=vs.85%2...
extended-length path (with the "\\?\" prefix) can be that long, and
GetModuleFileNameW will return in the format used to start the process. So if a
process was started using the "\\?\" prefix the path can be that long.

https://codereview.chromium.org/1145053002/diff/40002/tests/standalone/io/pla...
File tests/standalone/io/platform_test.dart (right):

https://codereview.chromium.org/1145053002/diff/40002/tests/standalone/io/pla...
tests/standalone/io/platform_test.dart:31:
Expect.equals(Platform.executable.substring(1, 3), ':\\');
On 2015/05/20 10:48:43, Lasse Reichstein Nielsen wrote:
> That's slightly optimistic in general, but probably fine for this test.
> Windows can use UNC names "\\foo\bar\baz" and long names "\\?\.." and even
> combinations.
> 
> Can we make a test where the executable is at a UNC-named location?
> 

True I am assuming that tests (both locally and on the bots) are running off a
location referred to by a drive letter. I don't think we have a share we can use
for testing.

Added a comment. If it gives problems I can work more on it.

[Søren Gjesse, 2015-05-20 11:14:56]

Committed patchset #5 (id:70001) manually as
e03ab1743717c39099a9161999e231699098a811 (presubmit successful).

[Bill Hesse, 2015-05-20 13:44:05]

whesse@google.com changed reviewers:
+ whesse@google.com

[Bill Hesse, 2015-05-20 13:44:06]

https://codereview.chromium.org/1145053002/diff/70001/runtime/bin/platform_an...
File runtime/bin/platform_android.cc (right):

https://codereview.chromium.org/1145053002/diff/70001/runtime/bin/platform_an...
runtime/bin/platform_android.cc:9: 
I think you need to include bin/file.h here.  Android compilation is failing.

[Søren Gjesse, 2015-05-20 13:56:33]

https://codereview.chromium.org/1145053002/diff/70001/runtime/bin/platform_an...
File runtime/bin/platform_android.cc (right):

https://codereview.chromium.org/1145053002/diff/70001/runtime/bin/platform_an...
runtime/bin/platform_android.cc:9: 
On 2015/05/20 13:44:06, Bill Hesse wrote:
> I think you need to include bin/file.h here.  Android compilation is failing.

Thanks Bill. Fixed in a separate CL.

[kevmoo, 2015-05-20 19:45:54]

On 2015/05/20 13:56:33, Søren Gjesse wrote:
>
https://codereview.chromium.org/1145053002/diff/70001/runtime/bin/platform_an...
> File runtime/bin/platform_android.cc (right):
> 
>
https://codereview.chromium.org/1145053002/diff/70001/runtime/bin/platform_an...
> runtime/bin/platform_android.cc:9: 
> On 2015/05/20 13:44:06, Bill Hesse wrote:
> > I think you need to include bin/file.h here.  Android compilation is
failing.
> 
> Thanks Bill. Fixed in a separate CL.

DBC: could we have tests that validate the expected behavior?

dart in Path
dart in a symlinked directory
dart exe is sym-linked

etc...