Closed Bug 507807 Opened 10 years ago Closed 10 years ago

Clean up linking on OS/2

Categories

(Firefox Build System :: General, defect)

x86
OS/2
defect
Not set

Tracking

(status1.9.2 beta1-fixed, status1.9.1 .3-fixed)

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta1-fixed
status1.9.1 --- .3-fixed

People

(Reporter: wuno, Assigned: wuno)

Details

Attachments

(4 files, 2 obsolete files)

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
Attached patch remove seamonkey.def (obsolete) — Splinter Review
Assignee: nobody → wuno
Status: NEW → ASSIGNED
Attachment #392085 - Flags: review?(mozilla)
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.
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.
Product: SeaMonkey → Core
QA Contact: build-config → build-config
Hardware: Other → x86
Summary: OS/2 give the opportunity to build a statically linked seamonkey → Clean up linking on OS/2
Version: unspecified → Trunk
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.
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.
Attachment #392145 - Flags: review?(mozilla)
Attached patch patch for mozilla-1.9.1 branch (obsolete) — Splinter Review
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.
Attachment #392154 - Flags: review?(mozilla)
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.
Attachment #392085 - Attachment is obsolete: true
Attachment #392156 - Flags: review?(mozilla)
Attachment #392085 - Flags: review?(mozilla)
Attachment #392145 - Flags: review?(mozilla) → review+
Attachment #392154 - Flags: review?(mozilla) → review-
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 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.
Attachment #392156 - Flags: review?(mozilla) → review+
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 on attachment 392145 [details] [diff] [review]
trunk patch [checkin comment 11]

http://hg.mozilla.org/mozilla-central/rev/cae5076351fa
Attachment #392145 - Attachment description: trunk patch → trunk patch [checkin comment 11]
(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?
ok, now w/o touching js
Attachment #392154 - Attachment is obsolete: true
Attachment #392314 - Flags: review?(mozilla)
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.
Attachment #392314 - Flags: review?(mozilla)
Attachment #392314 - Flags: review+
Attachment #392314 - Flags: approval1.9.1.3?
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
Attachment #392314 - Flags: approval1.9.1.3? → approval1.9.1.3+
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Next time Id appreciate if sunbird-only@calendar.bugs was cc'd. Thanks for the patch though!
(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?
Attachment #392314 - Attachment description: new patch for mozilla-1.9.1 → new patch for mozilla-1.9.1 [checkin comment 16]
Attachment #392156 - Attachment description: patch for c-c → patch for c-c [checkin comment 16]
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.)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #396081 - Flags: review?(mozilla) → review+
Keywords: checkin-needed
Whiteboard: attachment 396081 into mozilla-central and mozilla-1.9.2
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
Attachment #396081 - Attachment description: clean plugins on trunk for real → clean plugins on trunk for real [Checkin: Comment 21]
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: attachment 396081 into mozilla-central and mozilla-1.9.2 → [c-n: attachment 396081 to m-1.9.2]
Target Milestone: --- → mozilla1.9.3a1
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
Attachment #396081 - Attachment description: clean plugins on trunk for real [Checkin: Comment 21] → clean plugins on trunk for real [Checkin: Comment 21 & 22]
Keywords: checkin-needed
Whiteboard: [c-n: attachment 396081 to m-1.9.2]
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.