Add url_request_unittest to mac build.

Add url_request_unittest to mac build.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=5712

[eroman, 2008-11-18 05:52:32]

First stab at adding url_request_unittest to mac build.

The tests pass (provided you install tls), however something still remains to be
fixed for the autorelease pool, as it logs many lines like:

*** _NSAutoreleaseNoPool(): Object 0x1221880 of class NSPathStore2 autoreleased
with no pool in place - just leaking

[eroman, 2008-11-18 07:46:34]

Ok, resolved the autorelease pool issue by extending PlatformTest, and changing
to TEST_F.

The only remaining issue now is it won't cleanup rogue testserver processes.

[wtc, 2008-11-18 18:07:59]

http://codereview.chromium.org/11224/diff/209/212
File base/process_util_mac.mm (right):

http://codereview.chromium.org/11224/diff/209/212#newcode78
Line 78: return true;
Should this stub return false instead?
Do you return true because there are no output parameters?

http://codereview.chromium.org/11224/diff/209/211
File net/url_request/url_request_unittest.h (right):

http://codereview.chromium.org/11224/diff/209/211#newcode302
Line 302: bool tlslite_installed = !access("/usr/local/bin/tls.py", X_OK);
It may be safer to reverse the check, like this:

#if defined(OS_MACOSX)
    bool tlslite_installed = !access("/usr/local/bin/tls.py", X_OK);
#else
    bool tlslite_installed = !access("/usr/bin/tls.py", X_OK);
#endif

You could also just conditionalize the installation path
of TLSLite, because the rest of the code is the same.

http://codereview.chromium.org/11224/diff/209/211#newcode307
Line 307: command_line.push_back("/usr/bin/python");
Are you sure you want to hardcode the installation path of
python?

[eroman, 2008-11-18 18:51:23]

http://codereview.chromium.org/11224/diff/209/212
File base/process_util_mac.mm (right):

http://codereview.chromium.org/11224/diff/209/212#newcode78
Line 78: return true;
On 2008/11/18 18:07:59, wtc wrote:
> Should this stub return false instead?
> Do you return true because there are no output parameters?

Changed to false. A return value of true indicates that all processes exited
cleanly within the wait_milliseconds delay.

http://codereview.chromium.org/11224/diff/209/211
File net/url_request/url_request_unittest.h (right):

http://codereview.chromium.org/11224/diff/209/211#newcode302
Line 302: bool tlslite_installed = !access("/usr/local/bin/tls.py", X_OK);
On 2008/11/18 18:07:59, wtc wrote:
> It may be safer to reverse the check, like this:
> 
> #if defined(OS_MACOSX)
>     bool tlslite_installed = !access("/usr/local/bin/tls.py", X_OK);
> #else
>     bool tlslite_installed = !access("/usr/bin/tls.py", X_OK);
> #endif
> 
> You could also just conditionalize the installation path
> of TLSLite, because the rest of the code is the same.

I've gotten rid of the preprocessor branch all-together, by simply checking both
locations:

bool tlslite_installed = !access("usr/bin/tls.py", X_OK) ||
!access("usr/local/bin/tls.py, X_OK);

Can you tell me more about your proposed fix?

http://codereview.chromium.org/11224/diff/209/211#newcode307
Line 307: command_line.push_back("/usr/bin/python");
On 2008/11/18 18:07:59, wtc wrote:
> Are you sure you want to hardcode the installation path of
> python?

Not sure at all. The reason I changed it to a full path rather than name, is
that the Mac port's version of LaunchApp does not support just name. On linux it
uses execvp(), so it looks for "python" in the search paths. On Mac it uses
posix_spawn, which expects the path to the image (so "python" fails).

I will leave it up to the porting experts to iron out this inconsistency, but I
suspect /usr/bin/python is a fairly common install path for python.

[eroman, 2008-11-18 19:13:16]

> How about updating
> http://src.chromium.org/svn/trunk/src/build/install-build-deps.sh
> to support Mac OS X, and have it install tlslite
> (possibly from http://py-tlslite.darwinports.com/ )?

That is a good idea. However I am not too strong with the mac-foo, so I may need
some hand-holding. Hopefully I can just punt and worry about setting-up tls.py
on the buildbots/trybots for now.

[wtc, 2008-11-18 19:23:53]

http://codereview.chromium.org/11224/diff/209/211
File net/url_request/url_request_unittest.h (right):

http://codereview.chromium.org/11224/diff/209/211#newcode302
Line 302: bool tlslite_installed = !access("/usr/local/bin/tls.py", X_OK);
On 2008/11/18 18:51:23, eroman wrote:
>
> I've gotten rid of the preprocessor branch all-together, by simply checking
both
> locations:
> 
> bool tlslite_installed = !access("usr/bin/tls.py", X_OK) ||
> !access("usr/local/bin/tls.py, X_OK);
> 
> Can you tell me more about your proposed fix?

I like your new code better, so we can ignore my proposed
fix.

http://codereview.chromium.org/11224/diff/209/211#newcode307
Line 307: command_line.push_back("/usr/bin/python");
On 2008/11/18 18:51:23, eroman wrote:
>
> Not sure at all. The reason I changed it to a full path rather than name, is
> that the Mac port's version of LaunchApp does not support just name. On linux
it
> uses execvp(), so it looks for "python" in the search paths. On Mac it uses
> posix_spawn, which expects the path to the image (so "python" fails).

I see.  Thanks for the explanation.

[wtc, 2008-11-18 19:32:01]

http://codereview.chromium.org/11224/diff/209/211
File net/url_request/url_request_unittest.h (right):

http://codereview.chromium.org/11224/diff/209/211#newcode307
Line 307: command_line.push_back("/usr/bin/python");
On 2008/11/18 18:51:23, eroman wrote:
>
> Not sure at all. The reason I changed it to a full path rather than name, is
> that the Mac port's version of LaunchApp does not support just name. On linux
it
> uses execvp(), so it looks for "python" in the search paths. On Mac it uses
> posix_spawn, which expects the path to the image (so "python" fails).

Mark, since Mac OS X can also do fork + execvp, should we
change base::LaunchApp to use the conventional Unix
fork + execvp sequence on Mac OS X?

[eroman, 2008-11-18 19:40:49]

> Why not switch it to use posix_spawnp, then?

Good idea, done!

[Amanda Walker, 2008-11-19 01:12:04]

On 2008/11/18 19:32:01, wtc wrote:
> Mark, since Mac OS X can also do fork + execvp, should we
> change base::LaunchApp to use the conventional Unix
> fork + execvp sequence on Mac OS X?

Because fork+execvp will usually break any executable that uses any native
(non-POSIX) APIs (though you can sometimes get away with it with great care). 
Mach port rights do not get transferred to the child, which causes problem with
many Mac system libraries.  posix_spawn and posix_spawnp do not have this
problem.