Bug 160340

Summary: Allow more --cmakeargs option in build-jsc and build-webkit
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: New BugsAssignee: Csaba Osztrogonác <ossy>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ddkilzer, ossy
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Csaba Osztrogonác 2016-07-29 06:52:05 PDT
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.
Comment 1 Csaba Osztrogonác 2016-07-29 06:54:02 PDT
Created attachment 284864 [details]
Patch
Comment 2 David Kilzer (:ddkilzer) 2016-07-29 15:17:32 PDT
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.
Comment 3 Csaba Osztrogonác 2016-08-01 04:10:08 PDT
(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.
Comment 4 Csaba Osztrogonác 2016-08-01 04:20:30 PDT
Created attachment 285005 [details]
Patch
Comment 5 Csaba Osztrogonác 2016-08-04 02:29:12 PDT
ping?
Comment 6 WebKit Commit Bot 2016-08-09 02:12:32 PDT
Comment on attachment 285005 [details]
Patch

Clearing flags on attachment: 285005

Committed r204278: <http://trac.webkit.org/changeset/204278>
Comment 7 WebKit Commit Bot 2016-08-09 02:12:36 PDT
All reviewed patches have been landed.  Closing bug.