683266 - Remove certdata.c from the source tree, and autogenerate it from certdata.txt during the build process

Status: RESOLVED FIXED

(NSS :: Build, defect, P1)

Description

[(no longer active)]

cacert.c is an autogenerated C file from cacert.txt.  I'm not sure why we can't just remove it from the source tree and generate it automatically as part of the build process.  I noticed this when I was trying to transplant the patches for bug 682927 across multiple branches today.

Comment 1

[Kaspar]

To be precise, it's security/nss/lib/ckfw/builtins/certdata.{txt,c}, and certdata.perl is the script which generates the .c file.

Comment 2

[(no longer active)]

(In reply to comment #1)
> To be precise, it's security/nss/lib/ckfw/builtins/certdata.{txt,c}, and
> certdata.perl is the script which generates the .c file.

Yes, I stand corrected!  :-)

Comment 3

[Wan-Teh Chang]

It used to work that way, until we needed to port
this code to Mac OS Classic and had trouble running
perl.

Comment 4

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

Attachment: Generate certdata.c at built time and allow builder to override location of certdata.txt

This patch changes certdata.c to be generated at build time as $(OBJDIR)/certdata.c. It requires a "cvs rm mozilla/security/nss/lib/ckfw/builtins/certdata.c" too.

The builder can set NSS_CERTDATA_TXT to the patch of an alternate certdata.txt. We will be using this option in B2G to change the code-signing permissions and add the B2G code signing root to the database.

I modified certdata.perl so that it generates its output to stdout following prototypical Unix design. This required moving diagnostic messages to stderr instead of stdout.

I built this on all platforms using Mozilla's tryserver and all the builds succeeded.

Comment 5

[Lucas Adamski [:ladamski]]

Necessary for app signing (bug 772365).

Comment 6

[Wan-Teh Chang]

Comment on attachment 683243 [details] [diff] [review]
Generate certdata.c at built time and allow builder to override location of certdata.txt

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

::: mozilla/security/nss/lib/ckfw/builtins/README
@@ +28,5 @@
>  
>  5. Run gmake in this directory to build the nssckbi module.
>  
>  6. After you verify that the new nssckbi module is correct, check in
> +certdata.txt and and nssckbi.h.

Remove one "and".

@@ +40,1 @@
>  4. Edit nssckbi.h to bump the version of the module.

Steps 4-6 need to be renumbered.

::: mozilla/security/nss/lib/ckfw/builtins/certdata.perl
@@ +24,5 @@
>    s/^((?:[^"#]+|"[^"]*")*)(\s*#.*$)/$1/;
>    next if (/^\s*$/);
>  
>    if( /(^CVS_ID\s+)(.*)/ ) {
> +#    print STDERR "The CVS ID is $2\n";

Should we just delete this line and the other commented-out "print STDERR" line?

::: mozilla/security/nss/lib/ckfw/builtins/manifest.mn
@@ +9,5 @@
>  MODULE = nss
>  MAPFILE = $(OBJDIR)/nssckbi.def
>  
> +# Needed for compilation of $(OBJDIR)/certdata.c
> +INCLUDES += -I$(CORE_DEPTH)/nss/lib/ckfw/builtins

This should be moved to mozilla/ security/ nss/ lib/ ckfw/ builtins/ config.mk .

Comment 7

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

Attachment: Generate certdata.c at built time and allow builder to override location of certdata.txt [v2]

Thanks for reviewing the patch, Wan-Teh. I made the changes you suggested, including removing the "print STDERR" commands.

Comment 8

[Robert Relyea]

Comment on attachment 686369 [details] [diff] [review]
Generate certdata.c at built time and allow builder to override location of certdata.txt [v2]

r+ with some comments.

It would be nice to have the perl script take the filename as an optional argument. I'm r+ this patch because making the perl script output to standard out rather than a hardwired filename is definately an improvement.

I'm a little mystified how the $(OBJDIR)/certdata.c works when the old one was simply certdata.c.

Comment 9

[Wan-Teh Chang]

Comment on attachment 686369 [details] [diff] [review]
Generate certdata.c at built time and allow builder to override location of certdata.txt [v2]

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

I'm approving this patch only because I don't want to block B2G inadvertently.

The summary of this bug only describes one aspect of this patch. I object
to the other aspect of this patch (using the NSS_CERTDATA_TXT environment
variable to point to an alternative certdata.txt file outside the NSS
source tree). I hope we can figure out a better solution.

::: mozilla/security/nss/lib/ckfw/builtins/Makefile
@@ +46,5 @@
>  # Generate certdata.c.
> +
> +# By default, use the unmodified certdata.txt.
> +ifndef NSS_CERTDATA_TXT
> +NSS_CERTDATA_TXT = certdata.txt

I don't like pointing to a file outside the NSS source tree.

Why can't you check in the modified certdata.txt in the copy of NSS
in B2G's Hg repository?

It should be easy to maintain that with a private NSS patch.

Another possible solution is to change the certdata.perl script to
read certdata.txt and an optional certdata2.txt, if exists. Then
you just need to maintain the certdata2.txt file in the copy of NSS
in B2G's Hg repository. This solution will require some makefile
trickery to make $(OBJDIR)/certdata.c depend on an optional file,
but it can be done.

Comment 10

[Wan-Teh Chang]

Comment on attachment 686369 [details] [diff] [review]
Generate certdata.c at built time and allow builder to override location of certdata.txt [v2]

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

::: mozilla/security/nss/lib/ckfw/builtins/config.mk
@@ +24,5 @@
>      DEFINES += -DNSSDEBUG
>  endif
>  
> +# Needed for compilation of $(OBJDIR)/certdata.c
> +INCLUDES += -I$(CORE_DEPTH)/nss/lib/ckfw/builtins

I suspect -I. also works.

Comment 11

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

Thank you for the reviews!

> Comment on attachment 686369 [details] [diff] [review]
> It would be nice to have the perl script take the filename as an optional
> argument. I'm r+ this patch because making the perl script output to
> standard out rather than a hardwired filename is definately an improvement.

Bob, I don't object to doing that but also I am not sure what the benefit is, since we can always redirect stdout. Let's make that enhancement in another bug if it is needed for something.

> I'm a little mystified how the $(OBJDIR)/certdata.c works when the old one
> was simply certdata.c.

Me too, to be honest.

Checking in mozilla/security/nss/lib/ckfw/builtins/config.mk;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/config.mk,v  <--  config.mk
new revision: 1.16; previous revision: 1.15(In reply to Robert Relyea from comment #8)

(In reply to Wan-Teh Chang from comment #9)
> I don't like pointing to a file outside the NSS source tree.
> 
> Why can't you check in the modified certdata.txt in the copy of NSS
> in B2G's Hg repository?
> 
> It should be easy to maintain that with a private NSS patch.

That is a possible alternative. However, it means that if/when we add/remove roots from the normal NSS certdata.txt, the person importing new versions of NSS into mozilla-central must do additional work during every import, including merging a guaranteed conflict in certdata.txt. The way I am doing things now, using the NSS_CERTDATA_TXT variable, allows me to do this merging automatically as part of the Gecko build:

  # Distrust all existing builtin CAs for code-signing
  hacked-certdata.txt : $(srcdir)/../nss/lib/ckfw/builtins/certdata.txt
      sed -e "s/^CKA_TRUST_CODE_SIGNING.*CKT_NSS_TRUSTED_DELEGATOR.* \
               /CKA_TRUST_CODE_SIGNING CK_TRUST CKT_NSS_MUST_VERIFY_TRUST/" \
          $< > $@

  combined-certdata.txt : hacked-certdata.txt $(srcdir)/b2g-certdata.txt
      cat $^ > $@

  libs:: combined-certdata.txt

  DEFAULT_GMAKE_FLAGS += NSS_CERTDATA_TXT='$(CURDIR)/combined-certdata.txt'

Long-term, I want to change the way Gecko works so that we don't need to modify the existing contents of the NSS certdata.txt at all. That will allow us to use the technique that you suggest. I filed bug 816820 in PSM to track that work. Once that is done, we can revisit the topic of whether to remove the NSS_CERTDATA_TXT option.

Checking in mozilla/security/nss/lib/ckfw/builtins/Makefile;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/Makefile,v  <--  Makefile
new revision: 1.22; previous revision: 1.21
done
Checking in mozilla/security/nss/lib/ckfw/builtins/README;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/README,v  <--  README
new revision: 1.6; previous revision: 1.5
done
Removing mozilla/security/nss/lib/ckfw/builtins/certdata.c;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.c,v  <--  certdata.c
new revision: delete; previous revision: 1.90
done
Checking in mozilla/security/nss/lib/ckfw/builtins/certdata.perl;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.perl,v  <--  certdata.perl
new revision: 1.16; previous revision: 1.15
done

Comment 12

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

(In reply to Wan-Teh Chang from comment #10)
> I suspect -I. also works.

It does work and I made that change prior to checkin.

Comment 13

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

Fixed in bug 818717 for mozilla-aurora and mozilla-beta.