Bug 77533 - [Qt] JSC build should handle --no-webkit2 option to avoid unwanted clean-builds
Summary: [Qt] JSC build should handle --no-webkit2 option to avoid unwanted clean-builds
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2012-02-01 01:53 PST by Nandor Huszka
Modified: 2012-04-17 09:27 PDT (History)
5 users (show)

See Also:


Attachments
Modifications in build-jsc (1.68 KB, patch)
2012-02-01 01:58 PST, Nandor Huszka
vestbo: review-
vestbo: commit-queue-
Details | Formatted Diff | Diff
Modifications in build-jsc (1.64 KB, patch)
2012-02-01 05:38 PST, Nandor Huszka
vestbo: review-
vestbo: commit-queue-
Details | Formatted Diff | Diff
Patch (1.47 KB, patch)
2012-02-02 01:02 PST, Nandor Huszka
vestbo: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (1.39 KB, patch)
2012-04-02 00:19 PDT, Nandor Huszka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nandor Huszka 2012-02-01 01:53:47 PST
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.
Comment 1 Nandor Huszka 2012-02-01 01:58:56 PST
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.
Comment 2 Tor Arne Vestbø 2012-02-01 02:39:41 PST
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
Comment 3 Nandor Huszka 2012-02-01 05:38:30 PST
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.
Comment 4 Tor Arne Vestbø 2012-02-01 06:14:41 PST
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");
Comment 5 Csaba Osztrogonác 2012-02-01 06:43:43 PST
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.
Comment 6 Tor Arne Vestbø 2012-02-01 07:34:53 PST
(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.
Comment 7 Nandor Huszka 2012-02-02 01:02:29 PST
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.
Comment 8 Eric Seidel (no email) 2012-02-10 10:29:03 PST
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?
Comment 9 Nandor Huszka 2012-02-16 02:46:25 PST
(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.
Comment 10 Nandor Huszka 2012-03-30 03:43:05 PDT
(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.
Comment 11 Tor Arne Vestbø 2012-03-30 05:03:31 PDT
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.
Comment 12 WebKit Review Bot 2012-03-30 05:07:03 PDT
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
Comment 13 Nandor Huszka 2012-04-02 00:19:19 PDT
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.
Comment 14 Csaba Osztrogonác 2012-04-17 05:46:43 PDT
ping review?
Comment 15 WebKit Review Bot 2012-04-17 09:27:03 PDT
Comment on attachment 135032 [details]
Patch

Clearing flags on attachment: 135032

Committed r114387: <http://trac.webkit.org/changeset/114387>
Comment 16 WebKit Review Bot 2012-04-17 09:27:09 PDT
All reviewed patches have been landed.  Closing bug.