Port page_cycler_http to Mac

Port page_cycler_http to Mac
Accompanying buildbot changes @ http://codereview.chromium.org/67234

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

[Mark Mentovai, 2009-04-16 22:58:07]

I guess most of these are comments about the state of whatever this got copied
from and the interfaces that they need to adhere to.

http://codereview.chromium.org/79017/diff/1019/1022
File chrome/test/page_cycler/page_cycler_test.cc (right):

http://codereview.chromium.org/79017/diff/1019/1022#newcode270
Line 270: // TODO(nirnimesh): Intl1, Intl2 crash Chromium on Mac. Revisit later.
Why's this changing?

Actually, shouldn't the comment be above PageCyclerTest.Intl1File (the previous
TEST_F)?

http://codereview.chromium.org/79017/diff/1019/1024
File tools/python/google/httpd_config/httpd2_mac.conf (right):

http://codereview.chromium.org/79017/diff/1019/1024#newcode8
Line 8: Listen 127.0.0.1:8000
Wow, do we really need all of these ports?

http://codereview.chromium.org/79017/diff/1019/1024#newcode43
Line 43: LoadModule rewrite_module libexec/apache2/mod_rewrite.so
I'm sure we can pare this list down.

See chrome-internal/trunk/tools/buildbot/setup/mac/httpd.conf for ideas.

http://codereview.chromium.org/79017/diff/1019/1020
File tools/python/google/httpd_utils.py (right):

http://codereview.chromium.org/79017/diff/1019/1020#newcode179
Line 179: default=False, help='Starts Apache 2 instead of Apache 1.3 (default).'
Needs a space or two inside the string after the period.

http://codereview.chromium.org/79017/diff/1019/1023
File tools/python/google/platform_utils_mac.py (right):

http://codereview.chromium.org/79017/diff/1019/1023#newcode15
Line 15: base_dir: a directory where we want output to go
Is that really base_dir and not output_dir?  With the comment here now, it seems
weird that there's a member called _output_dir.

http://codereview.chromium.org/79017/diff/1019/1023#newcode35
Line 35: return "/tmp"
os.getenv('TMPDIR', '/tmp') but it doesn't really matter.

http://codereview.chromium.org/79017/diff/1019/1023#newcode42
Line 42: DocumentRoot; for a file URI, the full path to the file.
If it's a full path to the file, will it be an absolute path but without the
leading slash?  Seems like that's what the code below is expecting.

http://codereview.chromium.org/79017/diff/1019/1023#newcode53
Line 53: path = path.replace("\\", "/")
Really?

http://codereview.chromium.org/79017/diff/1019/1023#newcode54
Line 54: return "%s://127.0.0.1:%s/%s" % (protocol, str(port), path)
You can use %d for the middle one so that you don't need the str().

http://codereview.chromium.org/79017/diff/1019/1023#newcode93
Line 93: "user": os.environ.get("USERNAME", os.environ.get("USER", "")),
Don't even bother looking for USERNAME.  This can just be
os.environ.get('USER').

http://codereview.chromium.org/79017/diff/1019/1023#newcode97
Line 97: # Failed to get the username from the environment; use whoami instead.
Yuck.  Do this instead:

pwd.getpwuid(os.geteuid())[0]

Actually, you don't even need a username, Apache should accept a numeric UID if
prefixed by a '#'.  So just use:

httpd_vars['user'] = '#%d' % os.geteuid()

You can even put that in the block above, it's way more reliable than the
environment.

http://codereview.chromium.org/79017/diff/1019/1023#newcode123
Line 123: httpd_cmd_string = (httpd_cmd_string +
Why is this not just part of the above?

http://codereview.chromium.org/79017/diff/1019/1023#newcode142
Line 142: return ["kill", str(open(pid_file).readline().strip())]
This must be some sorta joke.  The interface to kill an httpd we started is to
run some external command?  Even though we likely have access to its pid and can
send it a signal ourselves?

Sigh.

http://codereview.chromium.org/79017/diff/1019/1023#newcode144
Line 144: # Don't have pid file. Kill all httpd processes
This is stupid.  No pid file means no httpd where one was expected.  It's not
running, don't go hunting for other httpds.

[TVL, 2009-04-16 23:26:07]

http://codereview.chromium.org/79017/diff/1019/1024
File tools/python/google/httpd_config/httpd2_mac.conf (right):

http://codereview.chromium.org/79017/diff/1019/1024#newcode2
Line 2: 
should this be a svn cp from windows/linux so some history is there for what
goes into this file?  then the history will show the specific edits from that
starting point.

http://codereview.chromium.org/79017/diff/1019/1023
File tools/python/google/platform_utils_mac.py (right):

http://codereview.chromium.org/79017/diff/1019/1023#newcode2
Line 2: # Use of this source code is governed by a BSD-style license that can be
should this be a svn cp also?

[Nirnimesh, 2009-04-17 13:34:54]

You're right, I had hastily just made the win script to work for mac, leaving it
ugly.
I've addressed all the comments. Please take a re-look.

Thanks

http://codereview.chromium.org/79017/diff/1019/1022
File chrome/test/page_cycler/page_cycler_test.cc (right):

http://codereview.chromium.org/79017/diff/1019/1022#newcode270
Line 270: // TODO(nirnimesh): Intl1, Intl2 crash Chromium on Mac. Revisit later.
On 2009/04/16 22:58:07, Mark Mentovai wrote:
> Why's this changing?
> 
> Actually, shouldn't the comment be above PageCyclerTest.Intl1File (the
previous
> TEST_F)?

Oops! My bad. Fixed.
Thanks

http://codereview.chromium.org/79017/diff/1019/1024
File tools/python/google/httpd_config/httpd2_mac.conf (right):

http://codereview.chromium.org/79017/diff/1019/1024#newcode2
Line 2: 
On 2009/04/16 23:26:07, TVL wrote:
> should this be a svn cp from windows/linux so some history is there for what
> goes into this file?  then the history will show the specific edits from that
> starting point.
Good point. Done.

http://codereview.chromium.org/79017/diff/1019/1024#newcode8
Line 8: Listen 127.0.0.1:8000
On 2009/04/16 22:58:07, Mark Mentovai wrote:
> Wow, do we really need all of these ports?
Well, page_cycler needs only 8000. But I'm not sure if this same httpd config
would be reused for some other test.

Removing.

http://codereview.chromium.org/79017/diff/1019/1024#newcode43
Line 43: LoadModule rewrite_module libexec/apache2/mod_rewrite.so
On 2009/04/16 22:58:07, Mark Mentovai wrote:
> I'm sure we can pare this list down.
> 
> See chrome-internal/trunk/tools/buildbot/setup/mac/httpd.conf for ideas.

Shouldn't this be the same as for windows?

http://codereview.chromium.org/79017/diff/1019/1020
File tools/python/google/httpd_utils.py (right):

http://codereview.chromium.org/79017/diff/1019/1020#newcode179
Line 179: default=False, help='Starts Apache 2 instead of Apache 1.3 (default).'
On 2009/04/16 22:58:07, Mark Mentovai wrote:
> Needs a space or two inside the string after the period.

Done.

http://codereview.chromium.org/79017/diff/1019/1023
File tools/python/google/platform_utils_mac.py (right):

http://codereview.chromium.org/79017/diff/1019/1023#newcode2
Line 2: # Use of this source code is governed by a BSD-style license that can be
On 2009/04/16 23:26:07, TVL wrote:
> should this be a svn cp also?

Done.

http://codereview.chromium.org/79017/diff/1019/1023#newcode15
Line 15: base_dir: a directory where we want output to go
On 2009/04/16 22:58:07, Mark Mentovai wrote:
> Is that really base_dir and not output_dir?  With the comment here now, it
seems
> weird that there's a member called _output_dir.

Fixed.

http://codereview.chromium.org/79017/diff/1019/1023#newcode35
Line 35: return "/tmp"
On 2009/04/16 22:58:07, Mark Mentovai wrote:
> os.getenv('TMPDIR', '/tmp') but it doesn't really matter.

Done.

http://codereview.chromium.org/79017/diff/1019/1023#newcode42
Line 42: DocumentRoot; for a file URI, the full path to the file.
On 2009/04/16 22:58:07, Mark Mentovai wrote:
> If it's a full path to the file, will it be an absolute path but without the
> leading slash?  Seems like that's what the code below is expecting.
Fixed

http://codereview.chromium.org/79017/diff/1019/1023#newcode53
Line 53: path = path.replace("\\", "/")
On 2009/04/16 22:58:07, Mark Mentovai wrote:
> Really?
Leftover from win. Removed

http://codereview.chromium.org/79017/diff/1019/1023#newcode54
Line 54: return "%s://127.0.0.1:%s/%s" % (protocol, str(port), path)
On 2009/04/16 22:58:07, Mark Mentovai wrote:
> You can use %d for the middle one so that you don't need the str().

Done.

http://codereview.chromium.org/79017/diff/1019/1023#newcode93
Line 93: "user": os.environ.get("USERNAME", os.environ.get("USER", "")),
On 2009/04/16 22:58:07, Mark Mentovai wrote:
> Don't even bother looking for USERNAME.  This can just be
> os.environ.get('USER').

Done.

http://codereview.chromium.org/79017/diff/1019/1023#newcode97
Line 97: # Failed to get the username from the environment; use whoami instead.
On 2009/04/16 22:58:07, Mark Mentovai wrote:
> Yuck.  Do this instead:
> 
> pwd.getpwuid(os.geteuid())[0]
> 
> Actually, you don't even need a username, Apache should accept a numeric UID
if
> prefixed by a '#'.  So just use:
> 
> httpd_vars['user'] = '#%d' % os.geteuid()
> 
> You can even put that in the block above, it's way more reliable than the
> environment.

Done. Merged with above.

http://codereview.chromium.org/79017/diff/1019/1023#newcode123
Line 123: httpd_cmd_string = (httpd_cmd_string +
On 2009/04/16 22:58:07, Mark Mentovai wrote:
> Why is this not just part of the above?

Done.

http://codereview.chromium.org/79017/diff/1019/1023#newcode142
Line 142: return ["kill", str(open(pid_file).readline().strip())]
On 2009/04/16 22:58:07, Mark Mentovai wrote:
> This must be some sorta joke.  The interface to kill an httpd we started is to
> run some external command?  Even though we likely have access to its pid and
can
> send it a signal ourselves?
> 
> Sigh.
This class didn't really start httpd. It just provided the command to do so.
The interface of the user (the slave, in this case) is such that it calls
GetStartHttpdCommand() & GetStopHttpdCommand() at about the same time, and saves
the two commands. At this point, no httpd really exists, hence no real pid.

http://codereview.chromium.org/79017/diff/1019/1023#newcode144
Line 144: # Don't have pid file. Kill all httpd processes
On 2009/04/16 22:58:07, Mark Mentovai wrote:
> This is stupid.  No pid file means no httpd where one was expected.  It's not
> running, don't go hunting for other httpds.
Yes, another win cruft ported mindlessly. Remodeled to use httpd's '-k stop'.
Thanks

[Mark Mentovai, 2009-04-17 17:55:32]

lg

http://codereview.chromium.org/79017/diff/5011/5016
File tools/python/google/httpd_config/httpd2_mac.conf (right):

http://codereview.chromium.org/79017/diff/5011/5016#newcode37
Line 37: LoadModule rewrite_module libexec/apache2/mod_rewrite.so
Bleh, now that this is a copy I'll say OK, but I really think that the fat ought
to be trimmed.  We don't need most of this stuff.

http://codereview.chromium.org/79017/diff/5011/5015
File tools/python/google/platform_utils_mac.py (right):

http://codereview.chromium.org/79017/diff/5011/5015#newcode121
Line 121: httpd_cmd = [self._bash, "-c", self._httpd_cmd_string]
There's no great reason to put this to bash as a string instead of invoking
httpd directly, but again, since this is a copy, OK for now.

[TVL, 2009-04-17 18:08:41]

lg

[Nirnimesh, 2009-04-17 18:11:23]

http://codereview.chromium.org/79017/diff/5011/5016
File tools/python/google/httpd_config/httpd2_mac.conf (right):

http://codereview.chromium.org/79017/diff/5011/5016#newcode37
Line 37: LoadModule rewrite_module libexec/apache2/mod_rewrite.so
On 2009/04/17 17:55:32, Mark Mentovai wrote:
> Bleh, now that this is a copy I'll say OK, but I really think that the fat
ought
> to be trimmed.  We don't need most of this stuff.

Ok, making it thinner. Added a warning note that it differs from the win config.