Bug 55438 – Cleanup: Separate port-specific implementation details from webkitdirs::buildCMakeProject()

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Daniel Bates]

webkitdirs::buildCMakeProject() should be a port-independent function for building a port that uses a CMake build system. Currently, this function also includes logic that is specific to the WinCE and EFL ports. We should extract these details so as to make this function port-independent.

[Daniel Bates]

Created attachment 84177 [details]
Patch

I am not very familiar with CMake or the EFL and WinCE ports. So, I would appreciate if this patch is scrutinized for correctness. 

I implemented the functionality for cleaning a CMake project (i.e. build-webkit --clean) using cmake --build ... --target clean. Currently, the code "cleans" a project by removing the build directory (why?).

[Daniel Bates]

Created attachment 84178 [details]
Patch

[WebKit Review Bot]

Attachment 84177 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8070579

[WebKit Review Bot]

Attachment 84178 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8075473

[Daniel Bates]

Created attachment 84180 [details]
Patch

[Gyuyoung Kim]

There is build break on EFL port. Please do not land this patch by fixing this error.


Failed to run "['Tools/Scripts/build-webkit', '--release', '--efl', '--makeargs="-j8"']" exit_code: 1

perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
	LANGUAGE = (unset),
	LC_ALL = (unset),
	LANG = "en_US.US-ASCII"
    are supported and installed on your system.
perl: warning: Falling back to the standard locale ("C").
CMake Error at CMakeLists.txt:97 (INCLUDE):
  include could not find load file:

    OptionsEFL


-- Platform-specific CMakeLists not found: /mnt/eflews/git/webkit/Source/JavaScriptCore/CMakeListsEFL.txt
-- Platform-specific CMakeLists not found: /mnt/eflews/git/webkit/Source/JavaScriptCore/wtf/CMakeListsEFL.txt
-- Platform-specific CMakeLists not found: /mnt/eflews/git/webkit/Source/JavaScriptCore/shell/CMakeListsEFL.txt
-- Platform-specific CMakeLists not found: /mnt/eflews/git/webkit/Source/WebCore/CMakeListsEFL.txt
-- Platform-specific CMakeLists not found: /mnt/eflews/git/webkit/Source/WebKit/efl/CMakeListsEFL.txt
You have called ADD_LIBRARY for library WebKit without any source files. This typically indicates a problem with your CMakeLists.txt file
-- Platform-specific CMakeLists not found: /mnt/eflews/git/webkit/Source/../Tools/CMakeListsEFL.txt
-- Enabled features:
-- Configuring incomplete, errors occurred

[Daniel Bates]

Comment on attachment 84180 [details]
Patch

Clearing review flag while I look into this some more.

[Daniel Bates]

Created attachment 84248 [details]
Patch

[Daniel Bates]

Created attachment 84287 [details]
Patch

[Patrick R. Gansterer]

Comment on attachment 84287 [details]
Patch

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

Sorry for the long delay. LGTM, but one change is required to work (at least on my machine).

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

I don't know why, but it only works when chanig the quotes:
buildCMakeProjectOrExit($clean, "WinCE", $prefixPath, $makeArgs, ("-DCMAKE_WINCE_SDK='STANDARDSDK_500 (ARMV4I)'", cMakeArgsFromFeatures()));

[Daniel Bates]

(In reply to comment #10)
> (From update of attachment 84287 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84287&action=review
> 
> [...]
> I don't know why, but it only works when chanig the quotes:
> buildCMakeProjectOrExit($clean, "WinCE", $prefixPath, $makeArgs, ("-DCMAKE_WINCE_SDK='STANDARDSDK_500 (ARMV4I)'", cMakeArgsFromFeatures()));

Notice, buildCMakeProjectOrExit() passes the argument @cmakeArgs to generateBuildSystemFromCMakeProject().

We don't need the inner quotes around the value of DCMAKE_WINCE_SDK since generateBuildSystemFromCMakeProject() calls system() using the Program, List form of this subroutine. This form of the subroutine passes the argument list directly to Program (instead of calling the system shell first) as per <http://perldoc.perl.org/functions/system.html>. In particular, the CMake argument "DCMAKE_WINCE_SDK='STANDARDSDK_500 (ARMV4I)'" will be passed directly. Hence, we don't need the quotes around the value of DCMAKE_WINCE_SDK.

[Daniel Bates]

Created attachment 85228 [details]
Patch

Simplified logic for $makeArgs by initializing it to the empty string.

[Daniel Bates]

Created attachment 85230 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 85230 [details]
Patch

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").

[Daniel Bates]

(In reply to comment #14)
> (From update of attachment 85230 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85230&action=review
> [...]
> > 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.

Will remove.

> 
> > 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):

I'll add quotes around this value and change the call to system() (in generateBuildSystemFromCMakeProject()) so that it passes through the shell for processing (similar to what is done in buildCMakeGeneratedProject()). 

Currently, quotes are unnecessary since I'm using the PROGRAM-LIST form of system() with a LIST that consists of more than one argument in generateBuildSystemFromCMakeProject(): <http://perldoc.perl.org/functions/system.html>.

> 
>     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.

Will add.

> 
> > 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\"";
> 

Will change.

Currently, the form of system() used obviates the need for quotes as explained above.

> 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?

Will change this to read:

if ($config =~ /release/i) {
    push @args, "-DCMAKE_BUILD_TYPE=Release";
} elsif ($config =~ /debug/i) {
    push @args, "-DCMAKE_BUILD_TYPE=Debug";
}

> 
> > 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").

Will change.

[Daniel Bates]

(In reply to comment #14)
> (From update of attachment 85230 [details])
> 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.

Patrick Gansterer said the patch "LGTM" with a remark to use quotes (which I have corrected) in comment 10. Also, the patch builds on EFL according the EFL EWS bot. I don't foresee issues with the changes you suggested. I will land this patch shortly and watch the build bots.

[Daniel Bates]

Committed r81041: <http://trac.webkit.org/changeset/81041>

[David Kilzer (:ddkilzer)]

(In reply to comment #17)
> Committed r81041: <http://trac.webkit.org/changeset/81041>

Follow-up fix in r81042: <http://trac.webkit.org/changeset/81042>