Closed Bug 505736 Opened 13 years ago Closed 13 years ago

Wrong IMPORT_LIB_SUFFIX on MinGW.

Categories

(Firefox Build System :: General, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jacek, Unassigned)

Details

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.1) Gecko/20090715 Firefox/3.5.1
Build Identifier: 

IMPORT_LIB_SUFFIX should be 'a' on MinGW. Its import library names have form of lib<name>.a.

Reproducible: Always
Attached patch Fix (obsolete) — Splinter Review
Attachment #389936 - Flags: review?(ted.mielczarek)
IIRC, using lib<name>.a for import libs conflicts with the static libs which have the same naming convention.  See bug 134113 comment 301 & 302.  

Also, see the "direct linking to a dll" section at http://www.redhat.com/docs/manuals/enterprise/RHEL-4-Manual/gnu-linker/win32.html .
I've looked deeper at this. Here are some facts that I found. First of all IMPORT_LIB_SUFFIX is used only by security module and compilation works fine for me with my patch. I cross compile on Linux using MinGW, but it looks like cross compiling doesn't change anything regardless this bug. Without this patch I get error:

/home/jacek/mozilla-build/mozilla-build/config/nsinstall -R -m 755 ../../dist/lib/libcrmf.a ../../dist/lib/libsmime3.dll.a ../../dist/lib/libssl3.dll.a ../../dist/lib/libnss3.dll.a ../../dist/lib/libnssutil3.dll.a ../../dist/lib/libsoftokn3.dll.a  ../../dist/sdk/lib
/home/jacek/mozilla-build/mozilla-build/config/nsinstall: cannot access ../../dist/lib/libsmime3.dll.a: No such file or directory

It's because most security makefiles don't take configure's IMPORT_LIB_SUFFIX into account and instead set it in WIN32.mk to .$(LIB_SUFFIX). Fixing it is not enough as I get other errors then.

The fact that compilation works with my patch and other parts of code don't take IMPORT_LIB_SUFFIX into account suggests me that one of two solutions seem to be the best:

- Keep configure's and security makefiles' IMPORT_LIB_SUFFIX in sync - that's what my patch does.

- Get rid of IMPORT_LIB_SUFFIX and always use LIB_SUFFIX.

Other option would be fixing security makefiles to properly use IMPORT_LIB_SUFFIX. That would mean that whole other code should be considered as buggy and should fixed as well, so we'd have to teach all other modules about it. It seems to be much more complication for no good point IMO.

I'd appreciate a comment about what's the prefered way to go.
Attachment #389936 - Flags: review?(ted.mielczarek)
Comment on attachment 389936 [details] [diff] [review]
Fix

Dropping review request per cls' comment. If you attach an updated patch, please r?cls directly.
This patch removes IMPORT_LIB_SUFFIX. AFAICS it's different from LIB_SUFFIX only when compiling on MinGW and it only causes troubles in this case. Note that if static lib conflicts with import lib, it's already handled by using lib<name>_s.a schema in current build system, so IMPORT_LIB_SUFFIX is useless complication.
Attachment #392742 - Flags: review?(cls)
Comment on attachment 392742 [details] [diff] [review]
Always use LIB_SUFFIX instead of IMPORT_LIB_SUFFIX.

We should not revert the use of IMPORT_LIB_SUFFIX just because of the known discrepancy between Mozilla's & NSS' naming conventions. 

It's been awhile since I've looked at this but I seem to recall that NSS didn't properly support mingw in their standalone builds and that we were expected to pass in the proper make variables to get it to work as part of Mozilla.  In this case, the problem is easily solved by passing in IMPORT_LIB_SUFFIX to DEFAULT_GMAKE_FLAGS in security/manager/Makefile.in .
Attachment #392742 - Flags: review?(cls) → review-
Attachment #389936 - Attachment is obsolete: true
Attachment #392742 - Attachment is obsolete: true
Attachment #392855 - Flags: review?(ted.mielczarek)
Your patch works great, thanks. If we're not going to change IMPORT_LIB_SUFFIX, then there is another improper use of IMPORT_LIB_SUFFIX in browser/components/build/Makefile.in. I will attach a patch.
Attachment #392987 - Flags: review?(cls)
Comment on attachment 392987 [details] [diff] [review]
Use IMPORT_LIB_SUFFIX for linking thebes library.

I'm not sure what tree you're using but I don't see thebes mentioned in browser/components/build/Makefile.in in either tree.  

The change looks correct though but it looks like the EXPAND_MOZLIBNAME macro/description is wrong in config/rules.mk.
Attachment #392855 - Flags: review?(ted.mielczarek) → review+
Attachment #392987 - Flags: review?(cls) → review+
Keywords: checkin-needed
Both patches have been checked into mozilla-central and the security/manager patch has been checked into CVS HEAD.

http://hg.mozilla.org/mozilla-central/rev/8ef666467a3b

Checking in security/manager/Makefile.in;
/cvsroot/mozilla/security/manager/Makefile.in,v  <--  Makefile.in
new revision: 1.84; previous revision: 1.83
done

http://hg.mozilla.org/mozilla-central/rev/546e369e6221
Status: UNCONFIRMED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.