533542 - don't build readstrings.cpp in source dir

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

the new wince installer builds in the source dir (not exactly sure why that is tbh). As it turns out we rebuild this file in 4 places which is just silly, so I moved it out to use as a static lib.

Comment 1

[(not currently active) Ted Mielczarek]

Brad: this patch is missing toolkit/mozapps/readstrings/Makefile.in

Comment 2

[Mitchell Field [:Mitch]]

That patch also adds hard tabs.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch with makefile

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 421258 [details] [diff] [review]
patch with makefile

>diff --git a/toolkit/mozapps/installer/wince/Makefile.in b/toolkit/mozapps/installer/wince/Makefile.in
>--- a/toolkit/mozapps/installer/wince/Makefile.in
>+++ b/toolkit/mozapps/installer/wince/Makefile.in
>@@ -55,16 +55,17 @@ CPPSRCS = \
> 	nsArchiveExtractor.cpp \
> 	ns7zipExtractor.cpp \
> 	nsSetupStrings.cpp \
>-	$(srcdir)/../../update/src/updater/readstrings.cpp \
> 	$(NULL)
> 
>-LOCAL_INCLUDES += -I$(srcdir)/../../update/src/updater
>+LOCAL_INCLUDES += -I$(srcdir)/../../readstrings
> 
> RCINCLUDE = nsInstallerppc.rc
> 
> DEFINES += -D_UNICODE
> 
>-LIBS += $(DIST)/lib/7z.lib
>+LIBS += $(DEPTH)/toolkit/mozapps/readstrings/$(LIB_PREFIX)readstrings.$(LIB_SUFFIX) \
>+	$(DIST)/lib/7z.lib \
>+	$(NULL)

Ditch the hard tabs here (as Mitch points out), and format it with a two-space indent like:
LIBS += \
  ... \
  ... \
  $(NULL)

> 
> OS_LIBS += aygshell.lib commctrl.lib note_prj.lib oleaut32.lib ole32.lib libcmt.lib coredll.lib corelibc.lib
> 
>diff --git a/toolkit/mozapps/installer/wince/uninstall/Makefile.in b/toolkit/mozapps/installer/wince/uninstall/Makefile.in
>--- a/toolkit/mozapps/installer/wince/uninstall/Makefile.in
>+++ b/toolkit/mozapps/installer/wince/uninstall/Makefile.in
>@@ -47,10 +47,11 @@ PROGRAM   = uninstall$(BIN_SUFFIX)
> CPPSRCS = \
> 	Uninstall.cpp \
> 	../nsSetupStrings.cpp \
>-	$(srcdir)/../../../update/src/updater/readstrings.cpp \
> 	$(NULL)
> 
>-LOCAL_INCLUDES += -I$(srcdir)/.. -I$(srcdir)/../../../update/src/updater
>+LIBS += $(DEPTH)/toolkit/mozapps/readstrings/$(LIB_PREFIX)readstrings.$(LIB_SUFFIX) \

You have an unnecessary trailing \ here.

>diff --git a/toolkit/mozapps/update/src/Makefile.in b/toolkit/mozapps/update/src/Makefile.in
>--- a/toolkit/mozapps/update/src/Makefile.in
>+++ b/toolkit/mozapps/update/src/Makefile.in
>@@ -47,7 +47,9 @@ MODULE = update
> EXTRA_PP_COMPONENTS = nsUpdateTimerManager.js
> 
> ifdef MOZ_UPDATER
>-DIRS = updater
>+DIRS =	../../readstrings \
>+	updater \
>+	$(NULL)

Same formatting here, two space indent.

r=me with those nits fixed. (also I ran this by the tryserver to test it with my patch from bug 428532, and it built fine.)

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/e0fae6222746

Comment 6

[Johnathan Nightingale [:johnath]]

Comment on attachment 421258 [details] [diff] [review]
patch with makefile

Won't make 1.9.2.2, but this seems safe. Moving the flag to 1.9.2.3, but is this something we even need to take on branch?

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #6)
> (From update of attachment 421258 [details] [diff] [review])
> Won't make 1.9.2.2, but this seems safe. Moving the flag to 1.9.2.3, but is
> this something we even need to take on branch?

This can cause phantom build bustages on clobber builds.  Since there isn't much code churn on branch though, so its less likely to happen.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

err... I meant dep builds, not clobber

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 421258 [details] [diff] [review]
patch with makefile

Clearing old approval requests now that 1.9.2.4 has shipped. If you believe this patch is still necessary on the 1.9.2 branch please re-request approval along with a risk/benefit analysis explaining why we need it.

Comment 10

[Robert Strong (they/them - no direct email)]

Brad, what is the plan regarding WinCE / WinMo code in the tree now that we aren't planning on supporting them anymore? I've reviewed a couple patches where the patch would have broken WinCE / WinMo support and with this happening in the tree I'd prefer putting readstrings back in updater since it would be the only consumer without WinCE / WinMo.

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

There's no requirement to support WinCE or WinMo anymore. While it would be nice to not break things if possible, don't feel any obligation to go out of your way to make it work.

In the case of readstrings, I'd imagine it could be useful elsewhere for localizing native programs, but I'll leave that decision to you.