466570 - create an l10n repackaging system for fennec

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Axel Hecht [:Pike]]

We need to have an l10n repackaging system similar to what we do for Firefox for Fennec.

I'm porting over browser right now, and I got as far as downloading and unpacking so far.

Still needs a good deal of work, though.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

it would help me learning this if you could upload WIP when you feel comfortable.

Comment 2

[Axel Hecht [:Pike]]

Not too bad, I got a testing build up, http://l10n.mozilla.org/~axel/fennec-1.0a2pre.pl.linux-arm.tar.bz2, with one or two known bugs, 

pl/mobile/chrome
  region.properties
      +browser.search.order.2
      -gecko.handlerService.schemes.mailto.2.name
      -gecko.handlerService.schemes.mailto.2.uriTemplate
      -gecko.handlerService.schemes.mailto.3.name
      -gecko.handlerService.schemes.mailto.3.uriTemplate
  shortcuts.properties
      -cmd_openLocation
      +cmd_openLocation.name
pl:
keys: 379
unchanged: 297
changed: 3320
obsolete: 5
missing: 2
91% of entries changed


I'll come up with a patch shortly, the langpacks still need work, and the build above might need setting general.useragent.locale = pl manually still.

Comment 3

[Axel Hecht [:Pike]]

Attachment: add l10n.mk and use it for fennec only for now

Here's what I think we should do for l10n repack Makefiles in general, and fennec in particular.

I've factored a lot of content from browser/locales/Makefile.in into an l10n.mk, which I currently added to mobile-browser/locales for the sake of simplicity of the patch. It should really go into toolkit/locales, IMHO.

I didn't put the windows installer logic into l10n.mk as I'm not sure how much we actually want to share that piece.

For the fennec side, I'd like to get reviews for the filter.py, but also for the langpack EID, for which I used langpack-$(AB_CD)@firefox-mobile.mozilla.org.

There are some details that are a bit heavier than in browser, in particular, I allow for defines.inc and list.txt to be missing, which should make starting localizations easier.

Apart from the location of l10n.mk, this looks pretty solid to me, what do you guys think?

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 351934 [details] [diff] [review]
add l10n.mk and use it for fennec only for now

>diff --git a/locales/Makefile.in b/locales/Makefile.in

> 
> include $(DEPTH)/config/autoconf.mk
>+include $(topsrcdir)/config/config.mk

Question: Should this be in the Makefile twice?
(I'm just learning makefiles, so I'm not sure)

> 
>-vpath %.xml $(srcdir)/$(MOZ_UI_LOCALE)/searchplugins
>+vpath %.xml @srcdir@/en-US/searchplugins
>+vpath %.xml $(LOCALE_SRCDIR)/searchplugins

Question: Should this be in the Makefile twice?
(I'm just learning makefiles, so I'm not sure)

> 
>-SEARCH_PLUGINS = $(shell cat $(srcdir)/$(MOZ_UI_LOCALE)/searchplugins/list.txt)
>+PREF_JS_EXPORTS = $(firstword $(wildcard $(LOCALE_SRCDIR)/firefox-l10n.js) \
>+                              @srcdir@/en-US/firefox-l10n.js )

can we use "mobile-l10n.js" for now? we use "mobile.js" for our regular prefs file
(need to change in several files)

>diff --git a/locales/filter.py b/locales/filter.py

>+  if mod != "mobile":
>+    # we only have exceptions for browser and extensions/spellcheck

nit: "mobile" not "browser" in the comment?

Looks good (for the parts I am ok with looking at). Ted's review is more important.

Comment 5

[Axel Hecht [:Pike]]

config.mk is included from rules.mk, but I needed it early. Not exactly sure why, but browser/locales/Makefile.in does so, too. It's wallpapered against being included twice in rules.mk, so that shouldn't be a problem.

The double vpath's are for picking up searchplugins from en-US if available, and helps us to not duplicate the google.xml plugin a few dozen times. Again something we use in browser/locales already.

I'll change the comments and the file name of the l10n prefs file.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Thanks for the explanation Axel

Comment 7

[(not currently active) Ted Mielczarek]

(In reply to comment #5)
> config.mk is included from rules.mk, but I needed it early. Not exactly sure
> why, but browser/locales/Makefile.in does so, too. It's wallpapered against
> being included twice in rules.mk, so that shouldn't be a problem.

config.mk is generally intended to be included after the body of your Makefile because it defines some variables based on the values of other variables you've set in the Makefile. Most of them are related to compiling code though, so it's probably not an issue here.

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 351934 [details] [diff] [review]
add l10n.mk and use it for fennec only for now

+DEFINES += \
+	-DMOZ_LANGPACK_EID=langpack-$(AB_CD)@firefox-mobile.mozilla.org \
+	-DPACKAGE=browser \
+	$(NULL)

We don't tend to use actual tabs for line continuations anymore, we just do one or two space indent.

+PREF_JS_EXPORTS = $(firstword $(wildcard $(LOCALE_SRCDIR)/firefox-l10n.js) \
+                              @srcdir@/en-US/firefox-l10n.js )

Feels like this would be clearer as:
$(or $(wildcard ...) @srcdir@/en-US... )
See http://www.gnu.org/software/make/manual/make.html#Conditional-Functions

Also, I think you should be using $(srcdir) in the various places that you currently have @srcdir@. It seems a bit less confusing.

+++ b/locales/en-US/firefox-l10n.js

This is in the mobile repo, right? If so, I don't think that it ought to say Firefox.

+++ b/locales/filter.py

Should probably have some license boilerplate.

+  # ignore anything but toolkit and mobile, which is our local repo checkout
+  # name
+  if mod not in ("netwerk", "dom", "toolkit", "security/manager",
+                 "mobile"):

That comment seems seriously misleading.

+++ b/locales/l10n.mk

Are you intending that this will be copied to each app's locales/ dir, or would you like to keep it in a common directory like toolkit?

+PWD := $(shell pwd)

You can use $(CURDIR) here, it's always set to the directory in which make was invoked.

+core_abspath = $(if $(findstring :,$(1)),$(1),$(if $(filter /%,$(1)),$(1),$(PWD)/$(1)))

This is sort of awful. Is this really the only way to accomplish this? :-(

+	if test -d $(DIST)/l10n-stage; then \
+	  $(RM) -r -v $(DIST)/l10n-stage; \
+	fi

I wouldn't bother with the "test -d" in these commands, just use rm -rf and unilaterally try removing it.

+	make clobber-zip AB_CD=en-US

Should be $(MAKE).

+	cp ../installer/windows/l10ngen/helper.exe $(STAGEDIST)/uninstall

What's the relative path here? I'd be happier if this used $(DEPTH) or something and then a full path.

r+ mostly because I don't want to have to review this again.

I think these makefiles are getting unmaintainable. We desperately need to rewrite this in python.

Comment 9

[Axel Hecht [:Pike]]

(In reply to comment #8)
> +++ b/locales/l10n.mk
> 
> Are you intending that this will be copied to each app's locales/ dir, or would
> you like to keep it in a common directory like toolkit?

I would like to put it in toolkit/locales. Would that be OK with you?

> +core_abspath = $(if $(findstring :,$(1)),$(1),$(if $(filter
> /%,$(1)),$(1),$(PWD)/$(1)))
> 
> This is sort of awful. Is this really the only way to accomplish this? :-(

It's just copied and pasted. I'd prefer to not see if I can do something else for all build platforms.

I'll fix the other comments.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Just a ping for an updated patch

Comment 11

[Axel Hecht [:Pike]]

Attachment: hg cp browser/locales/Makefile.in toolkit/locales/l10n.mk and strip [checked in]

Ted, just to safe guard, this is an easier patch that I created by doing an hg copy of browser/locales/Makefile.in and stripping everything that was fx specific.

The rest of the diff is to address some of your review comments, and to prefetch fixing bug 472431 for zips. I moved the helper.exe into a variable callback, so that it doesn't look like it's toolkit specific stuff.

And yes, that implies that there will be at least one more patch to the browser one to actually use that ;-)

Comment 12

[Axel Hecht [:Pike]]

Attachment: fennec l10n, using toolkit/locales/l10n.mk

This the fennec side of things. I tested this with the cs localization on a linux build, and with repackaging the arm bz2 from ftp.

I don't yet do the deb.pkg step, that should be a follow up from someone that knows these.

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 357195 [details] [diff] [review]
hg cp browser/locales/Makefile.in toolkit/locales/l10n.mk and strip [checked in]

I wish we could rewrite this all in Python. How much would I have to bribe you to do that? :)

Comment 14

[Axel Hecht [:Pike]]

Comment on attachment 357195 [details] [diff] [review]
hg cp browser/locales/Makefile.in toolkit/locales/l10n.mk and strip [checked in]

http://hg.mozilla.org/mozilla-central/rev/508324a2a77a, landed the toolkit part.

Giving it a bit to complete the cycles before landing on mobile-browser.

Comment 15

[Axel Hecht [:Pike]]

Landed on mobile-browser, too, http://hg.mozilla.org/mobile-browser/rev/caf66c67d509.

Resolving FIXED.

We should move the creation of an all-locales file to a follow up bug, as well as some makefile foo to actually create .deb files from the tarballs.

Comment 16

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: Mobile linux-arm build bustage

mobile linux-arm builds have been busted since this landed, see attachment for hang, at line 54 of locales/Makefile.in I think).

Comment 17

[Axel Hecht [:Pike]]

This works for me. Can you attach an autoconf.mk from the build, and the locales/Makefile? For both, the generated versions in the build dir.

Comment 18

[Axel Hecht [:Pike]]

... gavin tested on scratchbox, too, without any problems.

Comment 19

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: objdir/mobile/config/autoconf.mk

Path to objdir is /scratchbox/users/cltbld/home/cltbld/build/mozilla-central/ outside of scratchbox; /home/cltbld/build/mozilla-central/ inside scratchbox

Comment 20

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: objdir/mobile/mobile/locales/Makefile

Comment 21

[Axel Hecht [:Pike]]

Ted, the bustage here is due to $(or ) being a new function in GNU Make 3.81. The refplatform for mobile apparently has 3.80, and the build requirements docs say 3.79.1 (for linux). That kinda screws us, but I could undo your review comment 8 and use $(firstword $(wildcard ) en-US/path) again.

What's the right fix? Backport, update the refplatform, or both?

Comment 22

[Axel Hecht [:Pike]]

Backed out, http://hg.mozilla.org/mobile-browser/pushloghtml?changeset=319bb86e2b60

Comment 23

[(not currently active) Ted Mielczarek]

The build requirements doc must be out of date, because we require 3.80 these days. That being said, if the function doesn't exist in 3.80, feel free to ignore my review comment. IIRC, the SeaMonkey tinderboxes are using 3.80 as well.

Comment 24

[Axel Hecht [:Pike]]

Attachment: bustage fix for l10n.mk

Use $(firstword a b) instead of $(or a, b) again, to fix the bustage on make 3.80. This is a patch against the l10n.mk that's still in the tree, I didn't back that one out.

Comment 25

[Axel Hecht [:Pike]]

Attachment: fennec l10n, bustage fixed for make 3.80, for updated l10n.mk

Comment 26

[Axel Hecht [:Pike]]

Comment on attachment 357197 [details] [diff] [review]
fennec l10n, using toolkit/locales/l10n.mk

Obsoleting 3.81 patch, I suggest to look at the interdiff https://bugzilla.mozilla.org/attachment.cgi?oldid=357197&action=interdiff&newid=358828&headers=1 to see the changes for 3.80

Comment 27

[Axel Hecht [:Pike]]

... got rs=ted on irc on the 3.80 fixes.

http://hg.mozilla.org/mozilla-central/rev/e810c48a14f7 is for the central landing.

Comment 28

[Mark Finkle (:mfinkle) (use needinfo?)]

all is well now

Comment 29

[Anton Rogaynis (antonr)]

build-tree/mozilla/toolkit/locales/l10n.mk: No such file or directory
make[6]: *** No rule to make target `build-tree/mozilla/toolkit/locales/l10n.mk'.  Stop.

build is broken.

Comment 30

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #29)
> build-tree/mozilla/toolkit/locales/l10n.mk: No such file or directory
> make[6]: *** No rule to make target
> `build-tree/mozilla/toolkit/locales/l10n.mk'.  Stop.
> 
> build is broken.

A | hg pull -u | on the mozilla-central part of your tree should fix that.

Comment 31

[:Gavin Sharp [email: [e-mail]]]

What are the chances of this landing on 1.9.1? Fennec is planning to ship from 1.9.1, but this is currently preventing it from building there.

Comment 32

[Axel Hecht [:Pike]]

I intend to look at that next week.