[webkit-reviews] review granted: [Bug 55438] Cleanup: Separate port-specific implementation details from webkitdirs::buildCMakeProject() : [Attachment 85230] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has granted Daniel Bates
<dbates at webkit.org>'s request for review:
Bug 55438: Cleanup: Separate port-specific implementation details from
webkitdirs::buildCMakeProject()
https://bugs.webkit.org/show_bug.cgi?id=55438

Attachment 85230: Patch
https://bugs.webkit.org/attachment.cgi?id=85230&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=85230&action=review

r=me assuming the other CMake port maintainers are okay with this.

> Tools/Scripts/build-webkit:60
> +my $makeArgs = ""; # We initialize this to the empty string to simplify its
use in string operations.

Nit: Comment probably isn't necessary unless someone removes the initialization
in the future.

> Tools/Scripts/build-webkit:537
> +    buildCMakeProjectOrExit($clean, "WinCE", $prefixPath, $makeArgs,
("-DCMAKE_WINCE_SDK=STANDARDSDK_500 (ARMV4I)", cMakeArgsFromFeatures()));

I don't think the -DCMAKE_WINCE_SDK=STANDARDSDK_500 (ARMV4I) argument be quoted
correctly.  This should be (unless calling system() simply does the right thing
without quoting):

    buildCMakeProjectOrExit($clean, "WinCE", $prefixPath, $makeArgs,
("-DCMAKE_WINCE_SDK=\"STANDARDSDK_500 (ARMV4I)\"", cMakeArgsFromFeatures()));

> Tools/Scripts/webkitdirs.pm:1487
> +    chdir($buildPath);

Nit: Might consider adding back the "or die..." code to the chdir here.

> Tools/Scripts/webkitdirs.pm:1492
> +    push @args, "-DPORT=$port";
> +    push @args, "-DCMAKE_INSTALL_PREFIX=$prefixPath" if $prefixPath;
> +    push @args, "-DCMAKE_BUILD_TYPE=$config";

Nit: To be extra-safe and future-proof, you could double-quote the values.  At
least consider doing this for the $prefixPath value since paths can have spaces
in them (unless the system() call obviates the need for this):

    push @args, "-DPORT=\"$port\"";
    push @args, "-DCMAKE_INSTALL_PREFIX=\"$prefixPath\"" if $prefixPath;
    push @args, "-DCMAKE_BUILD_TYPE=\"$config\"";

Nit: The old code didn't assume that $config was always "Debug" or "Release". 
What happens if someone uses "release" instead?  Will cmake still do the
correct thing?

> Tools/Scripts/webkitdirs.pm:1494
> +    push @args, sourceDir() . "/Source";

Nit: I know the original code didn't do this, but this should probably use
File::Spec->catdir(sourceDir(), "Source").