Last Comment Bug 644987 - Wrong EXPAND_LIBNAME on mingw compilation
: Wrong EXPAND_LIBNAME on mingw compilation
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla6
Assigned To: Jacek Caban
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-25 06:30 PDT by Jacek Caban
Modified: 2011-04-26 23:43 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix v1.0 (1.11 KB, patch)
2011-03-25 06:30 PDT, Jacek Caban
mh+mozilla: review-
Details | Diff | Splinter Review
fix v1.1 (2.87 KB, patch)
2011-04-26 03:18 PDT, Jacek Caban
mh+mozilla: review+
Details | Diff | Splinter Review

Description Jacek Caban 2011-03-25 06:30:33 PDT
Created attachment 521797 [details] [diff] [review]
fix v1.0

Its GCC case was (probably accidentally) removed by bug 584474 part 9, resulting in wrong linking arguments on mingw.
Comment 1 Mike Hommey [:glandium] 2011-03-25 08:39:53 PDT
Comment on attachment 521797 [details] [diff] [review]
fix v1.0

That was deliberate. What exactly is the problem with mingw?
Comment 2 Jacek Caban 2011-03-25 08:49:15 PDT
Here is an example of the error (when crosscompiling using mingw on Linux):

/usr/bin/python2.7 /home/jacek/mozilla-build/mozilla-central/config/pythonpath.py -I../../../../config /home/jacek/mozilla-build/mozilla-central/config/expandlibs_exec.py --uselist --  i686-w64-mingw32-g++ -mno-cygwin   -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-variadic-macros -Werror=return-type -pedantic -Wno-long-long -fno-strict-aliasing -mms-bitfields -mstackrealign -pipe  -DDEBUG -D_DEBUG -DTRACING -g -o TestXREMakeCommandLineWin.exe TestXREMakeCommandLineWin.o -mconsole -static-libgcc -static-libstdc++    -L../../../../dist/bin -L../../../../dist/lib  -luuid -lgdi32 -lwinmm -lwsock32 libcomctl32.a libws2_32.a libshell32.a   
i686-w64-mingw32-g++: libcomctl32.a: No such file or directory
i686-w64-mingw32-g++: libws2_32.a: No such file or directory
i686-w64-mingw32-g++: libshell32.a: No such file or directory

Note that we use libcomctl32.a to link to comctl32, but the right syntax for system libraries is -lcomctl32. This argument is expanded by EXPAND_LIBNAME.
Comment 3 Jacek Caban 2011-04-22 08:31:19 PDT
(In reply to comment #1)
> That was deliberate.

Could you please tell me the reason? My guess was that it was done to allow using EXPAND_LIBNAME for static libraries created in a middle of compilation. I went thru all its occurrences in the tree and I've found that all of them were for Windows system libraries (except one for AIX, but that's still a system library). System libraries syntax needs to be -l... and EXPAND_LIBNAME seems like exactly the place to do this. How would you prefer this to be fixed?
Comment 4 Mike Hommey [:glandium] 2011-04-22 08:55:10 PDT
(In reply to comment #3)
> (In reply to comment #1)
> > That was deliberate.
> 
> Could you please tell me the reason? My guess was that it was done to allow
> using EXPAND_LIBNAME for static libraries created in a middle of compilation. I
> went thru all its occurrences in the tree and I've found that all of them were
> for Windows system libraries (except one for AIX, but that's still a system
> library). System libraries syntax needs to be -l... and EXPAND_LIBNAME seems
> like exactly the place to do this. How would you prefer this to be fixed?

Actually, now that I take a deeper look at it, EXPAND_LIBNAME is only ever used with system libraries, so it should just be good to do what your patch does. However, I now realize that we should really update the comments above.
Comment 5 Jacek Caban 2011-04-26 03:18:41 PDT
Created attachment 528288 [details] [diff] [review]
fix v1.1

A patch with comment changes
Comment 6 Mike Hommey [:glandium] 2011-04-26 04:09:46 PDT
Comment on attachment 528288 [details] [diff] [review]
fix v1.1

Review of attachment 528288 [details] [diff] [review]:

r=glandium with the following nits applied

::: config/rules.mk
@@ +90,5 @@
 VPATH += $(LIBXUL_SDK)/lib
 endif
 
 # EXPAND_LIBNAME - $(call EXPAND_LIBNAME,foo)
+# expands to foo.lib or -lfoo, depending on linker arguments syntax

Replace lib with $(LIB_SUFFIX) and add "should only be used for system libraries"

@@ +96,2 @@
 # EXPAND_LIBNAME_PATH - $(call EXPAND_LIBNAME_PATH,foo,dir)
+# expands to dir/foo.lib

Replace lib with $(LIB_SUFFIX)

@@ +99,2 @@
 # EXPAND_MOZLIBNAME - $(call EXPAND_MOZLIBNAME,foo)
+# expands to $(DIST)/lib/foo.lib

likewise
Comment 7 Mike Hommey [:glandium] 2011-04-26 23:43:27 PDT
http://hg.mozilla.org/mozilla-central/rev/401962aff99d

Note You need to log in before you can comment on or make changes to this bug.