Bug 51642 – Add WinCE support to build-webkit

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

[Patrick R. Gansterer]

see patch

[Patrick R. Gansterer]

Created attachment 77500 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 77500 [details]
Patch

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

> Tools/Scripts/build-webkit:522
> +    exit exitStatus($result) if exitStatus($result);

Why does WinCE short-circut the rest of the build-webkit infrastructure?  That seems like a bad idea long-term.

[Eric Seidel (no email)]

build-webkit is kinda a disaster these days.

[Patrick R. Gansterer]

(In reply to comment #2)
> (From update of attachment 77500 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77500&action=review
> 
> > Tools/Scripts/build-webkit:522
> > +    exit exitStatus($result) if exitStatus($result);
> 
> Why does WinCE short-circut the rest of the build-webkit infrastructure?  That seems like a bad idea long-term.
I do the same as EFL port, which uses CMake too.

(In reply to comment #3)
> build-webkit is kinda a disaster these days.
I don't want to use it, but i need it for the buildbot. An alternative is to extend the buildbot config, but I'm not sure if that's a better idea?

[Eric Seidel (no email)]

The correct solution is to use build-webkit.  But we (as a project) need to continue re-factoring and cleaning it up.  Eventually maybe we'll re-write it in a cleaner language. :)

[Patrick R. Gansterer]

(In reply to comment #5)
> The correct solution is to use build-webkit.  But we (as a project) need to continue re-factoring and cleaning it up.  Eventually maybe we'll re-write it in a cleaner language. :)
+1, but any "short term" solution for this? my buildbot machine is waiting and only needs the "build scripts" to start working ;-)

[David Kilzer (:ddkilzer)]

Comment on attachment 77500 [details]
Patch

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

r- to clean up variable usage in buildCMakeProject().

>>> Tools/Scripts/build-webkit:522
>>> +    exit exitStatus($result) if exitStatus($result);
>> 
>> Why does WinCE short-circut the rest of the build-webkit infrastructure?  That seems like a bad idea long-term.
> 
> I do the same as EFL port, which uses CMake too.
> 
> (In reply to comment #3)

This is fine IMO.

> Tools/Scripts/webkitdirs.pm:1454
> +            $makeArgs .= "--config Debug";

I think this should have a space at the beginning of the string (before "--config").

> Tools/Scripts/webkitdirs.pm:1456
> +            $makeArgs .= "--config Release";

This should also have a space at the beginning of the string.

Or are these cmake arguments that must come before the "--" argument?  If so, they should really be in their own variable, not prepended to $makeArgs.

> Tools/Scripts/webkitdirs.pm:1459
> +        $makeArgs .= "-- -j" . numberOfCPUs() if ($makeArgs !~ m/-j\s*\d+/);

Why is the empty "--" used here?  That makes this code path different than the previous code path.

I see, you changed $make to "$cmakebin --build ." below, which means you need to pass arguments to make after "--", right?  I think it would be clearer to add the "--" to the $make command so that $makeArgs doesn't need to add it.

> Tools/Scripts/webkitdirs.pm:1469
> +        my $make = $cmakebin . " --build .";

Add " --" to the end of the $make string here so it doesn't have to be added to $makeArgs in special places.

> Tools/Scripts/webkitdirs.pm:1476
> +                $makeArgs = "-- " . $1;

This change is not needed if " --" is prepended to $make above.

[Patrick R. Gansterer]

Comment on attachment 77500 [details]
Patch

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

I'll create a new patch.

>> Tools/Scripts/webkitdirs.pm:1456
>> +            $makeArgs .= "--config Release";
> 
> This should also have a space at the beginning of the string.
> 
> Or are these cmake arguments that must come before the "--" argument?  If so, they should really be in their own variable, not prepended to $makeArgs.

CMake passes _everything_ after "--" to the make tool, so the --config needs to be before it. A new variable makes sense!

[Patrick R. Gansterer]

Created attachment 77698 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 77698 [details]
Patch

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

r=me, but you need to fix up the space before the "-j", and please consider renaming the $make variable.

> Tools/Scripts/webkitdirs.pm:1465
> +        $makeArgs .= "-j" . numberOfCPUs() if ($makeArgs !~ m/-j\s*\d+/);

Still need a space before the "-j" here:  " -j".

> Tools/Scripts/webkitdirs.pm:1475
> -        my $make = "make";
> +        my $make = $cmakebin . " --build .";

Nit:  Using $make here is now confusing.  I would rename $make to something like $cmakeBuildCommand.

> Tools/Scripts/webkitdirs.pm:1511
> +        $result = system "$make $cmakeBuildArgs";

Renaming $make to $cmakeBuildCommand would make this read better:

        $result = system "$cmakeBuildCommand $cmakeBuildArgs";

[Patrick R. Gansterer]

Created attachment 77834 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 77834 [details]
Patch

r=me  Thanks!

[WebKit Commit Bot]

Comment on attachment 77834 [details]
Patch

Clearing flags on attachment: 77834

Committed r74928: <http://trac.webkit.org/changeset/74928>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.