Make startup_tests build and run on Linux (except reference tests).

Make startup_tests build and run on Linux (except reference tests).

[Paweł Hajdan Jr., 2009-02-26 22:31:36]

This contains parts of my other reviews, which I couldn't commit because of
closed tree: http://codereview.chromium.org/28212 and
http://codereview.chromium.org/27220

Please review:

agl: main part (feel free to add more reviewers)
sgk: SCons part. And I don't know how to ensure in startup_tests.scons that
Hammer/chrome is up2date. I previously tried with Depends, now Requires... both
don't work (if I start with a clobbered build, building startup_tests builds
chrome, but when everything is built and I change some dependency of chrome
making it outdated, it is not rebuilt when I build startup_tests)

[agl, 2009-02-27 03:28:41]

http://codereview.chromium.org/27240/diff/1/12
File chrome/test/automation/automation_proxy.cc (right):

http://codereview.chromium.org/27240/diff/1/12#newcode411
Line 411: map.push_back(std::pair<int, int>(src_fd, dest_fd));
std::make_pair is probably better here.

http://codereview.chromium.org/27240/diff/1/14
File chrome/test/startup/startup_test.cc (left):

http://codereview.chromium.org/27240/diff/1/14#oldcode22
Line 22: bool CopyFileWrapper(const std::wstring &src, const std::wstring &dest)
{
It doesn't seem safe to get rid of this. Why do you believe it to be unnecessary
now?

[Paweł Hajdan Jr., 2009-02-27 17:10:36]

On 2009/02/27 03:28:41, agl wrote:
> http://codereview.chromium.org/27240/diff/1/12
> File chrome/test/automation/automation_proxy.cc (right):
> 
> http://codereview.chromium.org/27240/diff/1/12#newcode411
> Line 411: map.push_back(std::pair<int, int>(src_fd, dest_fd));
> std::make_pair is probably better here.

Done.

> http://codereview.chromium.org/27240/diff/1/14
> File chrome/test/startup/startup_test.cc (left):
> 
> http://codereview.chromium.org/27240/diff/1/14#oldcode22
> Line 22: bool CopyFileWrapper(const std::wstring &src, const std::wstring
&dest)
> {
> It doesn't seem safe to get rid of this. Why do you believe it to be
unnecessary
> now?

It is unnecessary. I use a function which evicts file from disk cache instead of
hackily copying files around.

sgk: I wouls still need some help with scons, please look at my earlier messages
and TODO in startup_tests.scons.

[Paweł Hajdan Jr., 2009-02-27 17:23:23]

agl: I forgot to add base/test_file_util.cc. Please make sure to review it.

[agl, 2009-02-27 18:16:04]

http://codereview.chromium.org/27240/diff/1033/1035
File base/test_file_util_posix.cc (right):

http://codereview.chromium.org/27240/diff/1033/1035#newcode47
Line 47: DCHECK(suffix[0] == '/');
DCHECK_EQ

http://codereview.chromium.org/27240/diff/1033/1035#newcode54
Line 54: if (mkdir(target_path.value().c_str(), 0777) != 0) {
0777 is very permissive. Why not 0755?

http://codereview.chromium.org/27240/diff/1033/1035#newcode90
Line 90: LOG(WARNING) << "skipping symbolic link.";
This warning should be a little more verbose.

LOG(WARNING) << "Skipping symlink in CopyRecursiveDirNoCache"
             << " filename:" << ent->fts_path;

http://codereview.chromium.org/27240/diff/1033/1035#newcode93
Line 93: LOG(WARNING) << "skipping weird file.";
Likewise with the warning above, also "weird" isn't helpful :)

LOG(WARNING) << "Skipping non-regular file in CopyRecursiveDirNoCache"
             << " filename:" << ent->fts_path;

http://codereview.chromium.org/27240/diff/1033/1035#newcode112
Line 112: LOG(ERROR) << strerror(error);
Again, probably best to give some context in this error message.

[Paweł Hajdan Jr., 2009-02-27 22:17:12]

Patch updated. I'll do above fixes in a follow-up, both in
{test_,}file_util_posix.cc.

[sgk, 2009-02-27 23:14:28]

Hmm, Depends() should do exactly what you want.  If there's a dependency on the
chrome executable, then any time it changes, then startup_tests should be
rebuilt.  When you tried with Depends(), did it only work some times, or did it
not seem to work at all?

Requires() definitely will *not* do what you want, because it's what Make calls
an "order-only prerequisite:"  it will update the chrome executable first, but
*not* make chrome a real dependency--that is, it will not rebuild startup_tests
if chrome changes.

The most common reason why Depends() might not always trigger a "downstream"
rebuild when the chrome executable is rebuilt is if the new checksum of the
newly-build chrome executable matches the last checksum, SCons will be "smart"
and recognize you don't need to rebuild downstream dependencies.  So if you
(e.g.) remove a .o file to force a recompile and then a rebuild of chrome, but
didn't actually change any code, it will recognize that you just built the same
chrome bits and decide that startup_tests is already up to date (which it is). 
If it seemed like it only worked "sometimes" on Linux, that's likely the cause. 
(This isn't generally an issue if the compiler inserts timestamp info in the
object file headers, of course, because then the MD5 checksums change every
recompile.)

The other issue here is that the CL says you want to "build *and run*" on Linux.
 As far as I can see, SCons should be taking care of the build portion fine
(when you use Depends()), but I'm not seeing what in the configuration you're
intending to have trigger the run.  Am I overlooking something?  To actually run
the startup tests, I'd expect to see something like:

    startup_tests = env.ChromeTestProgram('startup_tests', input_files)
    env.AddPostAction(startup_tests, startup_tests[0])

That should run the just-built startup_tests executable, essentially as side
effect of the link.  (You may need to turn startup_tests[0] into a string or an
Action.)

(There are other idioms that people sometimes use to make sure startup_tests is
run every build, whether or not startup_tests is relinked, but I don't think
that's what we want here.)

[Paweł Hajdan Jr., 2009-03-02 17:40:29]

agl: patch updated.

sgk: Thanks for explaining. I removed dependency-related code for now. I would
like it to behave like on Windows/vcproj: when I build startup_tests, the build
system ensures that chrome is up to date. But it may be better to check this in
for now, and to proper deps tracking in a follow-up if it's non-trivial.

[agl, 2009-03-02 18:39:25]

LGTM

[Paweł Hajdan Jr., 2009-03-03 10:09:29]

sgk: ping. I'm waiting for your input.

Please say if you think I can commit the patch in current version (scons parts).
Then I would do proper dependency tracking in a follow-up (if it's complicated
or otherwise problematic, I wouldn't want it to block this patch; we should be
able to ensure on buildbot that chrome is up to date).

Then please try to suggest how should I properly indicate dependencies (building
startup_tests should ensure that chrome is up to date) - and if I can just
commit with those changes, or should show you the patch for review.

[Paweł Hajdan Jr., 2009-03-04 20:55:08]

agl: Please take another look. I had to change few things after syncing with
trunk to make startup_tests work reliably (make sure that browser gets closed
before launching new one). You can ignore process_util bits. They got reviewed
separately in http://codereview.chromium.org/40119 .

[Paweł Hajdan Jr., 2009-03-05 09:45:49]

agl: I just made one change in process_util (it was my code I recently
commited): when status was -1, I should return true in CrashAwareSleep instead
of false (because that means the process may exist). It was breaking
startup_tests for Linux and I didn't wan't to further block this patch, so I
commited.

I'll submit a follow-up patch for process_util_posix to correct a small issue in
it (this "may" in explanation above).