Bug 116079

Summary: [Windows] Update ORWT to know about the Windows 8 SDK and Debug Tools
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: Tools / TestsAssignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, bfulgham, commit-queue, dbates, ddkilzer, roger_fong, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch ddkilzer: review+

Description Brent Fulgham 2013-05-13 16:45:43 PDT
Many of the Windows debugging tools referenced in our build instructions are no longer easily accessible to people just starting to create a build environment. For example, the "Debugging Tools" download package is now replaced with a "Windows 8 SDK" installer that includes the debugging tools as part of its payload.

Since these paths have changed, and most new users will not be able to run tests, we should update old-run-webkit-tests to find the debugging tools in the appropriate spots.
Comment 1 Radar WebKit Bug Importer 2013-05-13 17:02:22 PDT
<rdar://problem/13881971>
Comment 2 Brent Fulgham 2013-05-13 17:42:25 PDT
Created attachment 201650 [details]
Patch
Comment 3 Benjamin Poulain 2013-05-13 22:11:06 PDT
Comment on attachment 201650 [details]
Patch

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

> Tools/ChangeLog:24
> -2013-05-13  Zan Dobersek  <zdobersek@igalia.com>
> +

Uh?

> Tools/Scripts/old-run-webkit-tests:2846
> +    my $ntsdPath = File::Spec->catfile(toCygwinPath($ENV{PROGRAMFILES}), "Windows Kits", "8.0", "Debuggers", "x64", "ntsd.exe");
>      unless (-f $ntsdPath) {
> -        $ntsdPath = File::Spec->catfile(toCygwinPath($ENV{ProgramW6432}), "Debugging Tools for Windows (x64)", "ntsd.exe");
> +        $ntsdPath = File::Spec->catfile(toCygwinPath($ENV{PROGRAMFILES}), "Windows Kits", "8.0", "Debuggers", "x86", "ntsd.exe");
>          unless (-f $ntsdPath) {
> -            $ntsdPath = File::Spec->catfile(toCygwinPath($ENV{SYSTEMROOT}), "system32", "ntsd.exe");
> +            $ntsdPath = File::Spec->catfile(toCygwinPath($ENV{PROGRAMFILES}), "Debugging Tools for Windows (x86)", "ntsd.exe");
>              unless (-f $ntsdPath) {
> -                print STDERR "Can't find ntsd.exe. Crash logs will not be saved.\nSee <http://trac.webkit.org/wiki/BuildingOnWindows#GettingCrashLogs>.\n";
> -                return;
> +                $ntsdPath = File::Spec->catfile(toCygwinPath($ENV{ProgramW6432}), "Debugging Tools for Windows (x64)", "ntsd.exe");
> +                unless (-f $ntsdPath) {
> +                    $ntsdPath = File::Spec->catfile(toCygwinPath($ENV{SYSTEMROOT}), "system32", "ntsd.exe");
> +                    unless (-f $ntsdPath) {
> +                        print STDERR "Can't find ntsd.exe. Crash logs will not be saved.\nSee <http://trac.webkit.org/wiki/BuildingOnWindows#GettingCrashLogs>.\n";
> +                        return;
> +                    }
> +                }
>              }
>          }
>      }

Instead of this crazy nesting, I think we should have a loop going over a list of stuff we are looking for.

> Tools/Scripts/webkitdirs.pm:1657
> +return;

Uh?
Comment 4 Brent Fulgham 2013-05-14 10:09:03 PDT
Created attachment 201729 [details]
Patch
Comment 5 Brent Fulgham 2013-05-14 10:10:17 PDT
Cleaned up the messy patch, and switched to a loop over the possible locations for ntsd.exe.
Comment 6 David Kilzer (:ddkilzer) 2013-05-14 10:47:11 PDT
Comment on attachment 201729 [details]
Patch

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

r=me

> Tools/Scripts/old-run-webkit-tests:2844
> +    my $ntsdPath = shift @possiblePaths;
> +
> +    while (not -f $ntsdPath) {
> +        if (!@possiblePaths) {
> +            print STDERR "Can't find ntsd.exe. Crash logs will not be saved.\nSee <http://trac.webkit.org/wiki/BuildingOnWindows#GettingCrashLogs>.\n";
> +            return;
>          }
> +
> +        $ntsdPath = shift @possiblePaths;

I think this might be a little clearer written as a for() loop, but I'm also fine landing this version:

my $ntsdPath;
for ($ntsdPath = shift @possiblePaths; not -f $ntsdPath; $ntsdPath = shift @possiblePaths) {
    if (…) { …}
}
Comment 7 David Kilzer (:ddkilzer) 2013-05-14 10:48:56 PDT
Comment on attachment 201729 [details]
Patch

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

> Tools/Scripts/old-run-webkit-tests:2834
> +    my @possiblePaths = (File::Spec->catfile(toCygwinPath($ENV{PROGRAMFILES}), "Windows Kits", "8.0", "Debuggers", "x64", "ntsd.exe"),
> +        File::Spec->catfile(toCygwinPath($ENV{PROGRAMFILES}), "Windows Kits", "8.0", "Debuggers", "x86", "ntsd.exe"),
> +        File::Spec->catfile(toCygwinPath($ENV{PROGRAMFILES}), "Debugging Tools for Windows (x86)", "ntsd.exe"),
> +        File::Spec->catfile(toCygwinPath($ENV{ProgramW6432}), "Debugging Tools for Windows (x64)", "ntsd.exe"),
> +        File::Spec->catfile(toCygwinPath($ENV{SYSTEMROOT}), "system32", "ntsd.exe"));

May be slightly easier to read if each File::Spec->catfile() is on its own line:

    my @possiblePaths = (
        File::Spec->catfile(…),
        File::Spec->catfile(…),
        File::Spec->catfile(…),
        File::Spec->catfile(…),
        File::Spec->catfile(…),
    );

But again, it's fine this way as well.
Comment 8 Brent Fulgham 2013-05-14 10:57:36 PDT
Committed r150077: <http://trac.webkit.org/changeset/150077>