418304 - jsDriver.pl: Replacing Getopt::Mixed by Getopt::Long

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

Currently jsDriver.pl requires to install Getopt::Mixed Perl module. Since the module is not a part of the standard Perl distribution and obsolete, downloading and installing it brings en extra hassle that one must overcome before running the test suites. It would be nice to replace the module with Getopt::Long as explained below in the fragment from README for Getopt::Mixed :

Getopt::Mixed 1.10
Copyright 1995 Christopher J. Madsen

This module is obsolete.

Getopt::Mixed provides GNU-style option processing for Perl 5 scripts,
with both long and short options.  Please see the documentation at the
end of the module for instructions on its use and licensing
restrictions.

However, some time ago Getopt::Long was changed to support short
options as well, and it has the huge advantage of being part of the
standard Perl distribution.  So, Getopt::Mixed is now effectively
obsolete.  I don't intend to make any more changes, but I'm leaving it
available for people who have code that already uses it.  For new
modules, I recommend using Getopt::Long like this:

    use Getopt::Long;
    Getopt::Long::Configure(qw(bundling no_getopt_compat));
    GetOptions(...option-descriptions...);

Comment 1

[Igor Bukanov]

Attachment: v1

The new version replaces Getopt::Mixed with Getopt::Long. To support usage like 

perl jsDriver.pl -e smdebug -l item1 item2 -L excludeList1 excludeList2 

I have to turn off the bundling feature so it is not possible to use a usage like -tk, one has to use -t -k instead.

Comment 2

[Bob Clary [:bc] (inactive)]

Comment on attachment 316465 [details] [diff] [review]
v1

>Index: jsDriver.pl
>===================================================================
...
> 
> sub parse_args {
>-    my ($option, $value, $lastopt);
>+    my $help = 0;
>+    my $suite_path;
>+    my $disable_exit_munge = 0;
>+    my $narcissus;
> 
>     &dd ("checking command line options.");
> 
...
>+    use Getopt::Long;
>+    Getopt::Long::Configure(qw(no_getopt_compat no_ignore_case));
>+    Getopt::Long::GetOptions ("bugurl|b=s" => \$opt_bug_url,
>+                              "classpath|c=s" => \$opt_classpath,
>+                              "engine|e=s{1,}" => \@opt_engine_list,
>+                              "file|f=s" => \$opt_output_file,
>+                              "help|h" => \$help,
>+                              "javapath|j=s" => \$opt_java_path,
>+                              "confail|k" => \$opt_console_failures,
>+                              "linefail|K" => \$opt_console_failures_line,
>+                              "report|R!" => \$opt_report_summarized_results,
>+                              "list|l=s{0,}" => \@opt_test_list_files,
>+                              "neglist|L=s{0,}" => \@opt_neg_list_files,

why 0 or more values for list and neglist. shouldn't it be {1,} ?

>+                              "opt|o=s" => \$opt_engine_params,
>+                              "testpath|p=s" => \$suite_path,
>+                              "shellpath|s=s" => \$opt_shell_path,
>+                              "trace|t" => \$opt_trace,
>+                              "lxrurl|u=s" => \$opt_lxr_url,
>+                              "timeout|T=s" => \$opt_timeout,
>+                              "noexitmunge|x=s" => \$disable_exit_munge,

why a required string value here? that is a change from previously isn't it?

>+                              "narcissus|n:s" => \$narcissus,
>+                              "noquitinthandler|Q" => $opt_no_quit);
...
>+                
>+    if (@opt_engine_list == 0) {
>         die "You must select a shell to test in.\n";
>     }
>-
>+    if (@ARGV != 0) {
>+        die "Unsupported stnadalone argument.\n"

sp: standalone

>+    }
> }
> 

r+ with answers to the repeated value questions and the spelling nit.

Thanks! This is very clean.

Comment 3

[Igor Bukanov]

(In reply to comment #2)
> (From update of attachment 316465 [details] [diff] [review])
> >+                              "list|l=s{0,}" => \@opt_test_list_files,
> >+                              "neglist|L=s{0,}" => \@opt_neg_list_files,
> 
> why 0 or more values for list and neglist. shouldn't it be {1,} ?

I have used {0,} so -L -l would not be interpreted as 'add a file named "-l" to the exclusion list'. With Getopt::Mixed -L -l is equivalent to -l. But I am not going to insist on this.

Comment 4

[Igor Bukanov]

Attachment: v2

The new version addresses the nits and uses sub {...} to process non-trivial parameter cases.

Comment 5

[Igor Bukanov]

I checked in the patch from the comment 4 to the trunk:

Checking in jsDriver.pl;
/cvsroot/mozilla/js/tests/jsDriver.pl,v  <--  jsDriver.pl
new revision: 1.74; previous revision: 1.73
done

Comment 6

[Igor Bukanov]

I backed out the patch as it depends on the repeat count in Getopt::Long, a feature that is not widely supported.

Comment 7

[Jason Orendorff [:jorendorff]]

Bug 463746 fixed this (in a different way).