Bug 77533 – [Qt] JSC build should handle --no-webkit2 option to avoid unwanted clean-builds

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Nandor Huszka]

This bug is made because of https://bugs.webkit.org/show_bug.cgi?id=74519
If we want to build only JavaScriptCore there may be an unwanted clean-build,
because there may be defines that can be found after building WebKit, 
but cannot found when it is built with the --no-webkit2 option.

[Nandor Huszka]

Created attachment 124910 [details]
Modifications in build-jsc

As far as I know, we do not use the --no-webkit2 option on bots, so passing the option to build-jsc by hand is solvable.

[Tor Arne Vestbø]

Comment on attachment 124910 [details]
Modifications in build-jsc

I'd rather we silently pick up BUILD_WEBKIT_ARGS, like in build-webkit, then introduce a  new option to build-jsc that does not make sense for a user passing --help

[Nandor Huszka]

Created attachment 124936 [details]
Modifications in build-jsc

(In reply to comment #2)
This patch picks up the option from BUILD_WEBKIT_ARGS, as you suggested. With this, if I understand it correctly, before want to  build JSC without wk2, we have to put the --no-webkit2 option to the mentioned environment variable.

[Tor Arne Vestbø]

Comment on attachment 124936 [details]
Modifications in build-jsc

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

> Tools/Scripts/build-jsc:79
> +    if ($ENV{'BUILD_WEBKIT_ARGS'}) {

How about an unconditional

push @ARGV, split(/ /, $ENV{'BUILD_WEBKIT_ARGS'}) if ($ENV{'BUILD_WEBKIT_ARGS'});

> Tools/Scripts/build-jsc:85
> +        push @buildWebKitArgs, split(/ /, $ENV{'BUILD_WEBKIT_ARGS'});
> +        foreach (@buildWebKitArgs) {
> +            if ($_ eq '--no-webkit2') {
> +                push @ARGV, "--qmakearg=CONFIG+=no_webkit2";
> +            }

And then a

push @ARGV, "--qmakearg=CONFIG+=no_webkit2" if checkForArgumentAndRemoveFromARGV("--no-webkit2");

[Csaba Osztrogonác]

Or what if making isWK2() handle --no-webkit2, not only -2? And then we can use it instead of foreach or checkForArgumentAndRemoveFromARGV. It would good for build-webkit script too.

[Tor Arne Vestbø]

(In reply to comment #5)
> Or what if making isWK2() handle --no-webkit2, not only -2? And then we can use it instead of foreach or checkForArgumentAndRemoveFromARGV. It would good for build-webkit script too.

That would be an option. Though it would change the default from 0 to 1.

[Nandor Huszka]

Created attachment 125095 [details]
Patch

(In reply to comment #5)
I tried it, but it is impossible to indicate 3 cases with one bool variable.
I mean if we put this into isWK2():

if (checkForArgumentAndRemoveFromARGV("-2")) {
  $isWK2 = 0;
} elsif (checkForArgumentAndRemoveFromARGV("--no-webkit2")){
  $isWK2 = 1;
} else {
  $isWK2 = 0;
}

or other combination of ifs, it would be incorrect, I think.
I tried to write an isNotWK2() function, but if in it we use in it the $isWK2 too, its value depends on whether isWK2() or isNotWK2() function was called sooner. But if we use a new, e.g. $isNotWK2 variable in it, it would be incomprehensible why we use $isWK2 and $isNotWK2 too.

(In reply to comment #4)
Ok, I have done that, but I do not understand why can we pass on all the other unchecked options in BUILD_WEBKIT_ARGS to the buildQMakeProjects.

[Eric Seidel (no email)]

Comment on attachment 125095 [details]
Patch

Do we normally do this transition in the main script like this?   don't we have helper functions in one of our perl libraries for this?

[Nandor Huszka]

(In reply to comment #8)
> (From update of attachment 125095 [details])
> Do we normally do this transition in the main script like this?   don't we have helper functions in one of our perl libraries for this?

build-webkit does similar thing in line ~578:
    push @options, "--qmakearg=CONFIG+=no_webkit2" if $noWebKit2;

but it does not use helper function for it. I did not find any other method which can be used here.

[Nandor Huszka]

(In reply to comment #9)
Is there any reaction related to my patch, is it OK? I couldn't find a simpler way to solve it.

[Tor Arne Vestbø]

Comment on attachment 125095 [details]
Patch

I think there's other ways to solve this, but those would be more intrusive and require more time, so this is good for now.

[WebKit Review Bot]

Comment on attachment 125095 [details]
Patch

Rejecting attachment 125095 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
', u'--force', u'--reviewer', u'Tor Arne V..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Parsed 2 diffs from patch file(s).
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/Scripts/build-jsc
Hunk #1 FAILED at 74.
1 out of 1 hunk FAILED -- saving rejects to file Tools/Scripts/build-jsc.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Tor Arne V..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12288197

[Nandor Huszka]

Created attachment 135032 [details]
Patch

The build-jsc is modified, in my opinion that is why the review bot could not run. I redo the modifications with the latest revision.

[Csaba Osztrogonác]

ping review?

[WebKit Review Bot]

Comment on attachment 135032 [details]
Patch

Clearing flags on attachment: 135032

Committed r114387: <http://trac.webkit.org/changeset/114387>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.