Closed
Bug 514665
Opened 16 years ago
Closed 16 years ago
fix stupid module name differences on windows
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9.3a1
People
(Reporter: ted, Assigned: Mitch)
References
Details
(Keywords: fixed-seamonkey2.0)
Attachments
(3 files, 2 obsolete files)
25.83 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
8.60 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
8.69 KB,
patch
|
kairo
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•16 years ago
|
||
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•16 years ago
|
Assignee: nobody → mitch_1_2
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•16 years ago
|
||
Attachment #401173 -
Flags: review?(ted.mielczarek)
Reporter | ||
Comment 3•16 years ago
|
||
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-
Reporter | ||
Comment 4•16 years ago
|
||
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•16 years ago
|
||
Attachment #401173 -
Attachment is obsolete: true
Attachment #401421 -
Flags: review?(ted.mielczarek)
Reporter | ||
Comment 6•16 years ago
|
||
Comment on attachment 401421 [details] [diff] [review]
Patch v1.1
[Checkin: Comment 11]
Looks good, thanks!
Attachment #401421 -
Flags: review?(ted.mielczarek) → review+
Reporter | ||
Comment 7•16 years ago
|
||
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•16 years ago
|
Keywords: checkin-needed
Comment 8•16 years ago
|
||
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)
Updated•16 years ago
|
Attachment #401893 -
Attachment is obsolete: true
Attachment #401893 -
Flags: review?(bugzilla)
Comment 9•16 years ago
|
||
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."
Comment 10•16 years ago
|
||
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)
Comment 11•16 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Comment 12•16 years ago
|
||
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+
Comment 13•16 years ago
|
||
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
Updated•16 years ago
|
Keywords: fixed-seamonkey2.0
Updated•16 years ago
|
Attachment #402383 -
Attachment description: more c-c patch → more c-c patch
[Checkin: See comment 13]
Updated•16 years ago
|
Attachment #401421 -
Attachment description: Patch v1.1 → Patch v1.1
[Checkin: Comment 11]
Comment 14•16 years ago
|
||
Attachment #405502 -
Flags: review?(kairo)
Comment 15•16 years ago
|
||
(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.
Comment 16•16 years ago
|
||
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.)
Comment 17•16 years ago
|
||
(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.
Reporter | ||
Comment 18•16 years ago
|
||
(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.
Reporter | ||
Comment 19•16 years ago
|
||
Also, could you take any additional work to another bug? This bug is fixed.
Comment 20•16 years ago
|
||
(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•16 years ago
|
Attachment #405502 -
Flags: review?(kairo) → review+
Comment 21•16 years ago
|
||
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•16 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 23•16 years ago
|
||
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 24•16 years ago
|
||
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•16 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 ;-)
Comment 26•15 years ago
|
||
(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
Updated•15 years ago
|
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•