445328 - add configure option to l10n repositories

Status: RESOLVED FIXED

(Toolkit Graveyard :: Build Config, defect)

Description

[Axel Hecht [:Pike]]

Attachment: 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.

Comment 1

[Axel Hecht [:Pike]]

PS: testing on mozillabuild, gonna pick up the patch on my mac now.

Comment 2

[Axel Hecht [:Pike]]

testing passed on the mac, too.

Comment 3

[Benjamin Smedberg]

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

Comment 4

[Robert Kaiser]

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 :)

Comment 5

[Axel Hecht [:Pike]]

Attachment: 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 6

[Axel Hecht [:Pike]]

Comment on attachment 329688 [details] [diff] [review]
add error handling, full paths

Moving review over to ted.

Comment 7

[(not currently active) Ted Mielczarek]

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.

Comment 8

[Axel Hecht [:Pike]]

The $(error) only happens for non-en-US builds, though, I did check that.

Comment 9

[(not currently active) Ted Mielczarek]

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.

Comment 10

[Axel Hecht [:Pike]]

Attachment: 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 11

[(not currently active) Ted Mielczarek]

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.

Comment 12

[Axel Hecht [:Pike]]

Attachment: 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.

Comment 13

[Axel Hecht [:Pike]]

Marking FIXED, as this is fixed on mozilla-central.

Comment 14

[Samuel Sidler (old account; do not CC)]

Comment on attachment 330750 [details] [diff] [review]
updated patch without error in config.mk

Approved for 1.9.0.2. Please land in CVS. a=ss

Comment 15

[Axel Hecht [:Pike]]

Landed in CVS.