[Webkit-unassigned] [Bug 39199] build-webkit should collect Visual Studio Express logs and display them

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=39199





--- Comment #24 from Daniel Bates <dbates at webkit.org>  2010-10-26 21:36:50 PST ---
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).

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.