Allow more --cmakeargs option in build-jsc and build-webkit

• Component: New Bugs
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

We can use --cmakeargs="-DFOO=bar -DCMAKE_PREFIX_PATH=/usr/local"
as the example says in the help. It works as expected. 

But it doesn't work if we use BUILD_WEBKIT_ARGS or BUILD_JSC_ARGS 
environment variable to set --cmakeargs option, because they
are splitted because of the whitespace.

Additionally if we set --cmakeargs in BUILD_WEBKIT_ARGS and
pass an other --cmakeargs to the command line, the cmdline
one is omitted, because Perl's GetOptions uses only the last option.

I think the proper fix would be to allow more --cmakeargs and simply concatenate them.

[Csaba Osztrogonác]

Created attachment 284864 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 284864 [details]
Patch

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

> Tools/Scripts/build-jsc:67
> -  --cmakeargs=<arguments>       Optional CMake flags (e.g. --cmakeargs="-DFOO=bar -DCMAKE_PREFIX_PATH=/usr/local")
> +  --cmakeargs=<arguments>       Optional CMake flags (e.g. --cmakeargs="-DFOO=bar -DCMAKE_PREFIX_PATH=/usr/local") More --cmakeargs allowed.

The "More --cmakeargs allowed." at the end will be hard to read/notice in the shell.  How about this?

--cmakeargs=<arguments> One or more optional CMake flags (e.g. --cmakeargs="-DFOO=bar -DCMAKE_PREFIX_PATH=/usr/local")

> Tools/Scripts/build-jsc:79
> -    'cmakeargs=s' => \$cmakeArgs
> +    'cmakeargs=s' => sub { $cmakeArgs .= " $_[1]" }

Can this also be accomplished by using an array for cmakeArgs?

'cmakeargs=s' => \@cmakeArgs,

Or will that destroy the whitespace? 

This is apparently the preferred way to do this via the "Options with multiple values" section in "man Getopt::Long".

> Tools/Scripts/build-webkit:104
> -  --cmakeargs=<arguments>           Optional CMake flags (e.g. --cmakeargs="-DFOO=bar -DCMAKE_PREFIX_PATH=/usr/local")
> +  --cmakeargs=<arguments>           Optional CMake flags (e.g. --cmakeargs="-DFOO=bar -DCMAKE_PREFIX_PATH=/usr/local") More --cmakeargs allowed.

Ditto.

> Tools/Scripts/build-webkit:119
> -    'cmakeargs=s' => \$cmakeArgs,
> +    'cmakeargs=s' => sub { $cmakeArgs .= " $_[1]" },

Ditto.

[Csaba Osztrogonác]

(In reply to comment #2)
> Comment on attachment 284864 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284864&action=review
> 
> > Tools/Scripts/build-jsc:67
> > -  --cmakeargs=<arguments>       Optional CMake flags (e.g. --cmakeargs="-DFOO=bar -DCMAKE_PREFIX_PATH=/usr/local")
> > +  --cmakeargs=<arguments>       Optional CMake flags (e.g. --cmakeargs="-DFOO=bar -DCMAKE_PREFIX_PATH=/usr/local") More --cmakeargs allowed.
> 
> The "More --cmakeargs allowed." at the end will be hard to read/notice in
> the shell.  How about this?
> 
> --cmakeargs=<arguments> One or more optional CMake flags (e.g.
> --cmakeargs="-DFOO=bar -DCMAKE_PREFIX_PATH=/usr/local")

OK, will use the one you suggested.
 
> > Tools/Scripts/build-jsc:79
> > -    'cmakeargs=s' => \$cmakeArgs
> > +    'cmakeargs=s' => sub { $cmakeArgs .= " $_[1]" }
> 
> Can this also be accomplished by using an array for cmakeArgs?
> 
> 'cmakeargs=s' => \@cmakeArgs,
> 
> Or will that destroy the whitespace? 
> 
> This is apparently the preferred way to do this via the "Options with
> multiple values" section in "man Getopt::Long".

Good point, I wasn't thorough enough to find this super easy way in the man.
Actually it isn't necessary to use whitespace separated string, let's
use array everywhere for cmake arguments.

[Csaba Osztrogonác]

Created attachment 285005 [details]
Patch

[Csaba Osztrogonác]

ping?

[WebKit Commit Bot]

Comment on attachment 285005 [details]
Patch

Clearing flags on attachment: 285005

Committed r204278: <http://trac.webkit.org/changeset/204278>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.