Created attachment 329673 [details] [diff] [review] add --with-l10n-base to configure With hg, l10n repositories might be really anywhere. Well, with cvs, too, honestly. Let's add an option to configure to pick up the l10n base dir from wherever we want. I'm not going to change that it's the base dir, and that it expects the localization file to be in a subdirectory named with the locale code. For mozilla-central, I think it's perfectly fine to error on l10n builds without a given l10n-base. We should back-port this to 1.9, there we should have a default to ../l10n in configure.in, IMHO. Tentatively blocking bug 434289 for hg l10n builds.
PS: testing on mozillabuild, gonna pick up the patch on my mac now.
testing passed on the mac, too.
Comment on attachment 329673 [details] [diff] [review] add --with-l10n-base to configure The $(error) is not in a good place. You could put it earlier in config.mk like this: ifndef L10NBASEDIR L10NBASEDIR = $(error ...) endif You should probably either allow the path to be absolute (e.g. --with-l10nbasedir=/home/foo/my-locale) or explicitly test for an absolute path and error in configure. You probably also want to deal with the case where somebody does --without-l10n-base by mistake
The basic approach of this looks good to me, I'll port this over to the comm-central build system as soon as it lands :)
Created attachment 329688 [details] [diff] [review] add error handling, full paths I added error handling and path guessing. I somewhat copied what --with-xul-sdk does, with erroring on --with-l10n-base and --without-l10n-base, and if a specified base path doesn't exist. As I absolute the path no matter which (by pwd'ing), I removed the topsrcdir reference in EXPAND_LOCALE_SRCDIR. I moved the $(error ) out, too, that works like charm.
Comment on attachment 329688 [details] [diff] [review] add error handling, full paths Moving review over to ted.
Comment on attachment 329688 [details] [diff] [review] add error handling, full paths The only bit that worries me is this (not part of your patch, but in the context): ifdef LOCALE_SRCDIR MAKE_JARS_FLAGS += -c $(LOCALE_SRCDIR) endif If you don't specify a locale srcdir, then MAKE_JARS_FLAGS will wind up with a $(error) for a Makefile that has relativesrcdir set, so if a Makefile sets relativesrcdir and has a JAR_MANIFEST, seems like it would blow up. I don't know if we have any instances of that in our tree, but I can see someone being very surprised that their build breaks just from defining these two variables in a Makefile. Otherwise, the approach of the patch looks good.
The $(error) only happens for non-en-US builds, though, I did check that.
Comment on attachment 329688 [details] [diff] [review] add error handling, full paths Right, I missed the point there. EXPAND_LOCALE_SRCDIR explicitly checks for en-US. Carry on.
Created attachment 330745 [details] [diff] [review] use $(topsrcdir)/../l10n as default for 1.9 to be backwards compat Ted, the only thing new in this patch is the else dnl default to $(topsrcdir)/../l10n for 1.9 L10NBASEDIR='$(topsrcdir)/../l10n' fi part in configure.in. The rest of the patch landed on mozilla-central as http://hg.mozilla.org/mozilla-central/index.cgi/rev/613c0128f56d.
Comment on attachment 330745 [details] [diff] [review] use $(topsrcdir)/../l10n as default for 1.9 to be backwards compat You should be able to just get rid of the ifndef L10NBASEDIR / error bits, since it will always be defined in 1.9.
Created attachment 330750 [details] [diff] [review] updated patch without error in config.mk Carrying over review from attachment 330745 [details] [diff] [review]. This patch adds some flexibility on where 1.9 builds find l10n, with a default value that is the same as previously. Getting this patch in to 1.9.0.x can help us to keep as much of the build infrastructure in parallel between 1.9 and central as possible. Some localizers might find it useful for their homework, too.
Marking FIXED, as this is fixed on mozilla-central.
Comment on attachment 330750 [details] [diff] [review] updated patch without error in config.mk Approved for 18.104.22.168. Please land in CVS. a=ss
Landed in CVS.