Closed
Bug 969757
Opened 10 years ago
Closed 10 years ago
Remove the dead code in our tree which pretends to support OS/2
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla30
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
Details
Attachments
(1 file, 2 obsolete files)
1.41 MB,
patch
|
gps
:
review+
jorendorff
:
review+
benjamin
:
review+
benjamin
:
superreview+
|
Details | Diff | Splinter Review |
I have a patch for this, trying to remove as much code as I can... Stay tuned. CCing a bunch of people who will review my patch!
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Comment on attachment 8372760 [details] [diff] [review] Remove support for OS/2 Review of attachment 8372760 [details] [diff] [review]: ----------------------------------------------------------------- Apologies for the size of the patch, but this was hard to split up in any meaningful way. Requesting review from gps on the build system parts, jorendorf for the js parts, roc for gfx/layout/widget and bsmedberg for the rest. Also, requesting superreview from bsmedberg.
Attachment #8372760 -
Flags: superreview?(benjamin)
Attachment #8372760 -
Flags: review?(roc)
Attachment #8372760 -
Flags: review?(mcmanus)
Attachment #8372760 -
Flags: review?(jorendorff)
Attachment #8372760 -
Flags: review?(gps)
Attachment #8372760 -
Flags: review?(benjamin)
Comment 3•10 years ago
|
||
I just realized when never made an official determination on any mailing list. I just replied to the dev.platform post from August stating that I believe we should remove OS/2 from the *build system*. This patch goes beyond that proposal and removes OS/2 from the *entire* tree. The OS/2 porters may object to this wider scope. I guess we'll wait and see.
Updated•10 years ago
|
OS: Mac OS X → OS/2
Summary: Remove support for OS2 → Remove support for OS/2
Comment on attachment 8372760 [details] [diff] [review] Remove support for OS/2 Review of attachment 8372760 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the widget, layout and gfx parts, except for the stuff under gfx/cairo. If you want to do that, please submit a separate patch because we should be tracking the changes we make in tree cairo vs upstream. And Jeff M should probably review it.
Attachment #8372760 -
Flags: review?(roc) → review+
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #3) > I just realized when never made an official determination on any mailing > list. I just replied to the dev.platform post from August stating that I > believe we should remove OS/2 from the *build system*. OK, thanks. > This patch goes beyond that proposal and removes OS/2 from the *entire* > tree. The OS/2 porters may object to this wider scope. I guess we'll wait > and see. Well, the reason I did the rest of the work is: 1. It makes very little sense for us to have directories for example that are outside of the build because the build system doesn't know about it (such as widget/os2), or code which is dead because it's hidden behind the XP_OS2 macro which is removed from the build system in the build system part of the patch. 2. The mozilla-os2 project on github doesn't seem to be interested in upstreaming any of their work for the OS2 port to us, not just the build system parts. For example, here is what they did for the XPCOM TimeStamp implementation back when we removed support for OS2 in that code (which is I think the first time when we broke the builds on that platform): <https://github.com/bitwiseworks/mozilla-os2/commit/527fc2cb87f7f8c0b7ebdd166e2f5fe780351d04>. I think the OS2 port can take the reverse of my patch if they would like to keep updated with mozilla-central, but looking at their github repo, they have not tried to merge anything from us in since Firefox 17 ESR, so I don't see this patch adversely affecting them. CCing Dmitriy who I believe is the current maintainer here.
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8372817 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 7•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=004e382a5aac Note that the TestPoisonArea failure on non-Windows platforms is a result of a typo in the patch which I have fixed locally.
Updated•10 years ago
|
Attachment #8372760 -
Flags: review?(mcmanus) → review+
Comment 8•10 years ago
|
||
Comment on attachment 8372817 [details] [diff] [review] Part 0: Remove support for OS2 from cairo and pixman; r=jrmuizel Review of attachment 8372817 [details] [diff] [review]: ----------------------------------------------------------------- No need to remove the upstream parts.
Attachment #8372817 -
Flags: review?(jmuizelaar) → review-
Comment 9•10 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #8) > No need to remove the upstream parts. More generally, no need to remove anything from third-party code. Including config.guess/config.sub.
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #9) > (In reply to Jeff Muizelaar [:jrmuizel] from comment #8) > > No need to remove the upstream parts. OK, but I'm still going to remove the build system parts as they are not upstream code. That will just leave some dead code in cairo. > More generally, no need to remove anything from third-party code. Including > config.guess/config.sub. I didn't know they're third-party code. Will revert those hunks.
Assignee | ||
Updated•10 years ago
|
Attachment #8372817 -
Attachment is obsolete: true
Assignee | ||
Comment 11•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8372760 -
Attachment is obsolete: true
Attachment #8372760 -
Flags: superreview?(benjamin)
Attachment #8372760 -
Flags: review?(jorendorff)
Attachment #8372760 -
Flags: review?(gps)
Attachment #8372760 -
Flags: review?(benjamin)
Assignee | ||
Updated•10 years ago
|
Attachment #8372860 -
Flags: superreview?(benjamin)
Attachment #8372860 -
Flags: review?(jorendorff)
Attachment #8372860 -
Flags: review?(gps)
Attachment #8372860 -
Flags: review?(benjamin)
Comment 12•10 years ago
|
||
In general, I think it's okay to declare that there are platforms for which we would not even accept community maintenance status. OS/2 pretty clearly fits that bill--it's a long-dead OS and it's different enough from other OSes we support that it needs a fair amount of special-casing.
Assignee | ||
Comment 13•10 years ago
|
||
(In reply to comment #12) > In general, I think it's okay to declare that there are platforms for which we > would not even accept community maintenance status. OS/2 pretty clearly fits > that bill--it's a long-dead OS and it's different enough from other OSes we > support that it needs a fair amount of special-casing. As I said in comment 5, all this patch is doing is just removing code that is already dead. Just to be clear, we have not supported building mozilla-central on OS/2 for at least two years now. Please don't let the presence of this amount of dead code mislead you on the status quo. Also, I'd like to ask reviewers to treat my review request here as a pure code review request. The superreview request here is intended to ask permission on landing this patch, and that is Benjamin's call (or whoever he decides to redirect the sr? to). Thanks!
Assignee | ||
Comment 14•10 years ago
|
||
Rephrasing the summary to make what I'm doing here more clear.
Summary: Remove support for OS/2 → Remove the dead code in our tree which pretends to support OS/2
Comment 15•10 years ago
|
||
Comment on attachment 8372860 [details] [diff] [review] Remove support for OS/2 Review of attachment 8372860 [details] [diff] [review]: ----------------------------------------------------------------- Are the OS/2 people at least aware that this is coming? r=me on the JS bits.
Attachment #8372860 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 16•10 years ago
|
||
(In reply to Jason Orendorff [:jorendorff] from comment #15) > Are the OS/2 people at least aware that this is coming? Yes, this is being discussed on dev.platform.
Comment 17•10 years ago
|
||
Comment on attachment 8372860 [details] [diff] [review] Remove support for OS/2 I agree with Gavin in the newsgroup thread: OS/2 is not a porting target which we as a project want to spend effort on, so even if patches were written to re-add support, I don't think we'd take them.
Attachment #8372860 -
Flags: superreview?(benjamin)
Attachment #8372860 -
Flags: superreview+
Attachment #8372860 -
Flags: review?(benjamin)
Attachment #8372860 -
Flags: review+
Comment 18•10 years ago
|
||
Comment on attachment 8372860 [details] [diff] [review] Remove support for OS/2 Review of attachment 8372860 [details] [diff] [review]: ----------------------------------------------------------------- r=me on build bits with 2 hard blockers: gfx/cairo/libpixman/src/Makefile.in and memory/mozalloc/moz.build contained code that did "if != os2" but it appears the body of the if was removed. Comments about $(filter) and $(filter-out) are style nits. Not a big deal if you don't fix them - we'll fix them eventually as part of moz.build migration. Nuke this code from orbit - it's the only way to be sure. ::: browser/app/Makefile.in @@ +93,5 @@ > LDFLAGS += /HEAP:0x40000 > endif #} > endif #} > > +ifneq (,$(filter-out WINNT,$(OS_ARCH))) Now that you have a single element, you don't need $(filter-out). You can replace with: ifneq ($(OS_ARCH),WINNT) I think replacing the above endif with an else will also do what you want. ::: build/binary-location.mk @@ +3,5 @@ > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > # finds the location of the browser and puts it in the variable $(browser_path) > > +ifneq (,$(filter WINNT,$(OS_ARCH))) Don't need $(filter). ::: config/Makefile.in @@ +122,5 @@ > check-jar-mn:: > $(MAKE) -C tests/src-simple check-jar > $(MAKE) -C tests/src-simple check-flat > $(MAKE) -C tests/src-simple check-flat USE_EXTENSION_MANIFEST=1 > +ifneq (,$(filter-out WINNT,$(OS_ARCH))) Don't need $(filter-out). ::: config/makefiles/target_binaries.mk @@ +66,5 @@ > SHARED_LIBRARY_DEST ?= $(FINAL_TARGET) > SHARED_LIBRARY_TARGET = binaries libs > INSTALL_TARGETS += SHARED_LIBRARY > > +ifneq (,$(filter WINNT,$(OS_ARCH))) Kill $(filter). ::: config/nspr/Makefile.in @@ +35,5 @@ > libs:: > $(MAKE) -C $(DEPTH)/nsprpub install prefix=$(ABS_DIST)/sdk exec_prefix=$(ABS_DIST)/sdk bindir=$(ABS_DIST)/sdk/dummy includedir=$(ABS_DIST)/include/nspr libdir=$(ABS_DIST)/sdk/lib datadir=$(ABS_DIST)/sdk/dummy DESTDIR= $(EXTRA_MAKE_FLAGS) > $(INSTALL) $(DEPTH)/nsprpub/config/nspr-config $(DIST)/sdk/bin > $(RM) -rf $(DIST)/sdk/dummy > +ifneq (,$(filter WINNT,$(OS_ARCH))) # { Kill $(filter). ::: config/rules.mk @@ +186,5 @@ > ifdef LIB_IS_C_ONLY > MKSHLIB = $(MKCSHLIB) > endif > > +ifneq (,$(filter WINNT,$(OS_ARCH))) Kill $(filter). @@ +1058,5 @@ > ifndef .PYMAKE > MAKEFLAGS += -r > endif > > +ifneq (,$(filter WINNT,$(OS_ARCH))) Kill $(filter). ::: gfx/cairo/libpixman/src/Makefile.in @@ -46,5 @@ > SSE2_CFLAGS=-msse -msse2 -Winline > endif > -ifneq ($(MOZ_WIDGET_TOOLKIT),os2) > -MMX_CFLAGS+=--param inline-unit-growth=10000 --param large-function-growth=10000 > -endif You shouldn't have removed the body. ::: js/src/Makefile.in @@ +44,5 @@ > endif # JS_HAS_CTYPES > > DASH_R = -r > > +ifneq (,$(filter WINNT,$(OS_ARCH))) Kill $(filter). ::: memory/mozalloc/Makefile.in @@ +9,5 @@ > endif > > DIST_INSTALL = 1 > > +ifneq (,$(filter WINNT,$(OS_ARCH))) Kill $(filter). ::: memory/mozalloc/moz.build @@ -46,5 @@ > FORCE_SHARED_LIB = True > - > -if CONFIG['OS_ARCH'] != 'OS2': > - # The strndup declaration in string.h is in an ifdef __USE_GNU section > - DEFINES['_GNU_SOURCE'] = True You shouldn't have removed the body. ::: mozglue/build/Makefile.in @@ +9,5 @@ > include $(topsrcdir)/config/config.mk > > ifneq (1_1,$(MOZ_MEMORY)_$(or $(MOZ_NATIVE_JEMALLOC),$(FORCE_SHARED_LIB))) > > +ifneq (,$(filter WINNT,$(OS_ARCH))) Kill $(filter). ::: security/build/Makefile.in @@ +33,5 @@ > endif > > SDK_LIBS = crmf > > +ifneq (,$(filter WINNT,$(OS_ARCH))) Kill $(filter). @@ +369,5 @@ > grep -v -h -e ^LIBRARY -e ^EXPORTS -e ^\; $^ >> $@.tmp > mv $@.tmp $@ > endif > > +ifneq (,$(filter WINNT,$(OS_ARCH))) Kill $(filter). ::: toolkit/library/Makefile.in @@ +163,5 @@ > ifdef MOZ_DIRECTSHOW > OS_LIBS += $(call EXPAND_LIBNAME,dmoguids wmcodecdspuuid strmiids msdmo) > endif > > +ifneq (,$(filter WINNT,$(OS_ARCH))) Kill $(filter). ::: toolkit/mozapps/installer/package-name.mk @@ +84,5 @@ > ifndef MOZ_PKG_LONGVERSION > MOZ_PKG_LONGVERSION = $(shell echo $(MOZ_PKG_VERSION)) > endif > > +ifeq (,$(filter-out Darwin, $(OS_ARCH))) # Mac Kill $(filter-out). ::: toolkit/mozapps/installer/packager.mk @@ +9,5 @@ > ifndef MOZ_PKG_FORMAT > ifeq (cocoa,$(MOZ_WIDGET_TOOLKIT)) > MOZ_PKG_FORMAT = DMG > else > +ifeq (,$(filter-out WINNT, $(OS_ARCH))) Kill $(filter-out). ::: tools/trace-malloc/Makefile.in @@ -20,5 @@ > CPPSRCS += $(EXTRACPPSRCS) > > -ifeq ($(OS_ARCH),WINNT) > -LOCAL_INCLUDES += -I$(topsrcdir)/config/os2 > -endif I don't even want to know how these lines were added in the first place. ::: xpcom/tests/Makefile.in @@ +14,5 @@ > ifneq (,$(SIMPLE_PROGRAMS)) > $(INSTALL) $(SIMPLE_PROGRAMS) $(DEPTH)/_tests/xpcshell/$(relativesrcdir)/unit > endif > > +ifeq (,$(filter-out WINNT, $(HOST_OS_ARCH))) Kill $(filter-out).
Attachment #8372860 -
Flags: review?(gps) → review+
Assignee | ||
Comment 19•10 years ago
|
||
I spoke with gps on IRC and we decided to leave out the filter stuff because my small mind cannot handle more than two levels of negation without making mistakes!
Assignee | ||
Comment 20•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/58fe9dc85fa4
Comment 21•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/58fe9dc85fa4 https://hg.mozilla.org/mozilla-central/rev/221df43616ab
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Comment 22•10 years ago
|
||
Bug 969757 (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailacopolypse) from comment #5) > I think the OS2 port can take the reverse of my patch if they would like to > keep updated with mozilla-central, but looking at their github repo, they > have not tried to merge anything from us in since Firefox 17 ESR, so I don't > see this patch adversely affecting them. > CCing Dmitriy who I believe is the current maintainer here. Removing the references in the build system is not optimal for us but removal of actual code is a true PITA for the next step.
Comment hidden (spam) |
Comment 24•10 years ago
|
||
Not even a courtesy CC?
Assignee | ||
Comment 25•10 years ago
|
||
(In reply to comment #24) > Not even a courtesy CC? Sorry, please see above, I did try to find who maintains the OS2 port and CC them on the bug, and they also weighed in on the dev-platform discussion. I didn't know you're involved with the OS2 port as well.
Comment 26•9 years ago
|
||
JFYI. The current OS/2 development continues here: https://github.com/bitwiseworks/mozilla-os2. We are currently at esr38 level (almost — it's already merged into our master and I'm trying to make it build ATM). This means that we restored all removed OS/2 patches back when we switched from esr24 to esr31 (and we released a couple of esr31 betas working relatively well since then). All our work is in the repository for the curious parties to observe.
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•