build-webkit should collect Visual Studio Express logs and display them

[Eric Seidel (no email)]

#!/usr/bin/python

build-webkit should collect Visual Studio Express logs and display them

Here is one example script, written in python:

#!/usr/bin/python
from __future__ import with_statement

import codecs
import os.path
import os

webkit_root = "/mnt/svn/webkit-win-ews"
build_root = os.path.join(webkit_root, "WebKitBuild")
obj_root = os.path.join(build_root, "obj")

build_log_paths = []

for dirpath, dirnames, filenames in os.walk(obj_root):
   for file_name in filenames:
       if file_name == "BuildLog.htm":
           file_path = os.path.join(dirpath, file_name)
           build_log_paths.append(file_path)


for build_log_path in build_log_paths:
    with codecs.open(build_log_path, "r", "utf-16") as build_log:
        print build_log.read()

Ideally we could wire up a hook in build-webkit itself to have them print to the console as they're generated.  But at least we need a script like the one above for the win-ews.

[Eric Seidel (no email)]

Created attachment 69313 [details]
Patch

[Eric Seidel (no email)]

I've not tested this yet, but I'm about to.  I'm very interested in thoughts from folks who actually use windows!

We need something like this to make the win-ews useful.

Yes, I know it just dumps the HTML, but that's *way* better than nothing.

[Adam Barth]

Comment on attachment 69313 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69313&action=review

> WebKitTools/Scripts/print-vse-failure-logs:69
> +            with codecs.open(build_log_path, "r", "utf-16") as build_log:

for reals?

[Adam Barth]

I have slight worries about destroying someone's build environment, but I'm not sure how to proceed except to try.

[Eric Seidel (no email)]

Comment on attachment 69313 [details]
Patch

Actually, lets leave this r? until the win-ews gets a chance. :)  That's the simplest way to test this patch!

[Eric Seidel (no email)]

Comment on attachment 69313 [details]
Patch

Once the win-ews approves this patch, it can be cq+'d

[Eric Seidel (no email)]

Created attachment 69394 [details]
patch with intentional compile failure for testing the win-ews

[Early Warning System Bot]

Attachment 69394 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4162036

[WebKit Review Bot]

Attachment 69394 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4216037

[Eric Seidel (no email)]

Attachment 69394 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4244003

[WebKit Review Bot]

Attachment 69394 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4141042

[Brent Fulgham]

Comment on attachment 69394 [details]
patch with intentional compile failure for testing the win-ews

View in context: https://bugs.webkit.org/attachment.cgi?id=69394&action=review

Looks great -- I'd love to try it out!

> WebCore/page/Frame.cpp:-201
> -}

Did you mean to include this fix for WebCore/page/Frame.cpp?

[Eric Seidel (no email)]

Yes, I'm trying to test the win-ews failing. :)

[WebKit Review Bot]

Attachment 69394 [details] did not build on win:
Build output: http://queues.webkit.org/results/4222023

[Adam Barth]

Can't exec "WebKitTools/Scripts/print-vse-failure-logs": No such file or directory at WebKitTools/Scripts/build-webkit line 482.

:(

[David Levin]

Comment on attachment 69394 [details]
patch with intentional compile failure for testing the win-ews

It looks like you got your intention failure that the patch was intended to provoke.

[David Kilzer (:ddkilzer)]

Comment on attachment 69313 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69313&action=review

r- to fix the issues in the Perl code.  Otherwise it looks good.

> WebKitTools/Scripts/build-webkit:479
> +            system("$scriptDir/print-vse-failure-logs");

This needs to use File::Spec->catfile() now:

    system(File::Spec->catfile($scriptDir, "print-vse-failure-logs"));

> WebKitTools/Scripts/print-vse-failure-logs:53
> +            print filenames

Do you really want to print all of these out?

> WebKitTools/Scripts/webkitdirs.pm:1193
> +    return $willUseVCExpressWhenBuilding;

You need to call determineConfigurationForVisualStudio() first to guarantee that this variable is set properly:

    determineConfigurationForVisualStudio()
    return $willUseVCExpressWhenBuilding;

[Eric Seidel (no email)]

(In reply to comment #17)
> (From update of attachment 69313 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69313&action=review
> 
> r- to fix the issues in the Perl code.  Otherwise it looks good.
> 
> > WebKitTools/Scripts/build-webkit:479
> > +            system("$scriptDir/print-vse-failure-logs");
> 
> This needs to use File::Spec->catfile() now:
> 
>     system(File::Spec->catfile($scriptDir, "print-vse-failure-logs"));

Thanks!

> > WebKitTools/Scripts/print-vse-failure-logs:53
> > +            print filenames
> 
> Do you really want to print all of these out?

The whole idea with this script is to put something minimal in place that people who actually use windows can make better. :)

> > WebKitTools/Scripts/webkitdirs.pm:1193
> > +    return $willUseVCExpressWhenBuilding;
> 
> You need to call determineConfigurationForVisualStudio() first to guarantee that this variable is set properly:
> 
>     determineConfigurationForVisualStudio()
>     return $willUseVCExpressWhenBuilding;

Fixed.  Thanks.

[Eric Seidel (no email)]

(In reply to comment #18)
> (In reply to comment #17)
> > > WebKitTools/Scripts/print-vse-failure-logs:53
> > > +            print filenames
> > 
> > Do you really want to print all of these out?
> 
> The whole idea with this script is to put something minimal in place that people who actually use windows can make better. :)

But this was infact left-over debug code.  Removed.  Thanks!

[Eric Seidel (no email)]

Created attachment 71964 [details]
Fix Dave's suggestions

[David Kilzer (:ddkilzer)]

Comment on attachment 71964 [details]
Fix Dave's suggestions

Thanks!  r=me

[WebKit Commit Bot]

Comment on attachment 71964 [details]
Fix Dave's suggestions

Clearing flags on attachment: 71964

Committed r70607: <http://trac.webkit.org/changeset/70607>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

David Kilzer already reviewed this patch and it landed. Here are some remarks I had:

View in context: https://bugs.webkit.org/attachment.cgi?id=71964&action=review

> WebKitTools/ChangeLog:11
> +        * Scripts/build-webkit:
> +        * Scripts/print-vse-failure-logs: Added.
> +        * Scripts/webkitdirs.pm:
> +

I suggest that you add a comment about the added function usingVisualStudioExpress() since prepare-ChangeLog does not generate a list of added/modified Perl functions. We should consider adding Perl support to prepare-ChangeLog.

> WebKitTools/Scripts/print-vse-failure-logs:64
> +        print "Found %s Visual Studio Express Build Logs:%s" % (len(build_log_paths), "\n".join(build_log_paths))

Take build_log_paths = ["A", "B"]. Then this line would print:

Found 2 Visual Studio Express Build Logs:A
B

Notice, "A" prints on the first output line.

I think it would be nicer to output something of the form:

Found 2 Visual Studio Express Build Logs:
A
B

Hence, I would add a new-line character ('\n') after "Logs:", such that the line reads:
print "Found %s Visual Studio Express Build Logs:\n%s" % (len(build_log_paths), "\n".join(build_log_paths)).

Do you foresee this command ever being called on its own and/or when there are zero BuildLog.htm files? If so, we may want to make the colon (':') after "Logs:" conditional on there being at least one path in the list build_log_paths. When we do not find any files whose name is BuildLog.htm then we output something of the form:

Found 0 Visual Studio Express Build Logs.

OR

No Visual Studio Express Build Logs found.

> WebKitTools/Scripts/webkitdirs.pm:1191
> +    determineConfigurationForVisualStudio();

Notice, the variable willUseVCExpressWhenBuilding is only set in setupCygwinEnv(). And determineConfigurationForVisualStudio() only calls setupCygwinEnv() if we are building a debug build as per line 286 of webkitdirs.pm <http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitdirs.pm?rev=70188#L286>.

So, we should call setupCygwinEnv() instead of determineConfigurationForVisualStudio(). Alternatively, we could make determineConfigurationForVisualStudio() always call setupCygwinEnv() (given determineConfigurationForVisualStudio() hasn't been called before; e.g. $configurationForVisualStudio is not defined).

[David Kilzer (:ddkilzer)]

(In reply to comment #24)
> David Kilzer already reviewed this patch and it landed. Here are some remarks I had:

Sorry about that Dan.  I probably shouldn't have set the cq+ flag as Eric didn't request it.

[Eric Seidel (no email)]

Thank you dan and david for the reviews.

@dan:  Honestly this script is mostly a shot in the dark.  I intended it to be half-baked.  Someone who actually uses windows needs to make it work nice.  But even as it is it's better than the current non-existant logs for VSE builds.

[Eric Seidel (no email)]

I may make further improvements to the script once I have the win-ews bot up and running with it.  We'll see.

[Eric Seidel (no email)]

Committed r70720: <http://trac.webkit.org/changeset/70720>

[Eric Seidel (no email)]

I did a follow-up commit to fix some issues getting this to run on the win-ews bot.  (The script was originally written on the win-ews bot, but I prepared the patch on my mac, and did final testing on my mac.  I had planned to do one final test on the EWS bot but got distracted yesterday.)

Thanks again for the reviews!

[guru]

I am getting the error with Visual studio express edition setup.