Closed Bug 577011 Opened 10 years ago Closed 9 years ago

[OS/2] make _declspec unconditionally default symbol export

Categories

(Firefox Build System :: General, defect)

x86
OS/2
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wuno, Assigned: wuno)

References

Details

Attachments

(4 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (OS/2; Warp 4.5; en-US; rv:2.0b2pre) Gecko/20100705 Minefield/4.0b2pre
Build Identifier: 

We should remove the option to not use _declspec for symbol export.

Reproducible: Always
Depends on: 576606
Version: unspecified → Trunk
Attached patch patch for mozilla-central (obsolete) — Splinter Review
The build I'm using for this post has applied the attached patch. This patch goes on top of the patch for bug576606 (for the changes in rules.mk). Beside cleaning the non-declspec parts it removes also the extra rules for components. The question is whether we may leave the check for declspec in configure.in and modify this check to error out if the compiler is too old. Second question concerns the cairo header. I'm not sure whether this has to be upstreamed and if so if we can skip the declspec check there. Probably cairo itself will build still with gcc-3.2.2, does it?
Assignee: nobody → wuno
Status: NEW → ASSIGNED
Attachment #456090 - Flags: review?(daveryeo)
I think that configure should check if the compiler supports declspec and error out if not. This way if someone tries an unsupported compiler he'll be informed of the dependence on declspec quickly.
The Cairo header is a Mozilla header with no corresponding header in Cairo trunk. I'd guess that Cairo will build still build with older GCC's and perhaps OpenWatcom with minimal patching.
I'm a bit surprised that there is no zlib patch in the tree as the parts you're changing were added by Mozilla (us?)
What about the line Filter='emxexp -o' in the various configure.in's? Seems we shouldn't need it either if only using declpec. 
Also wondering about comm-central. I'll see if they have their manifests are sorted out yet, do a couple of test builds.
(In reply to comment #1)

> The question is whether we may leave the check for declspec in configure.in
> and modify this check to error out if the compiler is too old.

An error check is definitely needed, not just for old compilers but for new ones too.  Who knows what bugs will appear in some future version of gcc?
This patch is addressing the need for declspec support in the compiler. Tested that with gcc-3.2.2 and it's working. Then, I've substituted FILTER='emxexp' by 'true' like it was added in the currently used moz_os2_use_declspec section. FYI I have a patch for comm-central at hand. However, at the moment the seamonkey build breaks during compilation of DOM-
inspector.
Attachment #456090 - Attachment is obsolete: true
Attachment #456184 - Flags: review?(daveryeo)
Attachment #456090 - Flags: review?(daveryeo)
Comment on attachment 456184 [details] [diff] [review]
patch for mozilla-central v1

Looks good, only thing I question is whether configure should say 4.3.2 or newer or 3.3.5 or newer.
Since 4.3.2 (or 4.0.4?) is the oldest that doesn't need patches applied to Mozilla it should be fine.
Once bug#575740 and dependencies are resolved we'll be able to find out if SeaMonkey needs any patches. (Which I doubt)
Attachment #456184 - Flags: review?(daveryeo) → review+
(In reply to comment #3)

> An error check is definitely needed, not just for old compilers but for new
> ones too.  Who knows what bugs will appear in some future version of gcc?

We're pretty well at the end of the line for new versions of GCC besides bug fixes. Aout is currently depreciated and being dropped in the next version.
(In reply to comment #5)
> (From update of attachment 456184 [details] [diff] [review])
> Looks good, only thing I question is whether configure should say 4.3.2 or
> newer or 3.3.5 or newer.
I was myself in doubt if I should mention any version number, but then I thought well 3.3.5 has declspec but won't build the code and 4.3.2 was the first reliable version that should still work.
> Since 4.3.2 (or 4.0.4?) is the oldest that doesn't need patches applied to
> Mozilla it should be fine.
> Once bug#575740 and dependencies are resolved we'll be able to find out if
> SeaMonkey needs any patches.
Comm-Central needs only the changes in configure, autoconf.mk and rules.mk.
(In reply to comment #6)
> (In reply to comment #3)
> 
> We're pretty well at the end of the line for new versions of GCC besides bug
> fixes. Aout is currently depreciated and being dropped in the next version.
Maybe we should add a compiler version check? Has 4.5 already dropped Aout? Then we could limit the version from >= 4.3.2 to < 4.5
(In reply to comment #8)
> (In reply to comment #6)
> > (In reply to comment #3)
> > 
> > We're pretty well at the end of the line for new versions of GCC besides bug
> > fixes. Aout is currently depreciated and being dropped in the next version.
> Maybe we should add a compiler version check? Has 4.5 already dropped Aout?
> Then we could limit the version from >= 4.3.2 to < 4.5

The release notes (TODO?) just mention the next version, so I'm unsure whether 4.5 or 5.x. Anyways I don't think we have to worry about versions that won't be built for OS/2 anyways. Besides someone might get ambitious and adapt COFF for OS/2 (relatively simple actually for an ambitious good programmer, as ilink will link the mixed object files).
While I do agree that 4.3.2 is a good minimal requirement, I was unsure whether to hardcode it. Further thought says that anyone who can get 3.3.5 to compile Mozilla can hack configure.in to support 3.3.5
These are the few changes for comm-central, syncing configure.in, autoconf.mk.in and rules.mk.
Attachment #457012 - Flags: review?(daveryeo)
Attachment #457012 - Flags: review?(daveryeo) → review+
(In reply to comment #10)
> Created attachment 457012 [details] [diff] [review]
> sync comm-central build system
> 
> These are the few changes for comm-central, syncing configure.in,
> autoconf.mk.in and rules.mk.

Justin, do we need to get approval for the c-c patch?
Keywords: checkin-needed
Comment on attachment 457012 [details] [diff] [review]
sync comm-central build system

feedback-as-approval-flag.  c-c is closed due to TB atm, (though suite/ is open) but a+=me anyway, as a "NPOTB" checkin [since none of our boxen use os/2]
Attachment #457012 - Flags: feedback+
Comment on attachment 457012 [details] [diff] [review]
sync comm-central build system

Actually for posterity, I'm marking as reviewed, since it does match the m-c fix, is OS/2 only, and since I don't recall Dave being announced in any fora as ok for c-c build-config patches (even if OS/2 only). I am not asserting he is not ok, just mentioning. [If I am wrong, just point me at a doc somewhere] :-)
Attachment #457012 - Flags: review+
Whiteboard: m-c a=NPOTB all in ifdef'd OS/2 sections
refreshed the m-c patch for a little bitrot added a header with checkin-comment
Walters patch updated for bit-rot and headers added. As no code changes, assuming that Calleks r+ carries over.
(In reply to comment #15)
> assuming that Calleks r+ carries over.

It does.
Comment on attachment 462893 [details] [diff] [review]
patch for mozilla-central v1 updated for checkin

Asking for approval since the patch touches a bunch of cross-platform files. however very low risk for other platforms than OS/2, since all is in ifdef'd sections. The patch lowers also the burden for programmers from other platforms as they won't have to think about what is MOZ_OS2_USE_DECLSPEC or if it is needed to check for __declspec.
Attachment #462893 - Flags: approval2.0?
Keywords: checkin-needed
Whiteboard: m-c a=NPOTB all in ifdef'd OS/2 sections
Attachment #462893 - Flags: approval2.0? → approval2.0+
Keywords: checkin-needed
Whiteboard: att 462893, m-c, OS2 changes only, approved
http://hg.mozilla.org/mozilla-central/rev/18143cc0603b
Keywords: checkin-needed
Whiteboard: att 462893, m-c, OS2 changes only, approved
There's an extra "(".

e:/builds/moz2_slave/mozilla-central-win32-debug/build/memory/mozalloc/msvc_throw_wrapper.cpp(43) : fatal error C1012: unmatched parenthesis : missing ')'
make[5]: *** [msvc_throw_wrapper.obj] Error 2
make[4]: *** [libs] Error 2
make[3]: *** [libs_tier_base] Error 2
make[2]: *** [tier_base] Error 2
make[1]: *** [default] Error 2
make: *** [build] Error 2
(In reply to comment #20)
> fix the typo
(In reply to comment #21)
> another one
Thanks for catching this, Ginn
Keywords: checkin-needed
Whiteboard: comm-central att 471419
Checked in:
http://hg.mozilla.org/comm-central/rev/40c697cb9a36
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Whiteboard: comm-central att 471419
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.