529319 - plc4.dll.a can't be found on mingw build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Jacek Caban]

Attachment: fix

I get following error when compiling on mingw:

make[2]: Entering directory `/home/jacek/mozilla-build/wine-gecko-git/security/nss/cmd/certutil'
make[2]: *** No rule to make target `/home/jacek/mozilla-build/mozilla-build/dist/lib/plc4.dll.a', needed by `/home/jacek/mozilla-build/mozilla-build/nss/certutil.exe

It's because plc4 doesn't use IMPORT_LIB_SUFFIX for its importlib:

$ ls plc*
plc4.a  plc4.dll  plc4_s.a

The fix is not to use LIB_SUFFIX instead of IMPORT_LIB_SUFFIX for importing plc4.

Comment 1

[Jacek Caban]

Could you please review this patch?

Comment 2

[Wan-Teh Chang]

Hi Jacek,

I believe this bug is introduced by your patch to
mozilla/security/manager/Makefile.in in bug 505736,
where you override NSS's IMPORT_LIB_SUFFIX on the
command line.  NSS's original value of IMPORT_LIB_SUFFIX
for MinGW is .a.  See

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/coreconf/WIN32.mk&rev=1.37&mark=89-90#89

and

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/coreconf/WIN32.mk&rev=1.37&mark=290-292#290

We should implement one of the following two solutions.

1. Change Mozilla to deal with the different naming convention
used in NSS and NSPR.

or

2. Change NSS and NSPR to follow Mozilla's naming convention.
Although I still think the .dll.a suffix looks strange, I am
willing to change NSS and NSPR to follow Mozilla's naming
convention, so that you are not caught between the two sides.

Comment 3

[Jacek Caban]

Thanks for your reply.

(In reply to comment #2)
> Hi Jacek,
> 
> I believe this bug is introduced by your patch to
> mozilla/security/manager/Makefile.in in bug 505736,
> where you override NSS's IMPORT_LIB_SUFFIX on the
> command line.  NSS's original value of IMPORT_LIB_SUFFIX
> for MinGW is .a.  See
> 
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/coreconf/WIN32.mk&rev=1.37&mark=89-90#89
> 
> and
> 
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/coreconf/WIN32.mk&rev=1.37&mark=290-292#290

The patch didn't really introduce the bug. The bug was there and the patch has made it visible (and, BTW, the patch wasn't mine). Although NSS had IMPORT_LIB_SUFFIX, it was always the same as LIB_SUFFIX previously, so it wasn't well tested and was mistaken in some places. When testing bug 505736, I didn't see it because not all NSS target were built (--enable-tests enables more targets).

> We should implement one of the following two solutions.
> 
> 1. Change Mozilla to deal with the different naming convention
> used in NSS and NSPR.
> 
> or
> 
> 2. Change NSS and NSPR to follow Mozilla's naming convention.
> Although I still think the .dll.a suffix looks strange, I am
> willing to change NSS and NSPR to follow Mozilla's naming
> convention, so that you are not caught between the two sides.

This bug does not cross Mozilla/NSS boundaries, but while we're at this, I'd vote for
3. Get rid of IMPORT_LIB_SUFFIX.
It's not really useful IMO. All configs except mingw have it the same as LIB_SUFFIX. There is not reason for them to be different. The original reason for this was for static libraries not to be confused with import libraries, but it's no longer true. Building with MSC uses the same suffixes for both and there is no problem with it. mingw being the only target that has them different causes problems, because it's not tested by most developers and I expect more problems with it in the future. I've proposed removing it, but cls didn't like the idea (perhaps because I didn't make my point clear enough):
https://bugzilla.mozilla.org/show_bug.cgi?id=505736#c6
This will also fix the bug.

I'm happy to go any direction you like. My patch was meant to make as few changes as possible. I will attach a patch that removes IMPORT_LIB_SUFFIX from security module. If you think it should be done different way, please let me know which and I will try to make a patch.

Comment 4

[Jacek Caban]

Attachment: A patch removing IMPORT_LIB_SUFFIX from security module.

Comment 5

[Wan-Teh Chang]

Jacek: you mean you get this build error when doing a standalone
NSS build?  But where did the dll.a suffix for plc4.dll.a come
from?  The "dll.a" string isn't in the NSS source tree
(security/svrcore is not part of NSS):
http://mxr.mozilla.org/security/search?string=dll.a

but is in Mozilla's configure.in:
http://mxr.mozilla.org/mozilla-central/search?string=dll.a

Comment 6

[Jacek Caban]

No, I get this error when building NSS as part of m-c, but the bug is caused by dependencies inside NSS, so it's NSS bug as long as we consider IMPORT_LIB_SUFFIX different than LIB_SUFFIX valid. It should be possible to see it by building standalone NSS with changed IMPORT_LIB_SUFFIX, I will try it.

Comment 7

[Wan-Teh Chang]

Comment on attachment 424765 [details] [diff] [review]
A patch removing IMPORT_LIB_SUFFIX from security module.

Jacek: security/manager/Makefile.in is part of Mozilla.
It's the Mozilla makefile that builds NSS.

NSS consists of:
  mozilla/security/coreconf
  mozilla/security/nss
  (and also
  mozilla/security/dbm
  mozilla/dbm)

For NSS, I think it's fine to keep the arguably useless
IMPORT_LIB_SUFFIX variable.

It seems that your change to security/manager/Makefile.in
in this patch is all we need.  That's essentially my proposal
1 (NSS's IMPORT_LIB_SUFFIX equals Mozilla's LIB_SUFFIX).

I'd still like to know exactly how you're building certutil
so that you get the plc4.dll.a file name.

Comment 8

[Wan-Teh Chang]

Jacek: Your comment 6 answered my question.

mozilla/nsprpub (NSPR) has its own build system.  NSPR
doesn't distinguish between LIB_SUFFIX and IMPORT_LIB_SUFFIX.
So when you override IMPORT_LIB_SUFFIX for NSS, it affects
NSS but doesn't affect NSPR, so NSPR's import libraries
still have the original .a suffix.

Comment 9

[Jacek Caban]

I'm sorry for confusion, I considered all security directory except security/manager/Makefile.in NSS. I will attach security/manager/Makefile.in part as a fix for this bug. I've confirmed that it fixes the problem. Thanks for looking at it!

Comment 10

[Jacek Caban]

Attachment: fix

Comment 11

[Wan-Teh Chang]

Comment on attachment 424832 [details] [diff] [review]
fix

r=wtc.

In the commit comment, please note that NSS's IMPORT_LIB_SUFFIX
is different from Mozilla's IMPORT_LIB_SUFFIX, but happens to
be equal to Mozilla's LIB_SUFFIX.

Comment 12

[Wan-Teh Chang]

Comment on attachment 424832 [details] [diff] [review]
fix

Jacek: you can simply remove the entire
ifneq (,$(filter OS2 WINCE WINNT,$(OS_ARCH)))
and just keep the "else" part.

I found that this code seems to be based on the following code in mozilla/configure.in:

http://mxr.mozilla.org/mozilla-central/source/configure.in#4383

4383    if test -z "$GNU_CC" && test "$OS_ARCH" = "WINNT" -o "$OS_ARCH" = "WINCE" -o "$OS_ARCH" = "OS2"; then
4384        NSS_LIBS="\
4385         \$(LIBXUL_DIST)/lib/\$(LIB_PREFIX)crmf.\$(LIB_SUFFIX) \
4386         \$(LIBXUL_DIST)/lib/\$(LIB_PREFIX)smime$NSS_VERSION.\$(IMPORT_LIB_SUFFIX) \
4387         \$(LIBXUL_DIST)/lib/\$(LIB_PREFIX)ssl$NSS_VERSION.\$(IMPORT_LIB_SUFFIX) \
4388         \$(LIBXUL_DIST)/lib/\$(LIB_PREFIX)nss$NSS_VERSION.\$(IMPORT_LIB_SUFFIX) \
4389         \$(LIBXUL_DIST)/lib/\$(LIB_PREFIX)nssutil$NSS_VERSION.\$(IMPORT_LIB_SUFFIX)"
4390    else
4391        NSS_LIBS='$(LIBS_DIR)'" -lcrmf -lsmime$NSS_VERSION -lssl$NSS_VERSION -lnss$NSS_VERSION -lnssutil$NSS_VERSION"
4392    fi

That code has an extra test for GNU_CC, which wasn''t carried over to this file.

Comment 13

[Jacek Caban]

(In reply to comment #12)
> (From update of attachment 424832 [details] [diff] [review])
> Jacek: you can simply remove the entire
> ifneq (,$(filter OS2 WINCE WINNT,$(OS_ARCH)))
> and just keep the "else" part.

They are not exactly the same. Note that libraries in else part don't contain version numbers in file names.

> I found that this code seems to be based on the following code in
> mozilla/configure.in:
> 
> http://mxr.mozilla.org/mozilla-central/source/configure.in#4383
> 
> 4383    if test -z "$GNU_CC" && test "$OS_ARCH" = "WINNT" -o "$OS_ARCH" =
> "WINCE" -o "$OS_ARCH" = "OS2"; then
> 4384        NSS_LIBS="\
> 4385         \$(LIBXUL_DIST)/lib/\$(LIB_PREFIX)crmf.\$(LIB_SUFFIX) \
> 4386        
> \$(LIBXUL_DIST)/lib/\$(LIB_PREFIX)smime$NSS_VERSION.\$(IMPORT_LIB_SUFFIX) \
> 4387        
> \$(LIBXUL_DIST)/lib/\$(LIB_PREFIX)ssl$NSS_VERSION.\$(IMPORT_LIB_SUFFIX) \
> 4388        
> \$(LIBXUL_DIST)/lib/\$(LIB_PREFIX)nss$NSS_VERSION.\$(IMPORT_LIB_SUFFIX) \
> 4389        
> \$(LIBXUL_DIST)/lib/\$(LIB_PREFIX)nssutil$NSS_VERSION.\$(IMPORT_LIB_SUFFIX)"
> 4390    else
> 4391        NSS_LIBS='$(LIBS_DIR)'" -lcrmf -lsmime$NSS_VERSION
> -lssl$NSS_VERSION -lnss$NSS_VERSION -lnssutil$NSS_VERSION"
> 4392    fi
> 
> That code has an extra test for GNU_CC, which wasn''t carried over to this
> file.

I've missed that part. It looks like we no longer need a special case for GCC. I will attach an updated patch.

Thanks!

Comment 14

[Jacek Caban]

Attachment: fix including configure.in

Comment 15

[Wan-Teh Chang]

Jacek: You're right.  In that case, we don't need the
else part at all, because the else part cannot be correct.
(The libraries without version numbers are the normal
static libraries on Unix.  NSS-based applications should
not link with NSS static libraries.  Even if they want
to, they'll need to link with much more static libraries
than those listed in the else part.)

Comment 16

[Wan-Teh Chang]

Comment on attachment 425255 [details] [diff] [review]
fix including configure.in

Your change to configure.in seems overly aggressive.
You should simply change IMPORT_LIB_SUFFIX to LIB_SUFFIX.
The if part you deleted is not used by MinGW.

Comment 17

[Jacek Caban]

I'm sorry about my configure.in. I was expecting this code to be used by mingw and didn't read the if carefully enough. I will attach a fixed version and remove that else part from Makefile.in. Thanks.

Comment 18

[Jacek Caban]

Attachment: fix including configure.in v1.1

Comment 19

[Wan-Teh Chang]

Comment on attachment 425266 [details] [diff] [review]
fix including configure.in v1.1

I discovered that the else part in mozilla/security/manager/Makefile.in
cannot be removed, otherwise the makefile rule below will break because
$(SDK_LIBS) is empty:

        $(INSTALL) -m 755 $(SDK_LIBS) $(DIST)/sdk/lib

I can take of that when I check this in, so you don't need to prepare
a new patch.

Comment 20

[Wan-Teh Chang]

Attachment: fix v2

I pushed this patch to mozilla-central in changeset 72d91445b838:
http://hg.mozilla.org/mozilla-central/rev/72d91445b838