Closed
Bug 533542
Opened 15 years ago
Closed 15 years ago
don't build readstrings.cpp in source dir
Categories
(Toolkit :: Application Update, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: blassey, Assigned: blassey)
References
Details
(Keywords: mobile)
Attachments
(1 file, 1 obsolete file)
6.72 KB,
patch
|
ted
:
review+
|
Details | Diff | 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)
Comment 1•15 years ago
|
||
Brad: this patch is missing toolkit/mozapps/readstrings/Makefile.in
Comment 2•15 years ago
|
||
That patch also adds hard tabs.
Assignee | ||
Comment 3•15 years ago
|
||
Assignee: nobody → bugmail
Attachment #416617 -
Attachment is obsolete: true
Attachment #421258 -
Flags: review?(ted.mielczarek)
Attachment #416617 -
Flags: review?(robert.bugzilla)
Comment 4•15 years ago
|
||
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.)
Attachment #421258 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 5•15 years ago
|
||
pushed http://hg.mozilla.org/mozilla-central/rev/e0fae6222746
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•15 years ago
|
Attachment #421258 -
Flags: approval1.9.2.2?
Comment 6•15 years ago
|
||
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?
Attachment #421258 -
Flags: approval1.9.2.2? → approval1.9.2.3?
Assignee | ||
Comment 7•15 years ago
|
||
(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.
Assignee | ||
Comment 8•15 years ago
|
||
err... I meant dep builds, not clobber
Comment 9•14 years ago
|
||
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.
Attachment #421258 -
Flags: approval1.9.2.4?
Comment 10•14 years ago
|
||
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.
Assignee | ||
Comment 11•14 years ago
|
||
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.
Description
•