fix stupid module name differences on windows

RESOLVED FIXED in mozilla1.9.3a1

Status

RESOLVED FIXED
9 years ago
9 months ago

People

(Reporter: ted, Assigned: Mitch)

Tracking

({fixed-seamonkey2.0})

Trunk
mozilla1.9.3a1
fixed-seamonkey2.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

For some reason some makefiles change their module names on Windows. I can only suppose these are ancient leftovers from something.

http://mxr.mozilla.org/mozilla-central/source/xpcom/proxy/public/Makefile.in#45
http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/Makefile.in#46
Mitch: interested in taking this bug? I left some notes in the package manifest file (all the XXX comments):
http://mxr.mozilla.org/mozilla-central/source/browser/installer/package-manifest.in
Assignee: ted.mielczarek → nobody
(Assignee)

Updated

9 years ago
Assignee: nobody → mitch_1_2
Status: NEW → ASSIGNED
(Assignee)

Comment 2

9 years ago
Created attachment 401173 [details] [diff] [review]
Patch v1.0
Attachment #401173 - Flags: review?(ted.mielczarek)
Comment on attachment 401173 [details] [diff] [review]
Patch v1.0

This looks pretty good, just a few things:
--- a/browser/app/Makefile.in
+ifneq (,$(filter WINNT,$(OS_ARCH)))
+PROGRAM = $(MOZ_APP_NAME)$(BIN_SUFFIX)
+else

If you only need to test for a single OS, it's cleaner (and easier to read) to write it:
ifeq ($(OS_ARCH),WINNT)

That being said, let's just ditch the USE_SHORT_LIBNAME block up above, and make this
ifneq (,$(filter WINNT WINCE OS2,$(OS_ARCH)))

--- a/browser/installer/removed-files.in
+components/@DLL_PREFIX@docshell_base@DLL_SUFFIX@
+components/docshell_base.xpt
+components/@DLL_PREFIX@proxyObjInst@DLL_SUFFIX@
+components/proxyObjInst.xpt
+components/@DLL_PREFIX@xpcom_thread@DLL_SUFFIX@
+components/xpcom_thread.xpt

You don't need any of these lines in here. AFAIK we've never shipped these xpt files by themselves, since Windows has always shipped a single linked xpt file. We've also never shipped a non-static or libxul build, so these components would never have been installed. (We do need the brwsrcmp.dll line, of course.)

+ifneq (,$(filter OS2 WINCE,$(OS_ARCH)))

Let's drop USE_SHORT_LIBNAME for WinCE, too. Only OS/2 requires it, so just make this:
ifeq (OS2,$(OS_ARCH))

You'll have to kill the USE_SHORT_LIBNAME bit for WinCE in configure as well.

--- a/xpcom/build/Makefile.in
-ifneq (,$(filter-out WINNT WINCE,$(OS_ARCH)))
+ifneq (,$(filter WINCE,$(OS_ARCH)))
 SHORT_LIBNAME	= xpcomcor
 endif

If you do that, you can just drop the if here.

--- a/xulrunner/app/Makefile.in
ifeq ($(USE_SHORT_LIBNAME), 1)
 PROGRAM = xulrunner$(BIN_SUFFIX)
 else
-ifeq ($(OS_ARCH), BeOS)
+ifneq (,$(filter BeOS WINCE WINNT,$(OS_ARCH)))
 PROGRAM = xulrunner$(BIN_SUFFIX)
 else
 PROGRAM = xulrunner-bin$(BIN_SUFFIX)
 endif
 endif

Same thing I mentioned above, just ditch the USE_SHORT_LIBNAME block and have an explicit list of OSes in the filter.

The rest of this looks good, just needs these fixes.
Attachment #401173 - Flags: review?(ted.mielczarek) → review-
This will probably have some comm-central fallout. Might also be some Fennec fallout since we're changing the SHORT_LIBNAME bits. Neither of them should be very serious.
(Assignee)

Comment 5

9 years ago
Created attachment 401421 [details] [diff] [review]
Patch v1.1
[Checkin: Comment 11]
Attachment #401173 - Attachment is obsolete: true
Attachment #401421 - Flags: review?(ted.mielczarek)
Comment on attachment 401421 [details] [diff] [review]
Patch v1.1
[Checkin: Comment 11]

Looks good, thanks!
Attachment #401421 - Flags: review?(ted.mielczarek) → review+
Thunderbird and SeaMonkey will need fixup to build against m-c:
http://mxr.mozilla.org/comm-central/source/suite/app/Makefile.in#68
http://mxr.mozilla.org/comm-central/source/mail/app/Makefile.in#89

Should be fine to just change those checks to OS-specific checks like Mitch's patch does, that will work fine on any branch.

I'm not sure if this will actually break anything in Fennec, after looking at it again. They don't have a packaging manifest, and they don't build an app binary (they just use the XR stub).
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Created attachment 401893 [details] [diff] [review]
simplest c-c patch

Not that I really expect us to be building on wince, but it's simpler for future searching to have things exactly copy-pasted when we can.
Attachment #401893 - Flags: review?(bugzilla)
Attachment #401893 - Attachment is obsolete: true
Attachment #401893 - Flags: review?(bugzilla)
Comment on attachment 401893 [details] [diff] [review]
simplest c-c patch

Where by "simplest" I mean, "d'oh, I do remember a little thing called packages-static, now that you mention it."
Created attachment 402383 [details] [diff] [review]
more c-c patch
[Checkin: See comment 13]

Some extras for Tb, to get us a clean package-compare, and just fallout from this for SM since for the other stuff it needs both packages files updated and needs decisions about some of the currently unpackaged stuff.
Attachment #402383 - Flags: review?(bugzilla)
http://hg.mozilla.org/mozilla-central/rev/614e3c4ba3c1
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Comment on attachment 402383 [details] [diff] [review]
more c-c patch
[Checkin: See comment 13]

>+#ifdef MOZILLA_1_9_1_BRANCH
> bin\res\charsetData.properties
> bin\res\charsetalias.properties
>+bin\res\wincharset.properties
>+#endif

These need adding in an ifndef to the removed-files.in

>diff --git a/suite/installer/windows/packages b/suite/installer/windows/packages

suite has a unix file as well. Is that affected? (if so I think we can do that in a separate patch and fix at least some of these bustages now).

r+a=Standard8
Attachment #402383 - Flags: review?(bugzilla) → review+
Both of suite's manifests need more, yeah, but I intentionally only fixed suite's issues that were fallout from this bug, so only Windows. Added the removed-files, and jsctypes.xpt for Tb-trunk, and pushed http://hg.mozilla.org/comm-central/rev/018708da1fbd
Depends on: 519068
Keywords: fixed-seamonkey2.0

Updated

9 years ago
Depends on: 520418
Depends on: 508421
Attachment #402383 - Attachment description: more c-c patch → more c-c patch [Checkin: See comment 13]
Attachment #401421 - Attachment description: Patch v1.1 → Patch v1.1 [Checkin: Comment 11]
Created attachment 405502 [details] [diff] [review]
(Cv1-SM) Support m-1.9.2 too, Improve sort order
[Checkin: See comment 21+23]
Attachment #405502 - Flags: review?(kairo)
Depends on: 518124
(In reply to comment #13)
> Added the [...] jsctypes.xpt for Tb-trunk, and pushed
> http://hg.mozilla.org/comm-central/rev/018708da1fbd

Fwiw, jsctypes.xpt doesn't exist (anymore) in m-1.9.1/m-1.9.2/m-c: bug 518721 removed it (since then).
You may want to undo that change to TB.
Depends on: 469443
I wonder why the removed .xpt are not added to removed-files.in.
If that's not needed (anymore) then could the older .xpt be unlisted from that file?

(NB: Same for bug 521428.)
(In reply to comment #16)
> I wonder why the removed .xpt are not added to removed-files.in.

In m-1.9.2/m-1.9.1, I mean.
(In reply to comment #16)
> I wonder why the removed .xpt are not added to removed-files.in.
> If that's not needed (anymore) then could the older .xpt be unlisted from that
> file?
> 
> (NB: Same for bug 521428.)

Because we have shipped a single browser.xpt on Windows and Linux for as long as I can remember. OS X didn't get that treatment until bug 510309 landed recently.
Also, could you take any additional work to another bug? This bug is fixed.
(In reply to comment #18)
> we have shipped a single browser.xpt on Windows and Linux for as long
> as I can remember.

Thanks, that's just the hint/confirmation I wanted :-)

Updated

9 years ago
Attachment #405502 - Flags: review?(kairo) → review+
Comment on attachment 405502 [details] [diff] [review]
(Cv1-SM) Support m-1.9.2 too, Improve sort order
[Checkin: See comment 21+23]


http://hg.mozilla.org/comm-central/rev/70007a762cab
Attachment #405502 - Attachment description: (Cv1-SM) Support m-1.9.2 too, Improve sort order → (Cv1-SM) Support m-1.9.2 too, Improve sort order [Checkin: Comment 21]

Comment 22

9 years ago
(In reply to comment #21)
> http://hg.mozilla.org/comm-central/rev/70007a762cab

...and one more un-approved checkin. I hope those won't happen more often. Even if I do the review, that doesn't mean it's auto-approved.
Comment on attachment 405502 [details] [diff] [review]
(Cv1-SM) Support m-1.9.2 too, Improve sort order
[Checkin: See comment 21+23]


Checked in without moving urlformatter.xpt, which doesn't need to.
Attachment #405502 - Attachment description: (Cv1-SM) Support m-1.9.2 too, Improve sort order [Checkin: Comment 21] → (Cv1-SM) Support m-1.9.2 too, Improve sort order [Checkin: See comment 21+23]
Comment on attachment 405502 [details] [diff] [review]
(Cv1-SM) Support m-1.9.2 too, Improve sort order
[Checkin: See comment 21+23]


(See bug 521428.)
Requesting approval (by comment) after the fact:
minus it if you want me to back this out.

Comment 25

9 years ago
(In reply to comment #24)
> Requesting approval (by comment) after the fact:
> minus it if you want me to back this out.

It's alright, a=me, just remember it next time ;-)
Depends on: 524022
(In reply to comment #15)
> Fwiw, jsctypes.xpt doesn't exist (anymore) in m-1.9.1/m-1.9.2/m-c: bug 518721
> removed it (since then).
> You may want to undo that change to TB.

I'll do that in bug 823820.
Depends on: 523820
Blocks: 519068
No longer depends on: 519068

Updated

9 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.