Closed Bug 533542 Opened 12 years ago Closed 12 years ago

don't build readstrings.cpp in source dir


(Toolkit :: Application Update, defect)

Windows 7
Not set





(Reporter: blassey, Assigned: blassey)



(Keywords: mobile)


(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
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.
Attachment #416617 - Flags: review?(robert.bugzilla)
Brad: this patch is missing toolkit/mozapps/readstrings/
That patch also adds hard tabs.
Assignee: nobody → bugmail
Attachment #416617 - Attachment is obsolete: true
Attachment #421258 - Flags: review?(ted.mielczarek)
Attachment #416617 - Flags: review?(robert.bugzilla)
Comment on attachment 421258 [details] [diff] [review]
patch with makefile

>diff --git a/toolkit/mozapps/installer/wince/ b/toolkit/mozapps/installer/wince/
>--- a/toolkit/mozapps/installer/wince/
>+++ b/toolkit/mozapps/installer/wince/
>@@ -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
>-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 += \
  ... \
  ... \

> 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/ b/toolkit/mozapps/installer/wince/uninstall/
>--- a/toolkit/mozapps/installer/wince/uninstall/
>+++ b/toolkit/mozapps/installer/wince/uninstall/
>@@ -47,10 +47,11 @@ PROGRAM   = uninstall$(BIN_SUFFIX)
> 	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/ b/toolkit/mozapps/update/src/
>--- a/toolkit/mozapps/update/src/
>+++ b/toolkit/mozapps/update/src/
>@@ -47,7 +47,9 @@ MODULE = update
> EXTRA_PP_COMPONENTS = nsUpdateTimerManager.js
>-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.)
Attachment #421258 - Flags: review?(ted.mielczarek) → review+
Closed: 12 years ago
Resolution: --- → FIXED
Attachment #421258 - Flags: approval1.9.2.2?
Comment on attachment 421258 [details] [diff] [review]
patch with makefile

Won't make, but this seems safe. Moving the flag to, but is this something we even need to take on branch?
Attachment #421258 - Flags: approval1.9.2.2? → approval1.9.2.3?
(In reply to comment #6)
> (From update of attachment 421258 [details] [diff] [review])
> Won't make, but this seems safe. Moving the flag to, 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.
err... I meant dep builds, not clobber
Comment on attachment 421258 [details] [diff] [review]
patch with makefile

Clearing old approval requests now that 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.
Attachment #421258 - Flags: approval1.9.2.4?
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.
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.
You need to log in before you can comment on or make changes to this bug.