Bug 74308 – build-jsc passing incorrect args to buildGtkProject

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Andy Wingo]

Patch forthcoming.

[Andy Wingo]

Created attachment 118812 [details]
Patch

[Martin Robinson]

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?

[Martin Robinson]

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?

[Andy Wingo]

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

[Martin Robinson]

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

[Andy Wingo]

Created attachment 119019 [details]
Patch

[Andy Wingo]

Does the newly attached patch work for you?

[Martin Robinson]

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.

[Andy Wingo]

Created attachment 119022 [details]
Patch

[Martin Robinson]

Comment on attachment 119022 [details]
Patch

Thank you!

[Martin Robinson]

Created attachment 119030 [details]
Patch which also prevents reconfiguration

[Gustavo Noronha (kov)]

Comment on attachment 119030 [details]
Patch which also prevents reconfiguration

ok, cleaner is gooder =)

[Martin Robinson]

Committed r102685: <http://trac.webkit.org/changeset/102685>