Last Comment Bug 748130 - mobile/android/base/locales/Makefile.in::strings.xml -- replace FORCE dep with file dependencies
: mobile/android/base/locales/Makefile.in::strings.xml -- replace FORCE dep wit...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Joey Armstrong [:joey]
:
Mentors:
Depends on:
Blocks: 748452 750675 751941
  Show dependency treegraph
 
Reported: 2012-04-23 14:37 PDT by Joey Armstrong [:joey]
Modified: 2012-05-11 09:13 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Replace 'FORCE' pre-req with file dependencies (2.86 KB, patch)
2012-04-23 19:28 PDT, Joey Armstrong [:joey]
no flags Details | Diff | Splinter Review
RFC: conditional makefile logic for repacks (15.43 KB, patch)
2012-04-25 08:56 PDT, Joey Armstrong [:joey]
no flags Details | Diff | Splinter Review
Same patch as before but FORCE based on def of L10NBASEDIR (2.92 KB, patch)
2012-04-25 12:20 PDT, Joey Armstrong [:joey]
no flags Details | Diff | Splinter Review
Replace FORCE with file dependencies, FORCE should be conditional on repak tasks (3.28 KB, patch)
2012-04-26 10:26 PDT, Joey Armstrong [:joey]
no flags Details | Diff | Splinter Review
replace FORCE with file deps, conditional force for repackage requests (4.31 KB, patch)
2012-04-27 17:00 PDT, Joey Armstrong [:joey]
ted: review+
Details | Diff | Splinter Review

Description Joey Armstrong [:joey] 2012-04-23 14:37:07 PDT
locales/Makefile.in - strings.xml target is dependent on FORCE.

android/base/Makefile.in: R.java target is dependent on strings.xml, target is perpetually stale forcing work to be done on every call.

Replace FORCE with make dependencies so make can do work only when needed.
Comment 1 Joey Armstrong [:joey] 2012-04-23 19:28:37 PDT
Created attachment 617747 [details] [diff] [review]
Replace 'FORCE' pre-req with file dependencies

Replace FORCE pre-req on target %/strings.xml with file dependencies so strings.xml will only regenerate when deps change.

Derive paths from ../res/values based on target/locale and use variables to reduce the chance of typos.

Added a clean target for strings.xml

% gmake; gmake
% touch /local/mozilla/bugs/fennec/mobile/android/base/strings.xml.in
% gmake; gmake

gmake: Nothing to be done for `libs'.
gmake: Nothing to be done for `libs'.
/local/mozilla/bugs/fennec/objdir-droid/config/nsinstall -D ../res/values
/usr/bin/python2.7 /local/mozilla/bugs/fennec/config/Preprocessor.py \
      -DAB_CD=en-US -DOSTYPE=\"Linux\" -DOSARCH=Linux \
  -DBRANDPATH="/local/mozilla/bugs/fennec/objdir-droid/mobile/android/base/locales/../../../../dist/bin/chrome/en-US/locale/branding/brand.dtd" \
  -DSTRINGSPATH="/local/mozilla/bugs/fennec/mobile/android/base/locales/en-US/android_strings.dtd" \
  -DSYNCSTRINGSPATH="/local/mozilla/bugs/fennec/mobile/android/base/locales/en-US/sync_strings.dtd" \
  -DBOOKMARKSPATH="/local/mozilla/bugs/fennec/mobile/locales/en-US/profile/bookmarks.inc" \
  /local/mozilla/bugs/fennec/mobile/android/base/locales/../strings.xml.in \
  > ../res/values/strings.xml
gmake: Nothing to be done for `libs'.
Comment 2 Joey Armstrong [:joey] 2012-04-24 07:25:26 PDT
Try job: https://tbpl.mozilla.org/?tree=Try&rev=033dbacc0273
Comment 3 Axel Hecht [pto-Aug-30][:Pike] 2012-04-24 11:55:33 PDT
How are the dependencies going to work with repacks, where the file modification times don't represent changes?

I.e., you repack german which has a mod time of a day ago and then fr, with a mod time of two days ago.
Comment 4 Ted Mielczarek [:ted.mielczarek] 2012-04-24 12:05:37 PDT
If repacks are abusing dependencies, then I think we should fix repacks instead of forcing developers to needlessly rebuild stuff over and over.
Comment 5 Joey Armstrong [:joey] 2012-04-24 12:19:18 PDT
I agree with Ted on fixing/using real make dependencies to drive the logic.

There are a few options for an interim answer with this patch:
  o append the FORCE pre-requisite only when a repack target or repack specific makefile var is set.
  o cleaner: have the repack target touch a timestamp file on disk for any languages of interest.  Targets can later glob/use individual timestamps as a dep to force re-packaging.
Comment 6 Mike Hommey [:glandium] 2012-04-24 12:22:08 PDT
Repacks are done after an hg clone or an hg update, right ? Then there should be no timestamp problem. Mercurial doesn't use the changeset timestamps. If it were, there would be problems when e.g. bisecting, switching branches, etc.
Comment 7 Axel Hecht [pto-Aug-30][:Pike] 2012-04-24 12:44:25 PDT
(In reply to Mike Hommey [:glandium] from comment #6)
> Repacks are done after an hg clone or an hg update, right ? Then there
> should be no timestamp problem. Mercurial doesn't use the changeset
> timestamps. If it were, there would be problems when e.g. bisecting,
> switching branches, etc.

That's making assumptions on the order of pulling the N repos and the N repacks, though, right?

Also, I don't mind you solving this, but this must not regress our repacks.
Comment 8 Joey Armstrong [:joey] 2012-04-25 08:56:33 PDT
Created attachment 618297 [details] [diff] [review]
RFC: conditional makefile logic for repacks

Opinions on this patch as an option to address special force building for repackage jobs.

1) python script is a simple config file generator.  Pass in a filename stem and a list of key=value pairs and it will generate config file(s) using appropriate syntax (makefile, shell, ini, etc).
2) config/makefiles/task.mk is the primary logic loader.


Processing nutshell:
  o Append the target repack-init as a pre-requisite for any top level repack/l10n targets.
  o include config/rules/tasks.mk in rules.mk (or wherever appropriate).  Logic will mostly be a nop for normal builds/non-repack targets.
  o When a repack is requested the -init target will call repack_mk.py and transient makefile named repack.mk beneath the obj directory.  The makefile currently contains assignments and cached values.
  o For all subsequent targets/$(MAKE) calls attempt to include repack.mk.
  o repack.mk will only exist when repack requests are made.
  o When included repack.mk will define the target REPACK-FORCE { todo: rename as FORCE-REPACK }.  This target can be appended to pre-req lists when a target is required to ignore timestamps, and explicitly do work on every call.  For all other build types FORCE-REPACK=$(NULL) disabling the logic.
  o Final task for the repack: target will be cleanup of the repack.mk metadata directory.

This logic should work within the confines of makefiles in a local tree and be able to emulate force target behavior only for repack requests.
Comment 9 Joey Armstrong [:joey] 2012-04-25 12:20:32 PDT
Created attachment 618387 [details] [diff] [review]
Same patch as before but FORCE based on def of L10NBASEDIR

Per IRC discussion append FORCE target as strings.xml pre-requisite based on repack's L10NBASEDIR macro.
Comment 10 Joey Armstrong [:joey] 2012-04-26 05:54:53 PDT
Comment on attachment 618387 [details] [diff] [review]
Same patch as before but FORCE based on def of L10NBASEDIR

A slightly different conditional will be needed.  Makefile was not re-gnerated after Makefile.in edits + gmake so patch edits have not been fully exercised.

L10NBASEDIR cannot exclusively test for $(NULL) as the string "$(error )" may be present and expansion w/o using $(value ) for access will trigger an error.

Using $(wildcard $(L10NBASEDIR) expansion to check is probably out for performance reasons, conditional may end up in several places.

A string path test will be needed but L10NBASEDIR=subdir_without_slash will be a problem.
Comment 11 Axel Hecht [pto-Aug-30][:Pike] 2012-04-26 06:24:01 PDT
I wonder if it's better to rm $(dir-res-values)/strings.xml in clobber-zip at http://mxr.mozilla.org/mozilla-central/source/mobile/android/locales/Makefile.in#61?

That's called after unpackaging, and at the end of each repackage-zip, http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/l10n.mk#180.

That'd leave the files there for chrome-% (multilocale) with the right deps, and clobber the file for the single-locale repacks.

I'm not sure that switching off the caching based on L10NBASEDIR is good, as many folks that had to try to repack l10n once probably have that left over in their .mozconfig.
Comment 12 Joey Armstrong [:joey] 2012-04-26 10:26:05 PDT
Created attachment 618707 [details] [diff] [review]
Replace FORCE with file dependencies, FORCE should be conditional on repak tasks

minor edits, strip space when testing L10NBASEDIR.
allocate a dependency path for strings.xml.in with ..[/..]+ removed.
added a clean target for strings.xml to easily force regeneration.
Comment 13 Joey Armstrong [:joey] 2012-04-27 07:52:23 PDT
(In reply to Axel Hecht [:Pike] from comment #11)
> I wonder if it's better to rm $(dir-res-values)/strings.xml in clobber-zip
> at
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/locales/
> Makefile.in#61?
> 
> That's called after unpackaging, and at the end of each repackage-zip,
> http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/l10n.mk#180.
> 
> That'd leave the files there for chrome-% (multilocale) with the right deps,
> and clobber the file for the single-locale repacks.

For the general case makefiles are normally setup with a default target like 'clean' that will only remove content generated by the makefile.  If logic is setup properly common commands like this should be usable anywhere in the tree:

  % cd $dir; gmake clean all

When logic is spread out across distinct makefiles or common targets are given special names it forces having to search for components or worse understand the dependency chain simply to force regeneration.

On the file removal side -- makefiles that remove content they cannot regenerate can often leave the build in a bad state for people working locally.  If a file off to the side is removed and cannot be regenerated, people must be aware of having to manually run make in that side directory ( or run make from the top level ) to regenerate the missing dependency.

That said clobber-zip is probably unique enough that if it would make sense to remove the file at this level {not yet aware of all the inter-dependencies for this} the clobber-zip target can be setup to run this:

  $(MAKE) -C ../base/locales clean

The makefile remains self-contained/easy to use and the android/locales::clobber-zip can also force a stale dependency on stings.xml when needed.

> I'm not sure that switching off the caching based on L10NBASEDIR is good, as
> many folks that had to try to repack l10n once probably have that left over
> in their .mozconfig.

If their mozconfig file is incorrectly configured won't this also force performing unrelated repackaging work until the config setting is removed ?  We also have a problem to handle on the flip side -- dependency builds are endlessly performing extra work because of the explicit FORCE pre-requisites.

Any suggestions for options ?
  o At a minimum unit tests able to validate repackaging could easily detect misconfiguration or general building/packaging related problems.
  o configure could be a little more verbose about when non-standard builds are detected.  A 2nd option could be used to inhibit the warning { for build system use or people who work with l10n regularly }.
  o Maybe setup a simple tool that can be run that would parse a mozconfig file and report when non-standard builds are in play.  The tool could be used as a cron job if people forget to remove the l10n arguments with any frequency.
Comment 14 Joey Armstrong [:joey] 2012-04-27 07:56:20 PDT
Comment on attachment 618707 [details] [diff] [review]
Replace FORCE with file dependencies, FORCE should be conditional on repak tasks

Moving L10NBASEDIR into config/config.mk as a new define to simplify the test conditional across makefiles.
Comment 15 Axel Hecht [pto-Aug-30][:Pike] 2012-04-27 09:22:40 PDT
Joey, sadly I think your assumptions are off. Mind figuring out what the repack logic is doing across all our applications? Just claiming it should work differently is not right, unless you actually rewrite all the logic to work the way you like before changing the status quo here.
Comment 16 Ted Mielczarek [:ted.mielczarek] 2012-04-27 10:32:07 PDT
Comment on attachment 618707 [details] [diff] [review]
Replace FORCE with file dependencies, FORCE should be conditional on repak tasks

Review of attachment 618707 [details] [diff] [review]:
-----------------------------------------------------------------

Except for a few things, functionally this looks okay (comparing strictly to the original Makefile).

I was already 3/4 of the way through this review when you cancelled the request, so here it is...

::: mobile/android/base/locales/Makefile.in
@@ +39,5 @@
>  topsrcdir = @top_srcdir@
>  srcdir    = @srcdir@
>  VPATH     = @srcdir@
> +
> +relativesrcdir = $(subst $(topsrcdir)/,$(NULL),$(srcdir))

We have a whole bug on fixing this in the general case, bug 395019. I had a patch with this exact fix and it broke some things in srcdir builds, so it got backed out. I'd rather not futz with this unrelated change right now, feel free to take that other bug if you'd like to fix this.

@@ +55,5 @@
>  BRANDPATH = $(call core_abspath,$(DEPTH)/dist/bin/chrome/$(AB_CD)/locale/branding/brand.dtd)
>  else
>  BRANDPATH = $(call core_abspath,$(DIST)/xpi-stage/$(XPI_NAME)/chrome/$(AB_CD)/locale/branding/brand.dtd)
>  endif
> +# $(warnIfEmpty,AB_CD) # problem? dep paths are invalid w/o path elements

Either put this in or leave it out, but not commented out.

@@ +61,5 @@
>  DEFINES += -DAB_CD=$(AB_CD)
>  
> +dir-res-values = ../res/values
> +strings-xml    = $(dir-res-values)/strings.xml
> +strings-xml-in = $(dir $(srcdir))/strings.xml.in

Nit: you should probably use := here.

Also: using $(dir) to strip off the dir is a bit confusing. I think $(srcdir)/../ reads more clearly.

@@ +63,5 @@
> +dir-res-values = ../res/values
> +strings-xml    = $(dir-res-values)/strings.xml
> +strings-xml-in = $(dir $(srcdir))/strings.xml.in
> +
> +libs realchrome:: $(strings-xml) ;

I know it's in the original, but the semicolon here seems extraneous. I think it can probably be removed.

@@ +84,5 @@
>  
> +# Determine the ../res/values[-*]/ path
> +dir-strings-xml  = $(filter %/strings.xml,$(MAKECMDGOALS))
> +ifeq ($(NULL),$(strip $(dir-strings-xml)))
> +  dir-strings-xml = $(strings-xml)

I think you should name this intermediate variable differently. I got confused reading $(dir $(dir-strings-xml)) below, because it first gets the value of the filename, and then you strip it down to the dir.

@@ +95,5 @@
> +  $(STRINGSPATH) \
> +  $(SYNCSTRINGSPATH) \
> +  $(BOOKMARKSPATH) \
> +  $(NULL)
> +ifneq (,$(strip $(L10NBASEDIR))) # todo: test using a named function

You use $(NULL) up above but not here. I think you should decide on a convention and we'll use it.

Also, I thought you decided you couldn't use this test because you'd hit the $(error) case?

@@ +100,5 @@
> +  strings-xml-preqs += FORCE
> +endif
> +
> +$(dir-strings-xml)/strings.xml: $(strings-xml-preqs)
> +	$(NSINSTALL) -D $(dir-strings-xml)

Presumably you could use mkdir_deps here instead?

@@ +107,5 @@
>  	  -DBRANDPATH="$(BRANDPATH)" \
>  	  -DSTRINGSPATH="$(STRINGSPATH)" \
>  	  -DSYNCSTRINGSPATH="$(SYNCSTRINGSPATH)" \
>  	  -DBOOKMARKSPATH="$(BOOKMARKSPATH)" \
> +	  $(strings-xml-in) \

Should be able to use $< here.

@@ +113,5 @@
>  
>  include $(topsrcdir)/config/rules.mk
> +
> +clean::
> +	$(RM) $(strings-xml)

You can just append $(strings-xml) to GARBAGE.
Comment 17 Joey Armstrong [:joey] 2012-04-27 17:00:37 PDT
Created attachment 619216 [details] [diff] [review]
replace FORCE with file deps, conditional force for repackage requests

1) relativesrcdir reverted in favor of bug 395019.
2) Changed comment to WarnIfEmpty
3) initial dir-strings-xml assignment changed to strings-xml-bypath.
4) NULL comparison removed.  I'd prefer it visually but Kyle mentined ifdef (,$(xyz)) was in common use.
4.1) $(error case) - yes that was the reason for canceling the review.  Massaging to ignore $(error) can be done but gets ugly fast.  Replaced with an added define in config.mk.
5) yes on mkdir_deps, omited for now until umask + permissions can be verified against nsinstall -D output.
6) Changed macro to $<.  Logic may break if the pre-reqs are added or reordered but Preprocessor.py should catch it.
7) Thanks
Comment 18 Ted Mielczarek [:ted.mielczarek] 2012-04-30 05:30:49 PDT
(In reply to Axel Hecht [:Pike] from comment #11)
> I'm not sure that switching off the caching based on L10NBASEDIR is good, as
> many folks that had to try to repack l10n once probably have that left over
> in their .mozconfig.

It's no worse than the existing state of affairs, where everyone has to clobber these files all the time. People with misconfigured mozconfigs can fix them. This should fix the average case where people haven't mucked with their mozconfigs.

(In reply to Axel Hecht [:Pike] from comment #15)
> Joey, sadly I think your assumptions are off. Mind figuring out what the
> repack logic is doing across all our applications? Just claiming it should
> work differently is not right, unless you actually rewrite all the logic to
> work the way you like before changing the status quo here.

I don't think we need to boil the oceans here. Yes, the repack logic is complicated. We're trying to execute the smallest useful change here so that regular non-l10n-repack builds will see a benefit without having to delve into the repack logic. I think this patch satisfies those goals: it leaves the current state of affairs alone for repacks, but adds real dependencies for normal non-repack builds.
Comment 19 Ted Mielczarek [:ted.mielczarek] 2012-04-30 05:32:33 PDT
Comment on attachment 619216 [details] [diff] [review]
replace FORCE with file deps, conditional force for repackage requests

Review of attachment 619216 [details] [diff] [review]:
-----------------------------------------------------------------

Okay, this looks good. Thanks!
Comment 20 Joey Armstrong [:joey] 2012-04-30 06:32:28 PDT
And the try job passed:
https://tbpl.mozilla.org/?tree=Try&rev=a35b73aaa0b4
Comment 21 Ted Mielczarek [:ted.mielczarek] 2012-04-30 06:39:01 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/9bbdaf21a5eb
Comment 23 Aki Sasaki [:aki] 2012-05-07 15:07:58 PDT
This may have broken single locale repacks. (bug 751941)
Comment 24 Aki Sasaki [:aki] 2012-05-08 11:03:24 PDT
Reopening due to https://bugzilla.mozilla.org/show_bug.cgi?id=751941#c8 .

Note You need to log in before you can comment on or make changes to this bug.