Bug 149904 - [Win] Support 64-bit Build and Testing
Summary: [Win] Support 64-bit Build and Testing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-10-07 16:33 PDT by Brent Fulgham
Modified: 2015-10-07 17:36 PDT (History)
4 users (show)

See Also:


Attachments
Patch (12.86 KB, patch)
2015-10-07 16:39 PDT, Brent Fulgham
dbates: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2015-10-07 16:33:04 PDT
Several of our scripts don't do the right thing if you are attempting to build 64-bit Windows code. This patch corrects that problem.
Comment 1 Brent Fulgham 2015-10-07 16:39:55 PDT
Created attachment 262655 [details]
Patch
Comment 2 Daniel Bates 2015-10-07 17:10:32 PDT
Comment on attachment 262655 [details]
Patch

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

> Tools/ChangeLog:29
> +        (WinPort._build_path): Added properly binary target path to _build_path.

This sentence does not read well.

> Tools/ChangeLog:33
> +        ntsd debugger does not understand this argument.

Nit: ntsd => NTSD

(NTSD is an acronym for (Microsoft) NT Symbolic Debugger)

> Tools/Scripts/build-dumprendertree:64
> +chdir File::Spec->catdir('Tools', 'DumpRenderTree') or die;

We use double quoted string literals in Perl code unless the literal contains a double quote or we explicitly do not want string interpolation to be performed.

> Tools/Scripts/webkitdirs.pm:622
> +    my $binDir;

This is OK as-is. I don't see the need to abbreviate the word "directory" in the variable name "binDir". I know we use this abbreviation throughout the code, including in the name "productDir". Having said that, I don't feel we should continue such a naming convention.

> Tools/Scripts/webkitpy/port/factory.py:67
> +        optparse.make_option('--64-bit', action='store_const', const='x86_64', default=None, dest="architecture",
> +            help='use 64-bit binaries by default (x86_64 instead of x86)'),

This is OK as-is. I know you are just mimicking the existing code style. It's weird that we alternate between using single quoted and double quoted literals.

> Tools/Scripts/webkitpy/port/win.py:131
> +            binPath = 'bin32'

Nit: In Python code variable names should use the words separated by underscores naming convention. The name of this variable is also inconsistent with the name of the analogous variable used in the webkitdirs::executableProductDir(), binDir. I suggest that we name this variable bin_directory to be consistent. Regardless, we should pick a name for the variable that represents the name of the bin directory and use in both the Perl and the Python code up to naming convention style for consistency.

> Tools/Scripts/webkitpy/port/win.py:176
> +                self._filesystem.join(os.environ['SYSTEMROOT'], "system32", "ntsd.exe")]

This is OK as-is. This path is identical in both the 64-bit and 32-bit cases. Instead of duplicating this path for both branches we could append it to possible_paths outside of this if-else block.
Comment 3 Brent Fulgham 2015-10-07 17:34:02 PDT
Comment on attachment 262655 [details]
Patch

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

>> Tools/ChangeLog:29
>> +        (WinPort._build_path): Added properly binary target path to _build_path.
> 
> This sentence does not read well.

You're right! I'll fix that.

>> Tools/ChangeLog:33
>> +        ntsd debugger does not understand this argument.
> 
> Nit: ntsd => NTSD
> 
> (NTSD is an acronym for (Microsoft) NT Symbolic Debugger)

Ok -- I'll correct that.

>> Tools/Scripts/build-dumprendertree:64
>> +chdir File::Spec->catdir('Tools', 'DumpRenderTree') or die;
> 
> We use double quoted string literals in Perl code unless the literal contains a double quote or we explicitly do not want string interpolation to be performed.

OK. Will fix.

>> Tools/Scripts/webkitdirs.pm:622
>> +    my $binDir;
> 
> This is OK as-is. I don't see the need to abbreviate the word "directory" in the variable name "binDir". I know we use this abbreviation throughout the code, including in the name "productDir". Having said that, I don't feel we should continue such a naming convention.

I'll change these to 'directory', since these are all local values, and won't cause much of a renaming-ripple-effect.

>> Tools/Scripts/webkitpy/port/win.py:131
>> +            binPath = 'bin32'
> 
> Nit: In Python code variable names should use the words separated by underscores naming convention. The name of this variable is also inconsistent with the name of the analogous variable used in the webkitdirs::executableProductDir(), binDir. I suggest that we name this variable bin_directory to be consistent. Regardless, we should pick a name for the variable that represents the name of the bin directory and use in both the Perl and the Python code up to naming convention style for consistency.

OK. I actually changed it to 'binary_directory', since I used 'binaryDirectory' in the Perl change.
Comment 4 Brent Fulgham 2015-10-07 17:36:25 PDT
Committed r190698: <http://trac.webkit.org/changeset/190698>