If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Clean up linking on OS/2

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
Build Config
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Walter Meinl, Assigned: Walter Meinl)

Tracking

Trunk
mozilla1.9.3a1
x86
OS/2
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

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

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

8 years ago
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
(Assignee)

Comment 1

8 years ago
Created attachment 392085 [details] [diff] [review]
remove seamonkey.def
Assignee: nobody → wuno
Status: NEW → ASSIGNED
Attachment #392085 - Flags: review?(mozilla)
(Assignee)

Comment 2

8 years ago
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

8 years ago
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.
Component: Build Config → Build Config
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
(Assignee)

Comment 4

8 years ago
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.
(Assignee)

Comment 5

8 years ago
Created attachment 392145 [details] [diff] [review]
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.
Attachment #392145 - Flags: review?(mozilla)
(Assignee)

Comment 6

8 years ago
Created attachment 392154 [details] [diff] [review]
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.
Attachment #392154 - Flags: review?(mozilla)
(Assignee)

Comment 7

8 years ago
Created attachment 392156 [details] [diff] [review]
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.
Attachment #392085 - Attachment is obsolete: true
Attachment #392156 - Flags: review?(mozilla)
Attachment #392085 - Flags: review?(mozilla)

Updated

8 years ago
Attachment #392145 - Flags: review?(mozilla) → review+

Updated

8 years ago
Attachment #392154 - Flags: review?(mozilla) → review-

Comment 8

8 years ago
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

8 years ago
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.

Updated

8 years ago
Attachment #392156 - Flags: review?(mozilla) → review+

Comment 10

8 years ago
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

8 years ago
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]
(Assignee)

Comment 12

8 years ago
(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?
(Assignee)

Comment 13

8 years ago
Created attachment 392314 [details] [diff] [review]
new patch for mozilla-1.9.1 [checkin comment 16]

ok, now w/o touching js
Attachment #392154 - Attachment is obsolete: true
Attachment #392314 - Flags: review?(mozilla)

Comment 14

8 years ago
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+

Comment 16

8 years ago
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
Last Resolved: 8 years ago
status1.9.1: --- → .3-fixed
Resolution: --- → FIXED
Next time Id appreciate if sunbird-only@calendar.bugs was cc'd. Thanks for the patch though!
(Assignee)

Comment 18

8 years ago
(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?

Updated

8 years ago
Attachment #392314 - Attachment description: new patch for mozilla-1.9.1 → new patch for mozilla-1.9.1 [checkin comment 16]

Updated

8 years ago
Attachment #392156 - Attachment description: patch for c-c → patch for c-c [checkin comment 16]

Comment 19

8 years ago
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 → ---
(Assignee)

Comment 20

8 years ago
Created attachment 396081 [details] [diff] [review]
clean plugins on trunk for real
[Checkin: Comment 21 & 22]
Attachment #396081 - Flags: review?(mozilla)

Updated

8 years ago
Attachment #396081 - Flags: review?(mozilla) → review+

Updated

8 years ago
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
Last Resolved: 8 years ago8 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]
status1.9.2: --- → beta1-fixed
Keywords: checkin-needed
Whiteboard: [c-n: attachment 396081 to m-1.9.2]
You need to log in before you can comment on or make changes to this bug.