If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

add configure option to l10n repositories

RESOLVED FIXED

Status

()

Toolkit
Build Config
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Pike, Assigned: Pike)

Tracking

({fixed1.9.0.2})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

9 years ago
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.
Attachment #329673 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 1

9 years ago
PS: testing on mozillabuild, gonna pick up the patch on my mac now.
Assignee: nobody → l10n
(Assignee)

Comment 2

9 years ago
testing passed on the mac, too.

Comment 3

9 years ago
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
Attachment #329673 - Flags: review?(ted.mielczarek) → review-

Comment 4

9 years ago
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 :)
(Assignee)

Comment 5

9 years ago
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.
Attachment #329673 - Attachment is obsolete: true
Attachment #329688 - Flags: review?(benjamin)
(Assignee)

Comment 6

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

Moving review over to ted.
Attachment #329688 - Flags: review?(benjamin) → review?(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.
Attachment #329688 - Flags: review?(ted.mielczarek) → review-
(Assignee)

Comment 8

9 years ago
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.
Attachment #329688 - Flags: review- → review+
(Assignee)

Comment 10

9 years ago
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.
Attachment #330745 - Flags: review?(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.
Attachment #330745 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 12

9 years ago
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.
Attachment #330745 - Attachment is obsolete: true
Attachment #330750 - Flags: review+
Attachment #330750 - Flags: approval1.9.0.2?
(Assignee)

Comment 13

9 years ago
Marking FIXED, as this is fixed on mozilla-central.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Depends on: 446630

Updated

9 years ago
No longer depends on: 446630

Updated

9 years ago
Depends on: 448338
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
Attachment #330750 - Flags: approval1.9.0.2? → approval1.9.0.2+
(Assignee)

Comment 15

9 years ago
Landed in CVS.
Keywords: fixed1.9.0.2
You need to log in before you can comment on or make changes to this bug.