507807 - Clean up linking on OS/2

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Walter Meinl]

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9.1.3pre) Gecko/20090801 SeaMonkey/2.0b2pre
Build Identifier: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9.1.3pre) Gecko/20090801 SeaMonkey/2.0b2pre

Configure arguments
--enable-application=suite --enable-optimize --disable-debug --disable-tests --enable-static --disable-shared --enable-application=suite --enable-optimize --disable-debug --disable-tests --enable-static --disable-shared --enable-application=../suite --disable-official-branding --with-branding=../suite/branding/nightly --disable-debug --enable-optimize --cache-file=.././config.cache --srcdir=E:/usr/src/hg/comm-central/mozilla 

It's possible I linked it statically and it runs 
1.08.09  12.44       9.681.187         124  seamonkey.exe is the size of the exe when built with gcc-4.4.0 (04-26)

We have just to remove the EXE_DEF_FILE = seamonkey.def from the suite/app/Makefile.in (this does not exist and the build breaks during linkage complaining that it can't be found)
patch comes in a second 

Reproducible: Always

Comment 1

[Walter Meinl]

Attachment: remove seamonkey.def

Comment 2

[Walter Meinl]

Searched a bit in mxr(1.8.0-1.9.2) about the various def files for the final static link. They are generated by the Makefile in the app directory:
265 ifeq ($(OS_ARCH),OS2)
266 ifdef BUILD_STATIC_LIBS
267 $(EXE_DEF_FILE):
268         rm -f $@
269         @echo NAME mozilla >$(EXE_DEF_FILE)
270         @echo IMPORTS >>$(EXE_DEF_FILE)
271         @echo   WinQueryProperty                = PMMERGE.5450 >>$(EXE_DEF_FILE)
272         @echo   WinRemoveProperty               = PMMERGE.5451 >>$(EXE_DEF_FILE)
273         @echo   WinSetProperty                  = PMMERGE.5452 >>$(EXE_DEF_FILE)
274 
275 LDFLAGS += -Zlinker /NOE
276 endif
277 endif
Interestingly, this is still true for browser (which cannot be build statically anymore). Sunbird's Makefile has it as well (funny, its also designated browser.def). Thunderbird's Makefile, the only app we build always statically never had a reference to a .def file as well as the lines to generate it (like with the icons, TB appears to be a stepchild ;-). Mozilla's Makefile contained both, the reference to mozilla.def and the lines to generate it. During the transition to SeaMonkey mozilla.def was renamed to seamonkey.def, but the lines to generate it got lost.
So, either we remove the line as suggested comment #1 or we add sth like the lines from the browser's Makefile back. However, it might not be necessary, since TB never had it and runs now statically built since several years.

Comment 3

[Peter Weilbacher]

While I'm sure that the patch is fine, I think we should take this chance and make this part of a larger .def-file cleanup. I stumbled on some extradefs.os2 files (like in widget/src/os2), and I don't think they are necessary any more. The imports of the Win* functions are likely a left-over from the VACPP days, and should be linked correctly with GCC without having them in the .def file. But perhaps you can verify that.

Comment 4

[Walter Meinl]

I searched mxr c-c with "def_file". We have two extradefs.os2 files in widget/src/os2 and in modules/plugin/base/src (I'm not sure whether flash and java work when we remove the latter).
sqlite.def (you've added that famous sed exchange to get all exports beginning with an underscore).
lcms.def will be cleaned, when lcms is removed.
Then we have os2extra.def in nsprpub/pr/src. For another reason I removed those lines referring to vacpp in that file, rebuilt nspr and copied the dlls over to a working seamonkey. After that flash and java crashed immediatly - so, we have to be cautious in that case.
Beside this we have to look in rules.mk (also of c-sdk) whether they contain obsolete lines that are taken up in produced .def files.
Finally, I found the ones already mentioned referred and/or generated by browser/app/Makefile suite/app/Makefile and calendar/sunbird/app/Makefile

I'd suggest that I try to remove the apps and extradefs.os2, let sqlite and lcms alone. If we want to do something with nspr and the rules.mk files we should do it in other bugs.

Comment 5

[Walter Meinl]

Attachment: trunk patch [checkin comment 11]

On trunk there was indeed only the extradefs.os2 in widget. I didn't build the trunk version yet, but I built seamonkey with an equivalent patch for mozilla-1.9.1 and did not observe crashes with java or flash. The part for browser cannot be tested because of the lack of building static, IIRC.

Comment 6

[Walter Meinl]

Attachment: patch for mozilla-1.9.1 branch

This includes in addition to the trunk patch the removal of modules/plugin/base/src/extradefs.os2 and js/src/jsOS240.def (couldn't find when this was removed from trunk, obviously some time after branching). Note, it will apply cleanly after branch checkin for bug507251.

Comment 7

[Walter Meinl]

Attachment: patch for c-c [checkin comment 16]

The patch for c-c comprises the change in the seamonkey app Makefile and what needs to be done for sunbird. For SB I included removal of app.ico and some bitmap files for official-branding that apparently were there for a splash screen.

Comment 8

[Peter Weilbacher]

Comment on attachment 392154 [details] [diff] [review]
patch for mozilla-1.9.1 branch

This is good, except that I don't really want to mess around with files in /js/ on branch, even if it is an obsolete OS/2-only file. (It was very recently removed on trunk, see bug 488824 comment 6 and following.)

Comment 9

[Peter Weilbacher]

Comment on attachment 392156 [details] [diff] [review]
patch for c-c [checkin comment 16]

This is fine, although now that I see it in this patch again, I am confused why it was necessary to pass /NOE to the linker (until now?). Following the ilink doc this stands for /NOEXTDICTIONARY and would be used if we overwrote a symbol.

The Sunbird BMP files were apparently thought for the installer which on OS/2 we never got. If we get one at some point, it will be WarpIn-based and not use the files. So yes, removing those lines makes sense.

Comment 10

[Peter Weilbacher]

Comment on attachment 392156 [details] [diff] [review]
patch for c-c [checkin comment 16]

OK, so I had a little bit of fun doing CVS archeology. /NOE was added to browser/app/Makefile.in in bug 187485 (attachment 110526 [details] [diff] [review]) to fix static builds of Phoenix. That was copied from a GCC fix of xpfe/bootstrap/Makefile.in (bug 188249, attachment 117939 [details] [diff] [review]) which in turn was moved from all OS/2 builds to only static builds as a bustage fix in this checkin: <http://bonsai.mozilla.org/cvsquery.cgi?date=explicit&mindate=2003-01-17+12%3A50&maxdate=2003-01-17+13%3A00&cvsroot=%2Fcvsroot>. Originally, /NOE was added in bug 179508; the checkin (http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/xpfe/bootstrap/Attic&command=DIFF_FRAMESET&file=Makefile.in&rev2=1.207&rev1=1.206) added it inside the BUILD_STATIC_LIBS block, although the patch on the bug (attachment 105885 [details] [diff] [review]) was reviewed to have it outside, i.e. or all OS/2 builds. I hope I got all that right. ;-)

In any case, all this was just to fix build breaks, so if it works without /NOE now, we can remove it.

Comment 11

[Peter Weilbacher]

Comment on attachment 392145 [details] [diff] [review]
trunk patch [checkin comment 11]

http://hg.mozilla.org/mozilla-central/rev/cae5076351fa

Comment 12

[Walter Meinl]

(In reply to comment #10)
> (From update of attachment 392156 [details] [diff] [review])
> OK, so I had a little bit of fun doing CVS archeology. 
;-)
> In any case, all this was just to fix build breaks, so if it works without /NOE
> now, we can remove it.
...and thunderbird never had it for several years now...
I built SM and SB statically with the patches applied. Though testing was very limited I didn't observe a crash or something unexpected (though I don't know what could happen because of a missing linker directive - I'd guess either they start or they don't because of linking problems, anything else what could one expect?

Comment 13

[Walter Meinl]

Attachment: new patch for mozilla-1.9.1 [checkin comment 16]

ok, now w/o touching js

Comment 14

[Peter Weilbacher]

Comment on attachment 392314 [details] [diff] [review]
new patch for mozilla-1.9.1 [checkin comment 16]

As this patch touches two cross-platform files, I'm asking for approval to get this onto 1.9.1.

Comment 15

[Samuel Sidler (old account; do not CC)]

Comment on attachment 392314 [details] [diff] [review]
new patch for mozilla-1.9.1 [checkin comment 16]

Approved for 1.9.1.3. a=NPOTB, aka ss

Comment 16

[Peter Weilbacher]

Pushed the relevant patches to comm-central and mozilla-1.9.1:
http://hg.mozilla.org/comm-central/rev/24ca0f65ebe0
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2d970f368acf

Comment 17

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Next time Id appreciate if [e-mail] was cc'd. Thanks for the patch though!

Comment 18

[Walter Meinl]

(In reply to comment #5)
> Created an attachment (id=392145) [details]
> trunk patch
> 
> On trunk there was indeed only the extradefs.os2 in widget. I didn't build the
> trunk version yet, but I built seamonkey with an equivalent patch for
> mozilla-1.9.1 and did not observe crashes with java or flash. The part for
> browser cannot be tested because of the lack of building static, IIRC.

Hrm, I've obviously overlooked that there's still extradefs.os2 in modules/plugin/base (and also it's still referenced by the Makefile.in). How could this happen?
Peter, shall I open a new bug or attach a follow-up patch?

Comment 19

[Peter Weilbacher]

Yes, add it here, please, so that we can at least remove it from trunk and 1.9.2. (Really weird, I had also searched for extradefs and ADD_TO_DEF_FILE and hadn't found anything else.)

Comment 20

[Walter Meinl]

Attachment: clean plugins on trunk for real [Checkin: Comment 21 & 22]

Comment 21

[Serge Gautherie (:sgautherie)]

Comment on attachment 396081 [details] [diff] [review]
clean plugins on trunk for real
[Checkin: Comment 21 & 22]


http://hg.mozilla.org/mozilla-central/rev/c3eec5d6a63a

Comment 22

[Serge Gautherie (:sgautherie)]

Comment on attachment 396081 [details] [diff] [review]
clean plugins on trunk for real
[Checkin: Comment 21 & 22]


http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f7277d57f597