Bug 39199 - build-webkit should collect Visual Studio Express logs and display them
Summary: build-webkit should collect Visual Studio Express logs and display them
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-17 00:25 PDT by Eric Seidel (no email)
Modified: 2011-01-20 22:44 PST (History)
17 users (show)

See Also:


Attachments
Patch (5.84 KB, patch)
2010-09-29 22:47 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
patch with intentional compile failure for testing the win-ews (6.32 KB, patch)
2010-09-30 16:40 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Fix Dave's suggestions (5.88 KB, patch)
2010-10-26 16:46 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2010-05-17 00:25:52 PDT
#!/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.
Comment 1 Eric Seidel (no email) 2010-09-29 22:47:08 PDT
Created attachment 69313 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-09-29 22:48:41 PDT
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.
Comment 3 Adam Barth 2010-09-29 23:07:53 PDT
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?
Comment 4 Adam Barth 2010-09-29 23:08:42 PDT
I have slight worries about destroying someone's build environment, but I'm not sure how to proceed except to try.
Comment 5 Eric Seidel (no email) 2010-09-30 01:55:29 PDT
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!
Comment 6 Eric Seidel (no email) 2010-09-30 01:56:33 PDT
Comment on attachment 69313 [details]
Patch

Once the win-ews approves this patch, it can be cq+'d
Comment 7 Eric Seidel (no email) 2010-09-30 16:40:57 PDT
Created attachment 69394 [details]
patch with intentional compile failure for testing the win-ews
Comment 8 Early Warning System Bot 2010-09-30 16:51:29 PDT
Attachment 69394 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4162036
Comment 9 WebKit Review Bot 2010-09-30 16:55:01 PDT
Attachment 69394 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4216037
Comment 10 Eric Seidel (no email) 2010-09-30 16:57:38 PDT
Attachment 69394 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4244003
Comment 11 WebKit Review Bot 2010-09-30 18:05:55 PDT
Attachment 69394 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4141042
Comment 12 Brent Fulgham 2010-09-30 21:20:47 PDT
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?
Comment 13 Eric Seidel (no email) 2010-09-30 23:33:00 PDT
Yes, I'm trying to test the win-ews failing. :)
Comment 14 WebKit Review Bot 2010-10-01 01:26:38 PDT
Attachment 69394 [details] did not build on win:
Build output: http://queues.webkit.org/results/4222023
Comment 15 Adam Barth 2010-10-01 07:34:11 PDT
Can't exec "WebKitTools/Scripts/print-vse-failure-logs": No such file or directory at WebKitTools/Scripts/build-webkit line 482.

:(
Comment 16 David Levin 2010-10-05 13:09:14 PDT
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.
Comment 17 David Kilzer (:ddkilzer) 2010-10-24 08:10:08 PDT
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;
Comment 18 Eric Seidel (no email) 2010-10-26 16:43:01 PDT
(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.
Comment 19 Eric Seidel (no email) 2010-10-26 16:44:04 PDT
(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!
Comment 20 Eric Seidel (no email) 2010-10-26 16:46:08 PDT
Created attachment 71964 [details]
Fix Dave's suggestions
Comment 21 David Kilzer (:ddkilzer) 2010-10-26 21:11:55 PDT
Comment on attachment 71964 [details]
Fix Dave's suggestions

Thanks!  r=me
Comment 22 WebKit Commit Bot 2010-10-26 21:29:25 PDT
Comment on attachment 71964 [details]
Fix Dave's suggestions

Clearing flags on attachment: 71964

Committed r70607: <http://trac.webkit.org/changeset/70607>
Comment 23 WebKit Commit Bot 2010-10-26 21:29:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Daniel Bates 2010-10-26 21:36:50 PDT
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).
Comment 25 David Kilzer (:ddkilzer) 2010-10-26 21:43:56 PDT
(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.
Comment 26 Eric Seidel (no email) 2010-10-27 14:58:01 PDT
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.
Comment 27 Eric Seidel (no email) 2010-10-27 14:58:26 PDT
I may make further improvements to the script once I have the win-ews bot up and running with it.  We'll see.
Comment 28 Eric Seidel (no email) 2010-10-27 15:38:01 PDT
Committed r70720: <http://trac.webkit.org/changeset/70720>
Comment 29 Eric Seidel (no email) 2010-10-27 15:39:06 PDT
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!
Comment 30 guru 2011-01-20 22:44:07 PST
I am getting the error with Visual studio express edition setup.