812802 - addbuiltin outputs files with carriage returns on Windows

Status: RESOLVED FIXED

(NSS :: Tools, defect, P1)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Stop addbuiltin from outputting carriage returns on Windows

When addbuiltin is run on Windows, the output file has carriage returns in it. certdata.perl doesn't ignore the carriage returns correctly and this causes certdata.perl to generate C code that won't compile with GCC or clang. Also, the generated files are usually checked into source control, and files containing carriage returns are not allowed according to the Mozilla coding style guide.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

By the way, if you expand the context to "file" scope you can see that this new code is copy/pasted from the code immediately above it, that does the same thing for stdin (for a different reason).

Comment 2

[Wan-Teh Chang]

Comment on attachment 682776 [details] [diff] [review]
Stop addbuiltin from outputting carriage returns on Windows

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

::: mozilla/security/nss/cmd/addbuiltin/addbuiltin.c
@@ +480,5 @@
> +    {
> +	int smrv = _setmode(_fileno(stdout), _O_BINARY);
> +	if (smrv == -1) {
> +	    fprintf(stderr,
> +	    "%s: Cannot change stdin to binary mode. Use -i option instead.\n",

stdin => stdout

Remove the "Use -i option instead." sentence.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Stop addbuiltin from outputting carriage returns on Windows [v2]

Thanks for catching my silly oversight, Wan-Teh.

Comment 4

[Wan-Teh Chang]

Comment on attachment 685457 [details] [diff] [review]
Stop addbuiltin from outputting carriage returns on Windows [v2]

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

r=wtc.

::: mozilla/security/nss/cmd/addbuiltin/addbuiltin.c
@@ +474,5 @@
>      }
>  
> +#if defined(WIN32)
> +    /* We must put stdout into O_BINARY mode or else the output will include
> +    ** carriage returns.

Just wanted to point out that the correctness of this change depends on
the cvs.exe binary used on Windows. This change is correct for the cvs.exe
binary in the MozillaBuilds package, which checks out text files with the
Unix line endings (LF).

The official cvs.exe binary for Windows, which we used before, checks out
text files with the native line endings. Therefore the original behavior
is correct in that environment.

I believe the NSS developers who are likely to run addbuiltin are all
using the MozillaBuilds package on Windows, so I approve this change.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Checking in security/nss/cmd/addbuiltin/addbuiltin.c;
/cvsroot/mozilla/security/nss/cmd/addbuiltin/addbuiltin.c,v  <--  addbuiltin.c
new revision: 1.20; previous revision: 1.19
done

(In reply to Wan-Teh Chang from comment #4)
> Just wanted to point out that the correctness of this change depends on
> the cvs.exe binary used on Windows. This change is correct for the cvs.exe
> binary in the MozillaBuilds package, which checks out text files with the
> Unix line endings (LF).

Thanks for the review and thanks for pointing that out. I considered fixing this by modifying certdata.perl to strip out ^Ms. But, addbuiltin still needs to be modified because we're not supposed to check in files with ^M into repositories (Gecko or NSS). Maybe the other version of CVS would strip out the ^Ms on commit, but mozilla-central's Mercurial repository won't do that stripping.