Look up executable on PATH when given environment variables.

Look up executable on PATH when given environment variables.

Fixes http://code.google.com/p/dart/issues/detail?id=4402

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

[Bob Nystrom, 2012-10-25 23:58:02]

This bug is blocking me getting the Linux pub buildbot working, so I went ahead
and applied Peter's fix. It fixes the issue I'm seeing on Mac. I applied the
same change to the Linux code since it's identical, but haven't tested it since
I don't have a Linux machine.

Thanks for the fix, Peter!

[ahe, 2012-10-26 03:39:09]

I don't see a test of this. I think the description is imprecise: this is not
about searching in PATH, but passing the new environment on.

So a simple test would be to pass the environment on to a sub process which exit
code depends on an environment variable being set.

[Mads Ager (google), 2012-10-26 08:43:44]

LGTM

https://codereview.chromium.org/11260048/diff/1/runtime/bin/process_linux.cc
File runtime/bin/process_linux.cc (right):

https://codereview.chromium.org/11260048/diff/1/runtime/bin/process_linux.cc#...
runtime/bin/process_linux.cc:465: char **orig_environ = environ;
char** orig_environ

However, I don't think there is a reason to save the old environment. We are in
the child process and if exec fails we are going to exit the child anyway and it
doesn't matter if we reset it.

https://codereview.chromium.org/11260048/diff/1/runtime/bin/process_linux.cc#...
runtime/bin/process_linux.cc:471: // Dead code, unless execvp fails.
I think we can remove this. It doesn't matter, does it?

[Mads Ager (google), 2012-10-26 08:44:49]

I was a bit fast, please add a test and then it will look good. :)

[Bob Nystrom, 2012-10-30 18:15:59]

On 2012/10/26 03:39:09, ahe wrote:
> I don't see a test of this. I think the description is imprecise: this is not
> about searching in PATH, but passing the new environment on.

It's about both. Before this patch, if you pass in an environment, then this
implicitly disables looking up the executable on the PATH. (It may be that in
the old code you also didn't get the passed in environment either, but that's
irrelevant in my case. If it can't find the executable to spawn at all, the
environment variables are the least of my concerns!)

> So a simple test would be to pass the environment on to a sub process which
exit
> code depends on an environment variable being set.

I think process_environment_test might cover this.

[Bob Nystrom, 2012-10-30 18:16:59]

https://codereview.chromium.org/11260048/diff/1/runtime/bin/process_linux.cc
File runtime/bin/process_linux.cc (right):

https://codereview.chromium.org/11260048/diff/1/runtime/bin/process_linux.cc#...
runtime/bin/process_linux.cc:465: char **orig_environ = environ;
On 2012/10/26 08:43:44, Mads Ager wrote:
> char** orig_environ
> 
> However, I don't think there is a reason to save the old environment. We are
in
> the child process and if exec fails we are going to exit the child anyway and
it
> doesn't matter if we reset it.

Removed.

https://codereview.chromium.org/11260048/diff/1/runtime/bin/process_linux.cc#...
runtime/bin/process_linux.cc:471: // Dead code, unless execvp fails.
On 2012/10/26 08:43:44, Mads Ager wrote:
> I think we can remove this. It doesn't matter, does it?

I don't think so? Done.

[Bob Nystrom, 2012-10-30 18:17:32]

On 2012/10/26 08:44:49, Mads Ager wrote:
> I was a bit fast, please add a test and then it will look good. :)

Added some tests.

[ahe, 2012-10-31 06:46:46]

https://codereview.chromium.org/11260048/diff/7001/tests/standalone/io/proces...
File tests/standalone/io/process_path_environment_test.dart (right):

https://codereview.chromium.org/11260048/diff/7001/tests/standalone/io/proces...
tests/standalone/io/process_path_environment_test.dart:5: // Test that the
executable is looked up on the user's PATH when spawning a
I don't think this test matches its description. It runs programs that are in
the system's default path, not necessarily the user's.

https://codereview.chromium.org/11260048/diff/7001/tests/standalone/io/proces...
tests/standalone/io/process_path_environment_test.dart:17:
options.environment['whatever'] = 'something';
You're not testing that this environment variable is passed on to the sub
process.

[ahe, 2012-10-31 06:59:17]

https://codereview.chromium.org/11260048/diff/1/runtime/bin/process_linux.cc
File runtime/bin/process_linux.cc (right):

https://codereview.chromium.org/11260048/diff/1/runtime/bin/process_linux.cc#...
runtime/bin/process_linux.cc:465: char **orig_environ = environ;
On 2012/10/26 08:43:44, Mads Ager wrote:
> char** orig_environ
> 
> However, I don't think there is a reason to save the old environment. We are
in
> the child process and if exec fails we are going to exit the child anyway and
it
> doesn't matter if we reset it.

I don't understand your reasoning.

ReportChildError calls strerror before exiting the process. strerror is affected
by locale (which is controlled by environment variables). Although there is no
call to setlocale, there is no way to know how error reporting code is going to
evolve.

[ahe, 2012-10-31 07:00:34]

On 2012/10/30 18:15:59, Bob Nystrom wrote:
> On 2012/10/26 03:39:09, ahe wrote:
> > I don't see a test of this. I think the description is imprecise: this is
not
> > about searching in PATH, but passing the new environment on.
> 
> It's about both. Before this patch, if you pass in an environment, then this
> implicitly disables looking up the executable on the PATH. (It may be that in
> the old code you also didn't get the passed in environment either, but that's
> irrelevant in my case. If it can't find the executable to spawn at all, the
> environment variables are the least of my concerns!)

I understand what this change does, and I do not think the description matches
the changes you made.

I do not think the tests are testing the changes you have made, and we are
likely to see regressions.

> 
> > So a simple test would be to pass the environment on to a sub process which
> exit
> > code depends on an environment variable being set.
> 
> I think process_environment_test might cover this.

[Bob Nystrom, 2012-11-01 20:53:03]

https://codereview.chromium.org/11260048/diff/7001/tests/standalone/io/proces...
File tests/standalone/io/process_path_environment_test.dart (right):

https://codereview.chromium.org/11260048/diff/7001/tests/standalone/io/proces...
tests/standalone/io/process_path_environment_test.dart:5: // Test that the
executable is looked up on the user's PATH when spawning a
On 2012/10/31 06:46:46, ahe wrote:
> I don't think this test matches its description. It runs programs that are in
> the system's default path, not necessarily the user's.

My unix knowledge is weak. What's the difference between the system's default
path and the user's?

https://codereview.chromium.org/11260048/diff/7001/tests/standalone/io/proces...
tests/standalone/io/process_path_environment_test.dart:17:
options.environment['whatever'] = 'something';
On 2012/10/31 06:46:46, ahe wrote:
> You're not testing that this environment variable is passed on to the sub
> process.

That's OK, I think. There are other tests that environment variables are
correctly passed. The intent of this test is just to ensure that path lookup
still happens when any environment variables happen to be provided.

[Bob Nystrom, 2012-11-01 20:57:19]

On 2012/10/31 07:00:34, ahe wrote:
> On 2012/10/30 18:15:59, Bob Nystrom wrote:
> > On 2012/10/26 03:39:09, ahe wrote:
> > > I don't see a test of this. I think the description is imprecise: this is
> not
> > > about searching in PATH, but passing the new environment on.
> > 
> > It's about both. Before this patch, if you pass in an environment, then this
> > implicitly disables looking up the executable on the PATH. (It may be that
in
> > the old code you also didn't get the passed in environment either, but
that's
> > irrelevant in my case. If it can't find the executable to spawn at all, the
> > environment variables are the least of my concerns!)
> 
> I understand what this change does, and I do not think the description matches
> the changes you made.

I'm not sure what would be a better description.

The bug was that if you passed any environment variables, dart:io would fail to
look up the executable on the PATH. This patch fixes that. I think the
description is clear?

> I do not think the tests are testing the changes you have made,

If you take out the change, then process_path_environment_test will fail. The
other test I added doesn't but that's because that test is essentially a bonus.
dart:io *was* looking up executables on PATH if you didn't provide environment
variables, but there were no tests at all to validate that, so I added one.

> and we are likely to see regressions.

Regressions of what?

[ahe, 2012-11-01 21:16:46]

Hi Bob,

I think I confused myself about this. You're right: the change is about using a
different version of exec that searches in PATH, and instead pass the
environment through the global variable "environment". I now see that you're
testing it well, and what I was thinking about would be a poor test. Sorry for
the noise.

Cheers,
Peter

On 2012/11/01 20:57:19, Bob Nystrom wrote:
> On 2012/10/31 07:00:34, ahe wrote:
> > On 2012/10/30 18:15:59, Bob Nystrom wrote:
> > > On 2012/10/26 03:39:09, ahe wrote:
> > > > I don't see a test of this. I think the description is imprecise: this
is
> > not
> > > > about searching in PATH, but passing the new environment on.
> > > 
> > > It's about both. Before this patch, if you pass in an environment, then
this
> > > implicitly disables looking up the executable on the PATH. (It may be that
> in
> > > the old code you also didn't get the passed in environment either, but
> that's
> > > irrelevant in my case. If it can't find the executable to spawn at all,
the
> > > environment variables are the least of my concerns!)
> > 
> > I understand what this change does, and I do not think the description
matches
> > the changes you made.
> 
> I'm not sure what would be a better description.
> 
> The bug was that if you passed any environment variables, dart:io would fail
to
> look up the executable on the PATH. This patch fixes that. I think the
> description is clear?
> 
> > I do not think the tests are testing the changes you have made,
> 
> If you take out the change, then process_path_environment_test will fail. The
> other test I added doesn't but that's because that test is essentially a
bonus.
> dart:io *was* looking up executables on PATH if you didn't provide environment
> variables, but there were no tests at all to validate that, so I added one.
> 
> > and we are likely to see regressions.
> 
> Regressions of what?