1294417 - Change certdata.perl to take input and output filenames as command line arguments

Status: RESOLVED FIXED

(NSS :: Build, defect)

Description

[(not currently active) Ted Mielczarek]

In bug 1237872 I'm writing gyp files to build NSS. It turns out to be difficult to make the certdata.perl invocation work on Windows because it assumes you have a POSIX shell, and that's hard to do with gyp+ninja. I instead wrote a patch that changes certdata.perl to simply take the input and output filenames as command line arguments, which simplifies the invocation on Windows and doesn't complicate the script much at all.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: Make certdata.perl take input/output as command-line parameters

My Perl is very rusty, but this works and I verified that it produces the same output as the existing version.

Comment 2

[Wan-Teh Chang]

Comment on attachment 8780071 [details] [diff] [review]
Make certdata.perl take input/output as command-line parameters

Review of attachment 8780071 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. Thanks. Your comment implies that standard I/O redirection
requires a shell, correct?

::: lib/ckfw/builtins/certdata.perl
@@ +13,5 @@
>  
>  $constants{CK_TRUE} = "static const CK_BBOOL ck_true = CK_TRUE;\n";
>  $constants{CK_FALSE} = "static const CK_BBOOL ck_false = CK_FALSE;\n";
>  
> +open(my $infh, '<', $ARGV[0])

Nit: I recommend adding an underscore: in_fh and out_fh.

Note: This is the convention used in http://perldoc.perl.org/functions/open.html.
The all-caps (e.g., 'FH') file handle names I am familiar with are called
"older style" by that man page. Current NSS perl scripts use the older
style.

@@ +97,5 @@
>  
>  sub dudump {
>  my $i;
>  for( $i = 1; $i <= $count; $i++ ) {
> +  print $outfh "\n";

In one NSS perl script (ckapi.perl), I saw the use of 'select',
which seems to redirect standard output to the file handle.
If that works, we don't need to modify the print statements.
We can also consider that. If you prefer being explicit, that's
fine, too.

@@ +185,3 @@
>  }
> +
> +close $outfh;

Also close $infh ?

Comment 3

[(not currently active) Ted Mielczarek]

Attachment: Make certdata.perl take input/output as command-line parameters (v2)

I looked at the perl docs and you're right, this is much easier if we just redirect STDIN/STDOUT by opening the input/output files. Much smaller patch!

(In reply to Wan-Teh Chang from comment #2)
> r=wtc. Thanks. Your comment implies that standard I/O redirection
> requires a shell, correct?

Yes, running `perl < certdata.txt > certdata.c` doesn't work on Windows unless you run it under some sort of POSIX shell like cygwin or msys.

Comment 4

[Wan-Teh Chang]

Comment on attachment 8780236 [details] [diff] [review]
Make certdata.perl take input/output as command-line parameters (v2)

Review of attachment 8780236 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. Wow! This is indeed much simpler.

Comment 5

[Wan-Teh Chang]

Comment on attachment 8780236 [details] [diff] [review]
Make certdata.perl take input/output as command-line parameters (v2)

Review of attachment 8780236 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ckfw/builtins/certdata.perl
@@ +13,5 @@
>  
>  $constants{CK_TRUE} = "static const CK_BBOOL ck_true = CK_TRUE;\n";
>  $constants{CK_FALSE} = "static const CK_BBOOL ck_false = CK_FALSE;\n";
>  
> +open(STDIN, '<', $ARGV[0])

Ted: should we check the number of command-line arguments
and a usage message?

if ($#ARGV != 1) {
  print "Usage: $0 <input-file> <output-file>\n";
  exit 1;
}

Comment 6

[Wan-Teh Chang]

Comment on attachment 8780236 [details] [diff] [review]
Make certdata.perl take input/output as command-line parameters (v2)

Review of attachment 8780236 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ckfw/builtins/certdata.perl
@@ +13,5 @@
>  
>  $constants{CK_TRUE} = "static const CK_BBOOL ck_true = CK_TRUE;\n";
>  $constants{CK_FALSE} = "static const CK_BBOOL ck_false = CK_FALSE;\n";
>  
> +open(STDIN, '<', $ARGV[0])

It seems that the usage message should go to stderr:

if ($#ARGV != 1) {
  print STDERR "Usage: $0 <input-file> <output-file>\n";
  exit 1;
}

Comment 7

[(not currently active) Ted Mielczarek]

Attachment: Make certdata.perl take input/output as command-line parameters (v3)

I needed one more change to make this play well with the Mozilla build system--it only supports file generation scripts that output to an already-open file, so I made the output file parameter optional to allow it to continue to output to stdout. I added a usage message as you suggested if no parameters are given.

Comment 8

[Wan-Teh Chang]

Comment on attachment 8798835 [details] [diff] [review]
Make certdata.perl take input/output as command-line parameters (v3)

Review of attachment 8798835 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. Please fix the indentation level.

::: lib/ckfw/builtins/certdata.perl
@@ +15,5 @@
>  $constants{CK_FALSE} = "static const CK_BBOOL ck_false = CK_FALSE;\n";
>  
> +if( scalar @ARGV == 0 ) {
> +    print STDERR "Usage: $0 <input-file> [output-file]\n";
> +    exit 1;

Nit: the indentation level is two spaces in this file.

Comment 9

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/projects/nss/rev/c3854cc8537134eb49f37a842d7247f93ecae2a2
bug 1294417 - Make certdata.perl take input/output as command-line parameters. r=wtc