Closed Bug 466570 Opened 11 years ago Closed 11 years ago

create an l10n repackaging system for fennec

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
fennec1.0b1

People

(Reporter: Pike, Assigned: Pike)

References

Details

Attachments

(6 files, 2 obsolete files)

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.
it would help me learning this if you could upload WIP when you feel comfortable.
Target Milestone: --- → Fennec A3
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.
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?
Attachment #351934 - Flags: review?(ted.mielczarek)
Attachment #351934 - Flags: review?(mark.finkle)
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.
Attachment #351934 - Flags: review?(mark.finkle) → review+
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.
Thanks for the explanation Axel
(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 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.
Attachment #351934 - Flags: review?(ted.mielczarek) → review+
(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.
Just a ping for an updated patch
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 ;-)
Attachment #351934 - Attachment is obsolete: true
Attachment #357195 - Flags: review?(ted.mielczarek)
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.
Attachment #357197 - Flags: review?(mark.finkle)
Attachment #357197 - Flags: review?(mark.finkle) → review+
Attachment #357195 - Flags: review?(ted.mielczarek) → review+
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 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.
Attachment #357195 - Attachment description: hg cp browser/locales/Makefile.in toolkit/locales/l10n.mk and strip → hg cp browser/locales/Makefile.in toolkit/locales/l10n.mk and strip [checked in]
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.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
mobile linux-arm builds have been busted since this landed, see attachment for hang, at line 54 of locales/Makefile.in I think).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
... gavin tested on scratchbox, too, without any problems.
Path to objdir is /scratchbox/users/cltbld/home/cltbld/build/mozilla-central/ outside of scratchbox; /home/cltbld/build/mozilla-central/ inside scratchbox
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?
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.
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 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
Attachment #357197 - Attachment is obsolete: true
... got rs=ted on irc on the 3.80 fixes.

http://hg.mozilla.org/mozilla-central/rev/e810c48a14f7 is for the central landing.
all is well now
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
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.
(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.
Blocks: 489313
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.
I intend to look at that next week.
You need to log in before you can comment on or make changes to this bug.