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.

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

(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! :-)

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

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.

Necessary for app signing (bug 772365).

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 .

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

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 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 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.

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

(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.

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