Bug 74308 - build-jsc passing incorrect args to buildGtkProject
Summary: build-jsc passing incorrect args to buildGtkProject
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Martin Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-12 09:54 PST by Andy Wingo
Modified: 2011-12-13 10:10 PST (History)
1 user (show)

See Also:


Attachments
Patch (1.40 KB, patch)
2011-12-12 09:56 PST, Andy Wingo
no flags Details | Formatted Diff | Diff
Patch (3.00 KB, patch)
2011-12-13 08:02 PST, Andy Wingo
no flags Details | Formatted Diff | Diff
Patch (2.99 KB, patch)
2011-12-13 08:35 PST, Andy Wingo
no flags Details | Formatted Diff | Diff
Patch which also prevents reconfiguration (6.50 KB, patch)
2011-12-13 09:57 PST, Martin Robinson
gns: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Wingo 2011-12-12 09:54:21 PST
Patch forthcoming.
Comment 1 Andy Wingo 2011-12-12 09:56:55 PST
Created attachment 118812 [details]
Patch
Comment 2 Martin Robinson 2011-12-12 10:19:08 PST
Comment on attachment 118812 [details]
Patch

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

> Tools/Scripts/build-jsc:74
> +    my $allowWebKit2 = 1;
> +    $result = buildGtkProject("JavaScriptCore", 0, 1, @ARGV);

Do you mean to pass $allowWebKit2 here instead of 1?
Comment 3 Martin Robinson 2011-12-12 10:21:55 PST
I agree that this argument is a bit pointless for other projects.  I think it would be smarter to simply add --enable-webkit2=no to @options in build-webkit instead of keeping the extra argument around. What do you think?
Comment 4 Andy Wingo 2011-12-12 10:27:37 PST
> > +    my $allowWebKit2 = 1;
> > +    $result = buildGtkProject("JavaScriptCore", 0, 1, @ARGV);
> 
> Do you mean to pass $allowWebKit2 here instead of 1?

Heh, indeed.  Will fix eventually.

(In reply to comment #3)
> I agree that this argument is a bit pointless for other projects.  I think it would be smarter to simply add --enable-webkit2=no to @options in build-webkit instead of keeping the extra argument around. What do you think?

The default in build-webkit is to enable it, so that should be the default in jsc, so as to avoid needless autofoo churn.  Therefore another way of defaulting it would be needed.  I punted.
Comment 5 Martin Robinson 2011-12-12 10:31:43 PST
> > I agree that this argument is a bit pointless for other projects.  I think it would be smarter to simply add --enable-webkit2=no to @options in build-webkit instead of keeping the extra argument around. What do you think?
> 
> The default in build-webkit is to enable it, so that should be the default in jsc, so as to avoid needless autofoo churn.  Therefore another way of defaulting it would be needed.  I punted.

Sorry, my proposal wasn't very clear:

1. Remove the WebKit2 argument completely.
2. If --no-webkit2 was passed to build-webkit, just add --enable-webkit2=no to @options in build-webkit.

This would allow us to have no code at all dealing with it in build-jsc and to avoid leaking details of this option down the call stack.
Comment 6 Andy Wingo 2011-12-13 08:02:52 PST
Created attachment 119019 [details]
Patch
Comment 7 Andy Wingo 2011-12-13 08:09:43 PST
Does the newly attached patch work for you?
Comment 8 Martin Robinson 2011-12-13 08:34:07 PST
Comment on attachment 119019 [details]
Patch

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

This seems a lot cleaner, but see my comment below...

> Tools/Scripts/build-webkit:754
> +        if ($noWebKit2) {
> +            unshift(@options, "--disable-webkit2");
> +        }

configure.ac has WebKit2 disabled by default. I guess this logic should be reversed. Sorry, I think I led you astray here.
Comment 9 Andy Wingo 2011-12-13 08:35:12 PST
Created attachment 119022 [details]
Patch
Comment 10 Martin Robinson 2011-12-13 08:54:10 PST
Comment on attachment 119022 [details]
Patch

Thank you!
Comment 11 Martin Robinson 2011-12-13 09:57:48 PST
Created attachment 119030 [details]
Patch which also prevents reconfiguration
Comment 12 Gustavo Noronha (kov) 2011-12-13 10:05:11 PST
Comment on attachment 119030 [details]
Patch which also prevents reconfiguration

ok, cleaner is gooder =)
Comment 13 Martin Robinson 2011-12-13 10:10:56 PST
Committed r102685: <http://trac.webkit.org/changeset/102685>