Bug 124537 – Build script should fail when any invalid option is passed to it

• Component: Tools / Tests
• Status: NEW

[Nick Diego Yamane (diegoyam)]

Build script should fail when any invalid option is passed to it

[Nick Diego Yamane (diegoyam)]

Created attachment 217231 [details]
Patch

[Tim Horton]

Comment on attachment 217231 [details]
Patch

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

> Tools/ChangeLog:7
> +

Please write a few words here about the exception (perhaps referencing the revision it was added, and the ARCHS thing I mentioned -- or maybe just generic words).

[Nick Diego Yamane (diegoyam)]

(In reply to comment #2)
> (From update of attachment 217231 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217231&action=review
> 
> > Tools/ChangeLog:7
> > +
> 
> Please write a few words here about the exception (perhaps referencing the revision it was added, and the ARCHS thing I mentioned -- or maybe just generic words).

Sorry, it was on my git commit message, just forgot to add it in the changelog :)

[Nick Diego Yamane (diegoyam)]

Created attachment 217233 [details]
Patch

[Tim Horton]

Comment on attachment 217233 [details]
Patch

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

> Tools/ChangeLog:10
> +        It pass through command-line options only for xcode builds as it

s/xcode/Xcode/

> Tools/ChangeLog:12
> +        E.g: Tools/Scripts/build-webkit ARCHS="x86 ppc"

It's actually i386, I think-o'd before. This is an antique example anyway, so it doesn't really matter.

> Tools/ChangeLog:15
> +

I confirmed that this still works fine with Xcode.

[EFL EWS Bot]

Comment on attachment 217233 [details]
Patch

Attachment 217233 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/28398006

[Tim Horton]

Comment on attachment 217233 [details]
Patch

Or maybe not?

[Nick Diego Yamane (diegoyam)]

Created attachment 217317 [details]
Patch

[Daniel Bates]

Comment on attachment 217317 [details]
Patch

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

> Tools/ChangeLog:11
> +        NOTICE: 'build-webkit' pass through command-line options only
> +        for xcode builds as it has its customized options/features mechanism.

This sentence doesn't read well. Maybe:

Note, build-webkit will not error out when it encounters an unrecognized command line option when building the Mac or iOS port. For these ports, unrecognized options will be passed through so that Xcode can process them.

> Tools/ChangeLog:15
> +        options which has to be added to the 'options' hashtable so that

Nit: hashtable => hash table

> Tools/ChangeLog:17
> +        GetOpt::Long::GetOptions will be aware of them. That's is by the
> +        new function 'initializePrivateOptions' added into webkitdirs module.

This sentence doesn't read well. Moreover, I suggest you make this sentence a function-specific remark for initializePrivateOptions() (i.e. move the sentence to line 32 in the change log entry), maybe:

(initializePrivateOptions): Added; used internally to expose command line options for non-{iOS, Mac} ports.

> Tools/ChangeLog:25
> +        (determineIsWK2):
> +        (isWK2):
> +        (determineShouldUpdateEfl):
> +        (shouldUpdateEfl):
> +        (determineShouldUpdateGtk):
> +        (shouldUpdateGtk):
> +        (shouldUseGuardMalloc):
> +        (determineShouldUseGuardMalloc):

Please add the remark "Added" to these change log lines just as you did for initializePrivateOptions() (below).

This will make it straightforward to determine when a function was added by searching for the functions name in Trac or SVN/Git history.

> Tools/ChangeLog:29
> +        In adition, this patch standardizes the way command-line options are

Nit: adition => addition

> Tools/ChangeLog:32
> +        (initilizePrivateOptions): Added.

Nit: initilizePrivateOptions => initializePrivateOptions

That is, the word "initialize" is misspelled in the name of this function.

> Tools/Scripts/build-webkit:133
> +    initilizePrivateOptions(\%options);

Nit: initilizePrivateOptions => initializePrivateOptions

That is, the word "initialize" is misspelled in the name of this function.

> Tools/Scripts/webkitdirs.pm:105
>  my $shouldUseXPCServiceForWebProcess;
>  my $shouldUseGuardMalloc;
>  my $xcodeVersion;
> +my $shouldUpdateEfl;
> +my $shouldUpdateGtk;

We should sort this list of declarations and insert these variable declarations in sorted order.

> Tools/Scripts/webkitdirs.pm:899
> +sub determineIsWK2

Please add "()" to the end of this line to force Perl to error when this function is called with an argument.

> Tools/Scripts/webkitdirs.pm:1054
> +sub determineShouldUpdateEfl

Ditto.

> Tools/Scripts/webkitdirs.pm:1060
> +sub shouldUpdateEfl

Ditto.

> Tools/Scripts/webkitdirs.pm:1392
> +sub shouldUseGuardMalloc

Please add "()" to the end of this line to force Perl to error when this function is called with an argument.

> Tools/Scripts/webkitdirs.pm:1398
> +sub determineShouldUseGuardMalloc

Ditto.

> Tools/Scripts/webkitdirs.pm:2360
> +sub initilizePrivateOptions($)

Nit: initilizePrivateOptions => initializePrivateOptions

That is, the word "initialize" is misspelled in the name of this function.

> Tools/Scripts/webkitdirs.pm:2371
> +        'efl' => \$isEfl,
> +        'gtk' => \$isGtk,
> +        'blackberry' => \$isBlackBerry,
> +        'wince' => \$isWinCE,
> +        'wincairo' => \$isWinCairo,
> +        '64-bit' => \$isWin64,
> +        'update-efl' => \$shouldUpdateEfl,
> +        'update-gtk' => \$shouldUpdateGtk

Nit: ' => "

We use double quotes for string literals unless we explicitly don't want string interpolation.

> Tools/Scripts/webkitdirs.pm:2376
> +    determineArchitecture();
> +    determinePassedConfiguration();
> +    determineIsWK2();

Is this necessary to call these functions? I mean, we'll ultimately call these functions when a caller queries for the such information (e.g. when architecture() is called).

[Nick Diego Yamane (diegoyam)]

Thanks for the review.

(In reply to comment #9)
> (From update of attachment 217317 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217317&action=review
> 
> > Tools/ChangeLog:11
> > +        NOTICE: 'build-webkit' pass through command-line options only
> > +        for xcode builds as it has its customized options/features mechanism.
> 
> This sentence doesn't read well. Maybe:
> 
> Note, build-webkit will not error out when it encounters an unrecognized command line option when building the Mac or iOS port. For these ports, unrecognized options will be passed through so that Xcode can process them.
> 
> > Tools/ChangeLog:15
> > +        options which has to be added to the 'options' hashtable so that
> 
> Nit: hashtable => hash table
> 
> > Tools/ChangeLog:17
> > +        GetOpt::Long::GetOptions will be aware of them. That's is by the
> > +        new function 'initializePrivateOptions' added into webkitdirs module.
> 
> This sentence doesn't read well. Moreover, I suggest you make this sentence a function-specific remark for initializePrivateOptions() (i.e. move the sentence to line 32 in the change log entry), maybe:
> 
> (initializePrivateOptions): Added; used internally to expose command line options for non-{iOS, Mac} ports.
> 
> > Tools/ChangeLog:25
> > +        (determineIsWK2):
> > +        (isWK2):
> > +        (determineShouldUpdateEfl):
> > +        (shouldUpdateEfl):
> > +        (determineShouldUpdateGtk):
> > +        (shouldUpdateGtk):
> > +        (shouldUseGuardMalloc):
> > +        (determineShouldUseGuardMalloc):
> 
> Please add the remark "Added" to these change log lines just as you did for initializePrivateOptions() (below).
> 
> This will make it straightforward to determine when a function was added by searching for the functions name in Trac or SVN/Git history.
> 
> > Tools/ChangeLog:29
> > +        In adition, this patch standardizes the way command-line options are
> 
> Nit: adition => addition
> 
> > Tools/ChangeLog:32
> > +        (initilizePrivateOptions): Added.
> 
> Nit: initilizePrivateOptions => initializePrivateOptions
> 
> That is, the word "initialize" is misspelled in the name of this function.
> 
> > Tools/Scripts/build-webkit:133
> > +    initilizePrivateOptions(\%options);
> 
> Nit: initilizePrivateOptions => initializePrivateOptions
> 
> That is, the word "initialize" is misspelled in the name of this function.
> 
> > Tools/Scripts/webkitdirs.pm:105
> >  my $shouldUseXPCServiceForWebProcess;
> >  my $shouldUseGuardMalloc;
> >  my $xcodeVersion;
> > +my $shouldUpdateEfl;
> > +my $shouldUpdateGtk;
> 
> We should sort this list of declarations and insert these variable declarations in sorted order.
> 
> > Tools/Scripts/webkitdirs.pm:899
> > +sub determineIsWK2
> 
> Please add "()" to the end of this line to force Perl to error when this function is called with an argument.
> 
> > Tools/Scripts/webkitdirs.pm:1054
> > +sub determineShouldUpdateEfl
> 
> Ditto.
> 
> > Tools/Scripts/webkitdirs.pm:1060
> > +sub shouldUpdateEfl
> 
> Ditto.
> 
> > Tools/Scripts/webkitdirs.pm:1392
> > +sub shouldUseGuardMalloc
> 
> Please add "()" to the end of this line to force Perl to error when this function is called with an argument.
> 
> > Tools/Scripts/webkitdirs.pm:1398
> > +sub determineShouldUseGuardMalloc
> 
> Ditto.
> 
> > Tools/Scripts/webkitdirs.pm:2360
> > +sub initilizePrivateOptions($)
> 
> Nit: initilizePrivateOptions => initializePrivateOptions
> 
> That is, the word "initialize" is misspelled in the name of this function.
> 
> > Tools/Scripts/webkitdirs.pm:2371
> > +        'efl' => \$isEfl,
> > +        'gtk' => \$isGtk,
> > +        'blackberry' => \$isBlackBerry,
> > +        'wince' => \$isWinCE,
> > +        'wincairo' => \$isWinCairo,
> > +        '64-bit' => \$isWin64,
> > +        'update-efl' => \$shouldUpdateEfl,
> > +        'update-gtk' => \$shouldUpdateGtk
> 
> Nit: ' => "
> 
> We use double quotes for string literals unless we explicitly don't want string interpolation.
> 

Fixed all above.

> > Tools/Scripts/webkitdirs.pm:2376
> > +    determineArchitecture();
> > +    determinePassedConfiguration();
> > +    determineIsWK2();
> 
> Is this necessary to call these functions? I mean, we'll ultimately call these functions when a caller queries for the such information (e.g. when architecture() is called).

Yes, they should be called some time as you said, but the problem is that if they are not called before "GetOptions" call in "build-webkit", "GetOptions" will fail :( because options like "--debug" "--release" "-2" are handled internally by webkitdir's functions, and are not in the "options" hash table, and calling those functions here process the arguments and remove them from ARGV, preventing "GetOptions" to fail. I preferred do not put those options into the "options" hash table because they have some custom handling (e.g: determinePassedConfiguration()) differently from other options which only set some variable (such as $isEfl, $shouldUpdateGtk, etc), but I'm not sure that's is the best approach to use.

[Nick Diego Yamane (diegoyam)]

Created attachment 217343 [details]
Patch

[Daniel Bates]

Comment on attachment 217343 [details]
Patch

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

The proposed patch is brittle. I wish we could come up with less fragile solution.

> Tools/ChangeLog:26
> +        (buildAutotoolsProject): Calls shouldUpdateGtk

Nit: Missing a period at the end of this line.

> Tools/ChangeLog:27
> +        (buildCMakeProjectOrExit): Calls shouldUpdateEfl

Ditto.

> Tools/Scripts/build-webkit:133
> +    initializePrivateOptions(\%options);

Maybe a better name for this function would be appendCommandLineOptionsToDictionary? Also, it seems sufficient to call this function for all ports. The advantage of this approach is that errors introduced to appendCommandLineOptionsToDictionary() and its subroutines will also be noticed by people building for Mac and iOS. This will expedite fixing such errors and/or reverting the culprit commit(s).

> Tools/Scripts/webkitdirs.pm:2363
> +    my %privateOptions = (

Please add --nix and --inspector-frontend to this dictionary.

[Brady Eidson]

Comment on attachment 217343 [details]
Patch

Assuming that patches for review since 2013 are stale, r-