Add makefile deps for classes.dex, R.java, gecko.ap_, etc. Remove the FORCE pre-req that forces targets to be perpetually stale. For targets explicitly using the entire list of dependencies for building see if the logic can be updated to only act on files that have been modified.

Added local vars in place of hardcoded paths. Use GENERATED_DIRS to create directories in bulk. classes.dex split into two rules: 1) compile 3rd party java sources 2) compile target sources Timestamp file added for the libs:: target so classes-dex and pkg-name will only be installed when sources are modified. Refactored common deps from targets R.java and gecko.ap_, list them only once. res/draw* targets: replace nsinstall -D calls with mkdir_deps. Use *-preqs macros to reformat targets for readability. Deps should work but edits tickle javac warnings (fatal w/-Werror)

Same patch as last time. Added option NO_WERROR for interactive use to disable -Werror and -Xlint:all to spot basic errors in the stream of failures. After fixing several Interface related problems in source, dependencies can now trigger proper builds with one exception -- strings.xml. Applying the patch from bug 748130 will repair the final stale deps beneath locales/ allowing dependency builds to become a nop.

Comment on attachment 620113 [details] [diff] [review] Replace FORCE with dependencies. FORCE is conditional for language repacks Review of attachment 620113 [details] [diff] [review]: ----------------------------------------------------------------- This is mostly okay, I just have a couple of issues with it. ::: mobile/android/base/Makefile.in @@ +46,5 @@ > > DIRS = locales > > +# Local vars to avoid typos > +classes-dex = classes.dex Okay, this doesn't seem very useful. It's the exact same number of characters, which means that typing it out as a variable will actually be *longer*. @@ +685,5 @@ > +ifdef NO_WERROR > + javac-flags := $(filter-out -Werror,$(JAVAC_FLAGS)) > +else > + javac-flags += -Xlint:all,-deprecation,-fallthrough > +endif You define this variable but don't seem to use it. @@ +703,5 @@ > +classes-dex-excl =\ > + $(dir-classes) \ > + $(NULL) > + > +# classes/ may be the only change so supply at least one file for compiling This doesn't make much sense. All the Java code is compiled into the classes/ dir, so if we had to make the dir, we have to compile *everything*, no?

(In reply to Ted Mielczarek [:ted] from comment #3) > Comment on attachment 620113 [details] [diff] [review] > Replace FORCE with dependencies. FORCE is conditional for language repacks > > Review of attachment 620113 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is mostly okay, I just have a couple of issues with it. > > ::: mobile/android/base/Makefile.in > @@ +46,5 @@ > > > > DIRS = locales > > > > +# Local vars to avoid typos > > +classes-dex = classes.dex > > Okay, this doesn't seem very useful. It's the exact same number of > characters, which means that typing it out as a variable will actually be > *longer*. Using macros in place of hardcoded values will allow for some basic typo checking with: gmake --warn-undefined-variables. Also the assignments tend to follow the syntax visually grouping them together: target target-preqs target-${modifier} $(target) $(target-preqs) classes.dex would be a special case in this context even though it is the actual target name. > @@ +685,5 @@ > > +ifdef NO_WERROR > > + javac-flags := $(filter-out -Werror,$(JAVAC_FLAGS)) > > +else > > + javac-flags += -Xlint:all,-deprecation,-fallthrough > > +endif > > You define this variable but don't seem to use it. The option was intended for interactive use, ignore warning induced failures and only display errors: gmake -C obj/mobile/base NO_WARN=1 > > @@ +703,5 @@ > > +classes-dex-excl =\ > > + $(dir-classes) \ > > + $(NULL) > > + > > +# classes/ may be the only change so supply at least one file for compiling > > This doesn't make much sense. All the Java code is compiled into the > classes/ dir, so if we had to make the dir, we have to compile *everything*, > no? classes/ is not the problem source, it is the .mkdir.deps timestamp file that exists within the directory that can be a problem. The dep is needed as a pre-requisite to create the directory but cannot be passed to javac as a source file. Any non-java deps appended to the dependency list will also need to be filtered from the compilers view. Source filtering could be avoided but it will require extra targets.

(In reply to Joey Armstrong [:joey] from comment #4) > Using macros in place of hardcoded values will allow for some basic typo > checking with: gmake --warn-undefined-variables. Unless this is running as some sort of automated test it's not going to prevent people from making mistakes. In general I'm in favor of putting repeated information into variables, but making the variable reference longer than the filename is silly. > > You define this variable but don't seem to use it. > > The option was intended for interactive use, ignore warning induced failures > and only display errors: > > gmake -C obj/mobile/base NO_WARN=1 I understand the purpose, but you don't actually use $(javac-flags) anywhere, so it doesn't do anything. (Unless I've missed something?) > classes/ is not the problem source, it is the .mkdir.deps timestamp file > that exists within the directory that can be a problem. The dep is needed > as a pre-requisite to create the directory but cannot be passed to javac as > a source file. Any non-java deps appended to the dependency list will also > need to be filtered from the compilers view. I understand that, I'm questioning your comment there specifically. If the only changed prerequisite is the .mkdir.deps file, then surely we had to create that directory, in which case we don't have any compiled .class files and have to recompile all the Java files, no?

(In reply to Ted Mielczarek [:ted] from comment #5) > (In reply to Joey Armstrong [:joey] from comment #4) > > Using macros in place of hardcoded values will allow for some basic typo > > checking with: gmake --warn-undefined-variables. > > Unless this is running as some sort of automated test it's not going to > prevent people from making mistakes. In general I'm in favor of putting > repeated information into variables, but making the variable reference > longer than the filename is silly. Hmmm interesting thought -- automated checkin test. This would be easy enough to add checking only lowercase macro names initially. Saving for a future bug, the general case will need some filtering and cleanup before that can be handled. Well not handling this filenames as a special case within the file and having the option of checking for typos -- for me outweighs having to decorate the value with three extra characters $(). Though I will revert the value for this nit. > > > You define this variable but don't seem to use it. > > > > The option was intended for interactive use, ignore warning induced failures > > and only display errors: > > > > gmake -C obj/mobile/base NO_WARN=1 > > I understand the purpose, but you don't actually use $(javac-flags) > anywhere, so it doesn't do anything. (Unless I've missed something?) Argh the flags edit was used for dx but did not make it in for the merge+patch upload. I'll yank this out for now so it will not need to be reviewed. If added the conditional probably should go in config/android-common.mk so it can be used in other directories. > > classes/ is not the problem source, it is the .mkdir.deps timestamp file > > that exists within the directory that can be a problem. The dep is needed > > as a pre-requisite to create the directory but cannot be passed to javac as > > a source file. Any non-java deps appended to the dependency list will also > > need to be filtered from the compilers view. > > I understand that, I'm questioning your comment there specifically. If the > only changed prerequisite is the .mkdir.deps file, then surely we had to > create that directory, in which case we don't have any compiled .class files > and have to recompile all the Java files, no? Two cases I can think of where classes related deps will be up-to-date but the timestamp file will not exist: o When these edits are deployed -- classes/* may have been created prior to deployment of directory dependency. An explicit clobber build would be needed to prevent the directory dependency from being the only stale dependency found by the target. o If someone were to manually run: find . -name '.mkdir.done' | xargs rm to test a change or force stale deps the op would leave the tree in a bad state. Another possible solution that would not require artificially adding files might be to prefix the command with an $(if $(filter)) check to only compile when sources are available. I opted to not use this early on because of the special case and it would have pushed the command over to the right margin making it harder to read. Hmmm how about this for an option, functionality can find usefulness in other places. Add a library function that will accept 3 unexpanded macros and emulate: xargs --no-run-if-empty: run-if($1=vals, $2=wanted, $3=cmd) 1) pass in a list of sources or pre-requisites that caused a target to be considered stale. 2) Use callback 'wanted' to find sources of interest. 3) If wanted returns a list of values, invoke cmd on those values. That should support conditional processing, only when needed, w/o having to explicitly add filters and conditionals everywhere.

Same patch as before + refresh with m-c + additions. 1) reverted $(classes-dex) references to classes.dex filename. 2) replaced NO_WERROR + javac-flags with a local var containing all of the -Xlint: flags. Var can be cleared from the command line to emulate NO_WERROR=1 behavior. 3) reworked $(JAVAC) compile line to remove the bogus 'always require at least one source file' dependency. Command wrapped by an $(if ) conditional and will only be invoked when sources were detected as a pre-req. Recent additions: ================= 1) Added an important missing dep - *-preqs are dependent on Makefile.in so targets will rebuild/repackage when makefile edits are checked in. 2) Added the use-makeutils.mk-as-a-front-end-loader-to-load-library-makefiles logic. autotargets.mk will need to load early so mkdir_deps will be available. 3) Included unit test for makeutils + autotarget loading. A try job to verify the patch is in progress.

Comment on attachment 621069 [details] [diff] [review] Replace FORCE pre-reqs with dependencies Review of attachment 621069 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review because a new revision of this patch is impending, but I had a couple of comments from when I started looking at this on Friday. ::: config/makefiles/autotargets.mk @@ +17,5 @@ > ########################################################################### > # Threadsafe directory creation > # GENERATED_DIRS - Automated creation of these directories. > ########################################################################### > +mkdir_deps =$(subst //,/,$(foreach dir,$(getargv),$(dir)/.mkdir.done)) What's the purpose of this change? ::: config/makefiles/makeutils.mk @@ +104,5 @@ > + > +########################################################################### > +## Common makefile library loader > +########################################################################### > +topsrcdir_my = $(if $(topsrcdir),$(topsrcdir),$(error topsrcdir is not defined)) This seems like overkill. topsrcdir is set at the top of every Makefile, it's really not worth checking for. ::: config/makefiles/test/Makefile.in @@ +29,5 @@ > ##------------------_## > all:: > > +clean: > + $(RM) $(check-tests) You can just add this to GARBAGE.

(In reply to Ted Mielczarek [:ted] from comment #8) > Comment on attachment 621069 [details] [diff] [review] > Replace FORCE pre-reqs with dependencies > > Review of attachment 621069 [details] [diff] [review]: > ----------------------------------------------------------------- > > Clearing review because a new revision of this patch is impending, but I had > a couple of comments from when I started looking at this on Friday. > > ::: config/makefiles/autotargets.mk > @@ +17,5 @@ > > ########################################################################### > > # Threadsafe directory creation > > # GENERATED_DIRS - Automated creation of these directories. > > ########################################################################### > > +mkdir_deps =$(subst //,/,$(foreach dir,$(getargv),$(dir)/.mkdir.done)) > > What's the purpose of this change? To squeeze extra /'s out of the path. These strings are easy to create with: $(call mkdir_deps,$(dir $(zzz))) > ::: config/makefiles/makeutils.mk > @@ +104,5 @@ > > + > > +########################################################################### > > +## Common makefile library loader > > +########################################################################### > > +topsrcdir_my = $(if $(topsrcdir),$(topsrcdir),$(error topsrcdir is not defined)) > > This seems like overkill. topsrcdir is set at the top of every Makefile, > it's really not worth checking for. > > ::: config/makefiles/test/Makefile.in > @@ +29,5 @@ > > ##------------------_## > > all:: > > > > +clean: > > + $(RM) $(check-tests) > > You can just add this to GARBAGE. Both of these edits were added for the makefile unit tests. Tests are running in a minimal environment where few makefiles have been included. rules.mk has not been loaded so an explicit clean target was defined.

wip: a mutex timestamp file has been added to synchronize building third-party java sources independent from classes.dex. Line 726: third-party-classes - problem timestamp Line 745: $(third-party-classes): $(third-party-preqs) gmake clean or $(RM) -r .deps then rebuild. $(third-party-classes) will pre-exist when the target is evaluated. timestamp is newer than all source pre-requisites leaving only .mkdir.done and Makefile.in on the dependency list. Removing third-party-classes as a dependency of classes.dex will allow sources/pre-reqs to be found and compiled. With $(third-party-classes) used as a target dependency these are the pre-requisistes found: % rm -r .deps % gmake ** target=[.deps/ts/third-party-classes.ts], preqs=[classes/.mkdir.done /local/mozilla/bugs/748470-new/mobile/android/base/Makefile.in] /bin/ls -ld .deps/ts/third-party-classes.ts -rw-rw-r-- 1 joey joey 0 2012-05-07 17:01 .deps/ts/third-party-classes.ts [ insert compile line here, no deps ] TIMESTAMP CREATED: [.deps/ts/third-party-classes.ts] - MAKECMDGOALS=[libs]

Merge in more edits from my local sandbox into the original patch

(In reply to Joey Armstrong [:joey] from comment #9) > Both of these edits were added for the makefile unit tests. Tests are > running in a minimal environment where few makefiles have been included. > rules.mk has not been loaded so an explicit clean target was defined. Ah, I missed that. Thanks.

Wrapper $(shell cat) commands within a target stem test so overhead will only be imposed while building or cleaning. Moved -Xlint:all flags into a named variable so usage can easily be seen or disabled from the command line: gmake javac-xlint= classes-dex was reverted to classes.dex from an earlier patch. Added an explicit default target for classes.dex in place of listing the target rule first in the makefile. Refactored common deps out of R.java and gecko.ap_ into a named variable and reused. Use third-party sources as a dependency for classes.dex but files need to be filtered from the compile line. These files will not compile cleanly with -Xlint:all and are compiled by another command line. Defined three vars per target: tgt{-preqs, -excl, -src}. -preqs: the full list of dependencies to check for a target. -excl: filter -preqs into a list of sources to check and conditionally compile. -src: shorten long pre-requisiste lists. mutex timestamp file third-party-classes added as a pre-req for classes.dex allowing third-party files to finish compiling before the jarfile is created. Include makeutils.mk w/unit tests to define mkdir_deps. Added a dep on Makefile.in to force regeneration when the makefile is modified. mkdir_deps - scrub '//' from given paths, the string can be easily created with $(call mkdir_deps,$(dir $(path))) nsinstall -D calls replaced with mkdir_deps dependencies

Compared classes.dex, R.java, gecko.ap_, Manifest* AndroidManifest.xml against copies built in a clean sandbox, no differences reported. classes.dex was converted to jar, unpacked and diff -qr old new

Comment on attachment 621761 [details] [diff] [review] full patch upload Review of attachment 621761 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, just a couple of real minor nits. ::: mobile/android/base/Makefile.in @@ +88,5 @@ > +#$(info SYNC_PP_JAVA_FILES=$(SYNC_PP_JAVA_FILES)) > +$(info SYNC_THIRDPARTY_JAVA_FILES=$(SYNC_THIRDPARTY_JAVA_FILES)) > +$(info ===========================================================================) > +$(info ) > +endif Make sure to remove this before landing. @@ +748,5 @@ > + $(if $(filter-out $(third-party-excl),$?),(\ > + $(JAVAC) $(JAVAC_FLAGS) -d classes $(filter-out $(third-party-excl),$?)\ > + )) > + @echo "TIMESTAMP CREATED: [$@] - MAKECMDGOALS=[$(MAKECMDGOALS)]" > + @$(TOUCH) $@ Looks like you still have a bunch of debug statements in here. Please remove them before landing. @@ +785,4 @@ > $(DX) --dex --output=$@ classes > > +# Makefile edits will regenerate the world > +$(classes-dex-preqs) : $(srcdir)/Makefile.in You could (and probably should) just use $(GLOBAL_DEPS) here: http://mxr.mozilla.org/mozilla-central/source/config/rules.mk#767

Comment on attachment 621967 [details] [diff] [review] Replace FORCE token use with dependencies, conditional force dep for language repacks Review of attachment 621967 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/Makefile.in @@ +78,5 @@ > + SYNC_RES_DRAWABLE_LDPI=$(shell cat $(topsrcdir)/mobile/android/sync/android-drawable-ldpi-resources.mn | tr '\n' ' ';) > + SYNC_RES_DRAWABLE_MDPI=$(shell cat $(topsrcdir)/mobile/android/sync/android-drawable-mdpi-resources.mn | tr '\n' ' ';) > + SYNC_RES_DRAWABLE_HDPI=$(shell cat $(topsrcdir)/mobile/android/sync/android-drawable-hdpi-resources.mn | tr '\n' ' ';) > + SYNC_RES_LAYOUT=$(shell cat $(topsrcdir)/mobile/android/sync/android-layout-resources.mn | tr '\n' ' ';) > + SYNC_RES_VALUES=$(shell cat $(topsrcdir)/mobile/android/sync/android-values-resources.mn | tr '\n' ' ';) If you're only running these when they need to be run, you should probably make these := to ensure the shell only gets run once. @@ +692,5 @@ > # Override the Java settings with some specific android settings > include $(topsrcdir)/config/android-common.mk > > +# Default target > +libs:: classes.dex rules.mk already enforces a default target, and it looks like this dependency already exists transitively through the rule below: libs:: $(R-java) $(gecko-ap) third-party-classes $(libs-install) $(libs-install): classes.dex $(pkg-name) @@ +779,4 @@ > $(DX) --dex --output=$@ classes > > +# Makefile edits will regenerate the world > +$(classes-dex-preqs) : $(srcdir)/Makefile.in As I mentioned in the other review, I think you want $(GLOBAL_DEPS) here. @@ +812,4 @@ > $(PYTHON) $(topsrcdir)/config/Preprocessor.py \ > $(AUTOMATION_PPARGS) $(DEFINES) $(ACDEFINES) $< > $@ > > +$(gen-preqs) : $(srcdir)/Makefile.in Probably GLOBAL_DEPS here too @@ +896,2 @@ > $(INSTALL) classes.dex $(FINAL_TARGET) > + $(INSTALL) $(pkg-name) $(FINAL_TARGET) I think you can just install these all in one call, which means you could just use $(INSTALL) $^ $(FINAL_TARGET)

(In reply to Ted Mielczarek [:ted] from comment #16) > Comment on attachment 621967 [details] [diff] [review] > Replace FORCE token use with dependencies, conditional force dep for > language repacks > > Review of attachment 621967 [details] [diff] [review]: > ----------------------------------------------------------------- > > > +# Makefile edits will regenerate the world > > +$(classes-dex-preqs) : $(srcdir)/Makefile.in > > As I mentioned in the other review, I think you want $(GLOBAL_DEPS) here. I'm going to leave the $(topsrcdir)/Makefile.in' references alone for now. GLOBAL_DEP pre-requisites are being found by vpath. Appending $(GLOBAL_DEPS) to the complier -excl{usion} list is a nop because string literal filenames were added rather than file-by-path so Makefile.in, etc cannot be filtered.

Ok, makes sense. Might want to flip that to be a whitelist-by-.java-extension instead of an exclusion list in the future then.

Comment on attachment 622170 [details] [diff] [review] replace FORCE with dependencies, conditional use of force for language repacks Same patch as before. o $(shell cat) lines changed to := o opened bug 753090 to address use of $(GLOBAL_DEPS) -vs- vpath -vs- $(filter ). Leaving $(topsrcdir)/Makefile.in dependency in place for now. o removed default target rule o combined final nsinstall commands

patching file config/makefiles/autotargets.mk Hunk #1 FAILED at 11 1 out of 1 hunks FAILED -- saving rejects to file config/makefiles/autotargets.mk.rej patching file js/src/config/makefiles/autotargets.mk Hunk #1 FAILED at 11 1 out of 1 hunks FAILED -- saving rejects to file js/src/config/makefiles/autotargets.mk.rej patching file mobile/android/base/Makefile.in Hunk #6 FAILED at 679 1 out of 6 hunks FAILED -- saving rejects to file mobile/android/base/Makefile.in.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh 748470

Comment on attachment 623222 [details] [diff] [review] replace FORCE with dependencies, conditional use of force for language repacks Same patch as last time. Re-merged against m-c to pickup the latest edits in Makefile.in Testing on try in progress.

Comment on attachment 623358 [details] [diff] [review] replace FORCE with dependencies, conditional use of force for language repacks Carry review forward. Re-adding RES_DRAWABLE, assignment was dropped with the last rebase. r=ted

Comment on attachment 623358 [details] [diff] [review] replace FORCE with dependencies, conditional use of force for language repacks Landed on Inbound http://hg.mozilla.org/integration/mozilla-inbound/rev/c115c58ef2b1

Joey, I think this needs to be backed out due to regressions (bug 757442, bug 760602, and bug 758272).

backed out on trunk (inbound) and aurora: https://hg.mozilla.org/integration/mozilla-inbound/rev/211f802a3c1c https://hg.mozilla.org/releases/mozilla-aurora/rev/3dfea949c53a

(In reply to Brad Lassey [:blassey] from comment #30) > backed out on trunk (inbound) and aurora: > https://hg.mozilla.org/integration/mozilla-inbound/rev/211f802a3c1c > https://hg.mozilla.org/releases/mozilla-aurora/rev/3dfea949c53a Does the same need to be done for FN14 on mozilla-beta? If not, any reason to continue tracking this?

Any chance we can get this patch back in (with whatever issues it had fixed)?

I was able to reproduce the multi-locale builds locally using info from this blog: http://escapewindow.dreamwidth.org/234671.html By capturing the output from the 'mozharness/scripts/multil10n.py --cfg myconfig.py' step, I can see that with the FORCE target present, gecko.ap_ is built twice, and the second time ends up with a resources.arsc file that has translated strings. Without the FORCE target, gecko.ap_ is only built the first time, so the resources.arsc file only contains en-US data. My iteration script was as follows: time mozharness/scripts/multil10n.py --cfg myconfig.py --restore-objdir cd mozilla-central-git2/obj-x86_64-unknown-linux-gnu ./config.status cd ../.. time mozharness/scripts/multil10n.py --cfg myconfig.py > output.txt 2>&1 The problem is that the locale builds generate extra files in res/ that aren't listed as dependencies for gecko.ap_, like these: res/values-de/strings.xml res/values-zh-rCN/strings.xml res/values-sk/strings.xml By using $(wildcard res/values*/strings.xml) we can add the appropriate dependencies, so gecko.ap_ gets rebuilt when locale information is present without having to FORCE it every time. The previous patch attempted to use $(if $(IS_LANGUAGE_REPACK),FORCE) to only add FORCE during these l10n builds, but unfortunately the condition is never true since the 'make gecko.ap_' call does not pass in an L10NBASEDIR, which is necessary for config.mk to set IS_LANGUAGE_REPACK. Requesting review from a build peer (gps), and feedback from a Fennec dev (nalexander) - feel free to redirect if you feel others are more appropriate.

Comment on attachment 780652 [details] [diff] [review] fennec-force.patch Review of attachment 780652 [details] [diff] [review]: ----------------------------------------------------------------- f+ with my query investigated to your satisfaction, mshal. ::: mobile/android/base/Makefile.in @@ +1371,5 @@ > $(MAKE) -C locales > > +# With multilocale builds, there will be multiple strings.xml files. We need to > +# rebuild gecko.ap_ if any of them change. > +STRINGS_XML_FILES := $(wildcard res/values*/strings.xml) The various strings.xml files are themselves the result of pre-processing. I don't know that language repacks actually do that pre-processing; they could just update the inputs (strings.xml.in and friends) but not actually generate the outputs. If you look at res/values/strings.xml above, we always build locales/, which does this processing for en-US. (And presumably for other locales if they are present.) Re-reading your comment in the ticket, perhaps this is not the case? If you're confident that the l10n repack doesn't need a fresh build locales/ step to pick up changes, roll on.

(In reply to Nick Alexander :nalexander from comment #34) > Comment on attachment 780652 [details] [diff] [review] > fennec-force.patch > > Review of attachment 780652 [details] [diff] [review]: > ----------------------------------------------------------------- > > f+ with my query investigated to your satisfaction, mshal. > > ::: mobile/android/base/Makefile.in > @@ +1371,5 @@ > > $(MAKE) -C locales > > > > +# With multilocale builds, there will be multiple strings.xml files. We need to > > +# rebuild gecko.ap_ if any of them change. > > +STRINGS_XML_FILES := $(wildcard res/values*/strings.xml) > > The various strings.xml files are themselves the result of pre-processing. > I don't know that language repacks actually do that pre-processing; they > could just update the inputs (strings.xml.in and friends) but not actually > generate the outputs. If you look at res/values/strings.xml above, we > always build locales/, which does this processing for en-US. (And > presumably for other locales if they are present.) > > Re-reading your comment in the ticket, perhaps this is not the case? If > you're confident that the l10n repack doesn't need a fresh build locales/ > step to pick up changes, roll on. That's a good point - using the $(wildcard) probably breaks the regular strings.xml dependency, since the file doesn't exist yet (double-checking that now). I don't fully understand the multilocale builds, but from what I gather from the output of multil10n.py, each values-*/strings.xml file is built with a separate make invocation like this: Running command: make chrome-zh-TW L10NBASEDIR=/home/mshal/multilocale/./l10n LOCALE_MERGEDIR=/home/mshal/multilocale/./mozilla-central-git2/obj-x86_64-unknown-linux-gnu/merged in /home/mshal/multilocale/./mozilla-central-git2/obj-x86_64-unknown-linux-gnu/mobile/android/locales Those makes end with a Preprocessor.py invocation to create a file like ../res/values-zh-rTW/strings.xml After all of those are complete, another make invocation is used with the 'gecko.ap_' target. This runs the aapt command that reads in everything under res, so it either needs a dependency on all of those strings.xml files, or we FORCE it every time. Note that for incremental builds (just a 'make'), gecko.ap_ isn't built at all - just removing FORCE from R.java is enough to avoid all the work in a no-op build. It looks like that was added to R.java in bug 712380, though it's not clear to me why. Maybe it would be better to just remove FORCE from R.java, and leave it for gecko.ap_ ?

Actually I don't understand why there is a strings.xml target in mobile/android/base/Makefile.in at all - it seems to get built already from the recursion into locales. I added some debug to the rule, and it doesn't look like it gets triggered. blassey, can you comment why it's needed? It was added in bug 889541.

if the res/ dir is clobbered (because a dependency gets touched) it gets deleted wholesale, which deletes strings.xml. The rule gives a mechanism to recreate strings.xml as needed. Another option would be to delete individual files in res/ but the problem with that is it doesn't clean up deleted or renamed resources.

Comment on attachment 780652 [details] [diff] [review] fennec-force.patch I'm updating this patch, will have a new version out shortly.

Updated patch leaves in explicit res/values/strings.xml, and only wildcards for the extra multilocale strings.xml files. r? from gps for build config, and r? from blassey to ensure this doesn't break any Fennec development use-cases that aren't already broken.

Comment on attachment 782764 [details] [diff] [review] Remove FORCE from mobile/android/base Review of attachment 782764 [details] [diff] [review]: ----------------------------------------------------------------- giving you a feedback+ because I don't see anything wrong with this patch but can't say I understand the implications well enough to r+

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #40) > giving you a feedback+ because I don't see anything wrong with this patch > but can't say I understand the implications well enough to r+ Is there someone on the Fennec team who would be able to r+ it? I'm confident in gps' ability to review build config changes, but I don't believe he does Fennec development on a regular basis so I would like someone with that expertise to approve it as well.

(In reply to Michael Shal [:mshal] from comment #41) > (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #40) > > giving you a feedback+ because I don't see anything wrong with this patch > > but can't say I understand the implications well enough to r+ > > Is there someone on the Fennec team who would be able to r+ it? I'm > confident in gps' ability to review build config changes, but I don't > believe he does Fennec development on a regular basis so I would like > someone with that expertise to approve it as well. I'm not *technically* a Fennec peer but I'm the right person to review.

Comment on attachment 782764 [details] [diff] [review] Remove FORCE from mobile/android/base Review of attachment 782764 [details] [diff] [review]: ----------------------------------------------------------------- This should be fine for local developers, and based on mshal's analysis of the l10n-repack story, we should be good there. I'd like to let this bake for a few days (to make sure we don't break Nk builds) and then I suggest we uplift to Aurora: the machinery for l10n builds appears to be different there, and I'd like us to have fresh context in the unlikely event this breaks. I just thought of a possible issue. There is some magic around invalidating the JS startup cache based on changing the buildid. It's possible that |make package| will no longer update the APK in just the right way, meaning that changes to omni.ja might not show up on device. (This happens in several irritating situations.) I've never dug into this buildid magic, so bouncing back to blassey for more feedback. ::: mobile/android/base/Makefile.in @@ +1384,5 @@ > $(PP_RES_XML) \ > $(NULL) > > R.java: $(all_resources) > +R.java: nit: kill this line. @@ +1389,4 @@ > $(AAPT) package -f -M AndroidManifest.xml -I $(ANDROID_SDK)/android.jar -S res -J . --custom-package org.mozilla.gecko > > gecko.ap_: $(all_resources) > +gecko.ap_: nit: ditto.

Comment on attachment 782764 [details] [diff] [review] Remove FORCE from mobile/android/base Review of attachment 782764 [details] [diff] [review]: ----------------------------------------------------------------- build id is rev'd by configure and incorperated into the build by in application.ini.h*. I believe this is what your referring to, and if so, this change shouldn't have any effect on it. *https://mxr.mozilla.org/mozilla-central/source/build/Makefile.in#121

Removed useless target lines, r+ carried forward. Try results: https://tbpl.mozilla.org/?tree=Try&rev=8240ff4efe14

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/eabb56620dad

Earlier, I said: "I'd like to let this bake for a few days (to make sure we don't break Nk builds) and then I suggest we uplift to Aurora: the machinery for l10n builds appears to be different there, and I'd like us to have fresh context in the unlikely event this breaks." I had a discussion with Pike around this, who recommended not uplifting: 08:26 nalexander: edmorley: can you point me at the TBPL build that generates Android l10n Aurora binaries? 08:26 nalexander: edmorley: whatever populates http://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/latest-mozilla-aurora-android-l10n/ 08:36 edmorley: nalexander: the l10n builds don't appear on tbpl since they're set up weirdly iirc 08:36 nalexander: edmorley: thanks for verification. Is there anything I should know about 08:37 edmorley: nalexander: specifically? 08:37 edmorley: :-) 08:37 nalexander: edmorley: I was suggesting an l10n-impacting bug get uplifted to Aurora so we're not confused in X weeks when aurora l10n break.s 08:38 nalexander: edmorley: it's really a very small bug, but I don't understand what (if any) differences there are in Aurora l10n packing. 08:38 nalexander: edmorley: so being cautioujs. 08:38 edmorley: nalexander: I don't know myself, maybe ask one of the build peers or pike? 08:38 nalexander: edmorley: will do. 08:39 Pike: nalexander: bug number? 08:39 Pike: also, there's no build reporting on l10n builds beyond the log files on ftp 08:42 nalexander: Pike: 08:23 firebot: Bug https://bugzilla.mozilla.org/show_bug.cgi?id=748470 nor, --, ---, joey, REOP, mobile/android/base: dependency builds 08:42 nalexander: Pike: it's a very small change to Android l10n dep builds, but I don't understand what the differences are between l10n on Nightly and on Aurora. 08:42 nalexander: Pike: and I can't find tbpl Aurora l10n jobs to investigate. 08:43 nalexander: Pike: so I was being defensive about requesting early uplift to see if this patch causes problems, so we can fix aggressively while we still have context. 08:44 Pike: why uplift to begin with? Also, the aurora builds shouldn't be different to the nightly builds. Beta builds might 08:46 nalexander: Pike: fair enough. 08:48 Pike: nalexander: https://wiki.mozilla.org/Mobile/Fennec/Android#Multilocale_builds is closest to what releng does, I think. you also want to make sure you're not breaking single-locale builds Further to the just completed build config meeting, we would like to get this onto Aurora at the beginning of this cycle so we have weeks rather than days to investigate fallout. I'll prepare the request immediately.

Actually, since we just rolled the train over, this is already on Aurora. I see repacks in http://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/latest-mozilla-aurora-android-l10n/ from August 5th (3 days ago); that's a good sign. Will monitor and take action as needed. In the meantime, let's consider this one fixed and stable.