Bug 1227795 - Add NSS_DISABLE_LIBPKIX to allow compiling without libpkix

Bug 1227795 - Add NSS_DISABLE_LIBPKIX to allow compiling without libpkix

[franziskus, 2016-05-26 13:54:53]

lgtm with the comments addressed.

Maybe ping Wan-Teh about it again. We'd need his review as well.

https://codereview.appspot.com/300000043/diff/1/lib/certhigh/certvfy.c
File lib/certhigh/certvfy.c (right):

https://codereview.appspot.com/300000043/diff/1/lib/certhigh/certvfy.c#newcode18
lib/certhigh/certvfy.c:18: /*#include "pkix_sample_modules.h" */
looks like this line could be removed

https://codereview.appspot.com/300000043/diff/1/lib/certhigh/certvfy.c#newcod...
lib/certhigh/certvfy.c:798: if (CERT_GetUsePKIXForValidation()) {
this returns false anyway so no need for the #ifndef

https://codereview.appspot.com/300000043/diff/1/lib/manifest.mn
File lib/manifest.mn (right):

https://codereview.appspot.com/300000043/diff/1/lib/manifest.mn#newcode33
lib/manifest.mn:33: NSS_SRCDIRS += \
is there a reason not to add this to the end of the list? would be less
NSS_SRCDIRS +=

[esawin, 2016-06-15 17:33:58]

On 2016/05/26 13:54:53, franziskus wrote:
> https://codereview.appspot.com/300000043/diff/1/lib/manifest.mn#newcode33
> lib/manifest.mn:33: NSS_SRCDIRS += \
> is there a reason not to add this to the end of the list? would be less
> NSS_SRCDIRS +=

We need to maintain the order because of inter-module dependencies, it wouldn't
compile otherwise.

[wtc1, 2016-06-15 18:07:34]

These changes look correct. Are they based on the patch in the Chromium source
tree?

I suggest an alternative change to lib/manifest.mn.

https://codereview.appspot.com/300000043/diff/20001/lib/manifest.mn
File lib/manifest.mn (left):

https://codereview.appspot.com/300000043/diff/20001/lib/manifest.mn#oldcode45
lib/manifest.mn:45: #  libpkix (not a separate dll)

Why did you delete this comment?

https://codereview.appspot.com/300000043/diff/20001/lib/manifest.mn
File lib/manifest.mn (right):

https://codereview.appspot.com/300000043/diff/20001/lib/manifest.mn#newcode34
lib/manifest.mn:34: libpkix \

Please handle this using a new LIBPKIX_SRCDIR variable. Use ZLIB_SRCDIR and
SYSINIT_SRCDIR as examples.

https://codereview.appspot.com/300000043/diff/20001/lib/nss/nssinit.c
File lib/nss/nssinit.c (right):

https://codereview.appspot.com/300000043/diff/20001/lib/nss/nssinit.c#newcode709
lib/nss/nssinit.c:709: 

Nit: please also delete these two blank lines.

[franziskus, 2016-06-23 10:00:02]

lgtm (and it builds! ;). r+ with those asserts added.

https://codereview.appspot.com/300000043/diff/60001/lib/certhigh/certvfypkix.c
File lib/certhigh/certvfypkix.c (right):

https://codereview.appspot.com/300000043/diff/60001/lib/certhigh/certvfypkix....
lib/certhigh/certvfypkix.c:64: PORT_SetError(PR_NOT_IMPLEMENTED_ERROR);
to make chasing down bugs related to this easier an PR_Assert would be great for
all of these functions.