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)

x86
OS/2
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla30

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

Attachments

(1 file, 2 obsolete files)

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!
Attached patch Remove support for OS/2 (obsolete) — Splinter Review
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)
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.
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+
(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.
Attachment #8372817 - Flags: review?(jmuizelaar)
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.
Attachment #8372760 - Flags: review?(mcmanus) → review+
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-
(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.
(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.
Attachment #8372817 - Attachment is obsolete: true
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)
Attachment #8372860 - Flags: superreview?(benjamin)
Attachment #8372860 - Flags: review?(jorendorff)
Attachment #8372860 - Flags: review?(gps)
Attachment #8372860 - Flags: review?(benjamin)
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.
(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!
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 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+
(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 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 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+
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!
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
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.
Not even a courtesy CC?
(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.
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.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.