489313 - Use toolkit/locales/l10n.mk for fx, land on the 1.9.1 branch

Status: RESOLVED FIXED

(Toolkit Graveyard :: Build Config, defect)

Description

[Axel Hecht [:Pike]]

The l10n.mk that I used successfully on mozilla-central for fennec should be good to use for Firefox, too.

We might want to land the status quo on the 1.9.1 branch before that, though, if Fennec decides to build off of 1.9.1 in the short term.

Comment 1

[Aki Sasaki (not active)]

We're planning on shipping Fennec off of 1.9.1 so this will be needed for Fennec 1.0 l10n.

Comment 2

[Axel Hecht [:Pike]]

Taking. I kept myself from fixing the pattern rules in Makefile.in to the firstword usecases in fennec's one, for the most part. That makes the patch a lot easier to read I hope. I might come up with a follow up to fix that later.

Comment 3

[Axel Hecht [:Pike]]

Attachment: Use l10n.mk, drop duplicate rules

run_for_effects_too is the part of run_for_effects that is not in l10n.mk already, aka the branding part.

The defines get changed to just amend those in l10n.mk.

I couldn't resist the one edit of using firstword for firefox-l10n.js already.

The rest is just removing rules that are literally the same in l10n.mk, and using l10n.mk.

Tested on a french mac build. The windows installer files are not touched at all, so all should be good.

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 377154 [details] [diff] [review]
Use l10n.mk, drop duplicate rules

run_for_effects_too := if ! test -d $(DIST)/branding; then $(NSINSTALL) -D $(DIST)/branding; fi)

It'd be nice if we could make these explicit dependencies instead of evaluating for side effects, something like:
http://mxr.mozilla.org/mozilla-central/source/config/rules.mk#1571

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

I'd rather you used $(srcdir) instead of @srcdir@, since the former is actual make syntax.

Comment 5

[Axel Hecht [:Pike]]

Fixed on mozilla-central, http://hg.mozilla.org/mozilla-central/rev/d2a23f2c0020.

We need this for fennec l10n, but I'll need a branch patch including l10n.mk for that. I'll flag the 1.9.1 flags once I have the patch.

Comment 6

[Axel Hecht [:Pike]]

Doh, I did something so stupid, it doesn't even break anything. Ran across that while trying to do the 1.9.1 patch.

run_for_effects_too := if ! test -d $(DIST)/branding; then $(NSINSTALL) -D $(DIST)/branding; fi)

doesn't contain an initial $(shell

So, if I have to fix that again anyway, let's do it right. Patch coming up.

Comment 7

[Axel Hecht [:Pike]]

Attachment: Use dependency for $(DIST)/branding instead of run_for_effects

Follow up patch, turning the $(shell) side effect thingie into a dependency.

I'm not doing this for the one in l10n.mk slightly on purpose, I need ABSDIST to be right early on, not sure if there's a good way around that.

Tested on a mac without the deps and the run_for_effects, which breaks on unpack, fixed by adding just the deps and unpacked and created an installer successfully.

Comment 8

[Axel Hecht [:Pike]]

PS: Note to self and the approval committee, I'll be taking the $(CURDIR) piece of bug 483856 when doing the 1.9.1 patch, too, so the only difference between trunk and branch will be 

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

which moved to config.mk in that patch. :bs said that he's not driving that over to 1.9.1, so I'm leaving core_abspath as it is.

Comment 9

[Chris Cooper [:coop] (he/him)]

Comment on attachment 383452 [details] [diff] [review]
Use dependency for $(DIST)/branding instead of run_for_effects

Mac l10n nightly repacks are currently broken on m-c until this patch goes in.

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 383452 [details] [diff] [review]
Use dependency for $(DIST)/branding instead of run_for_effects

+$(DIST)/branding:
+	$(NSINSTALL) -D $(DIST)/branding

Might just use $@ here to keep from duplicating.

Comment 11

[Axel Hecht [:Pike]]

http://hg.mozilla.org/mozilla-central/rev/a437dbe2197e, FIXED.

Still need to write a patch for 1.9.1.

Comment 12

[Axel Hecht [:Pike]]

Attachment: update browser/locales/Makefile.in on 1.9.1, add l10n.mk

This updates browser/locales/Makefile.in mostly to the status of mozilla-central.

It includes parts of bug 483856 and all of bug 496196.

The only remaining diff is the one mentioned in comment 8,

--- ../../mozilla-central/browser/locales/Makefile.in	2009-07-02 10:11:00.000000000 +0200
+++ browser/locales/Makefile.in	2009-07-06 13:14:38.000000000 +0200
@@ -77,6 +77,7 @@
 APP_VERSION := $(shell cat $(srcdir)/../config/version.txt)
 
 PWD := $(CURDIR)
+core_abspath = $(if $(findstring :,$(1)),$(1),$(if $(filter /%,$(1)),$(1),$(PWD)/$(1)))
 
 # These are defaulted to be compatible with the files the wget-en-US target
 # pulls. You may override them if you provide your own files. You _must_


Carrying over reviews.

Comment 13

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

Axel: Is this only a 1.9.2 thing now or do we still need it on 1.9.1?

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Axel: dunno if you saw Sam's question there, but it's a large patch so we're wondering if it's needed on mozilla-1.9.1 or if it can wait for the mozilla-1.9.2 branch.

Comment 15

[Axel Hecht [:Pike]]

Yes, we should take this on 1.9.1.

This isn't such a big deal in terms of patches, but it makes porting of build fixes from trunk to 1.9.1 easier and it's going to make it easier to port build changes for the stable branch onto 1.9.2 once 3.6 ships.

Comment 16

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

Comment on attachment 386972 [details] [diff] [review]
update browser/locales/Makefile.in on 1.9.1, add l10n.mk

Approved for 1.9.1.2. a=ss for release-drivers

Please land on mozilla-1.9.1 and use the ".2-fixed" option of the "status1.9.1" flag.

Comment 17

[Axel Hecht [:Pike]]

Landed, http://hg.mozilla.org/releases/mozilla-1.9.1/rev/562e14bff99f.

Gonna do the .2-fixed status as soon as the boxens cycle and some l10n builds came through.

Comment 18

[Axel Hecht [:Pike]]

Fixed .2. Unrequesting fennec, as I think that's not coming from 1.9.1 anymore.

Comment 19

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

Comment on attachment 386972 [details] [diff] [review]
update browser/locales/Makefile.in on 1.9.1, add l10n.mk

>diff --git a/toolkit/locales/l10n.mk b/toolkit/locales/l10n.mk
>+ifdef MOZ_MAKE_COMPLETE_MAR
>+	$(MAKE) -C $(DEPTH)/tools/update-packaging full-update AB_CD=$(AB_CD) \
>+	  MOZ_PKG_PRETTYNAMES=$(MOZ_PKG_PRETTYNAMES) \
>+	  PACKAGE_BASE_DIR="$(_ABS_DIST)/l10n-stage" \
>+	  DIST="$(_ABS_DIST)"
>+	$(MAKE) generate-snippet-$(AB_CD)  
>+endif

Adding the call to generate-snippet target without actually landing tools/update-packaging/generatesnippet.py on the branch broke 3.5.1 release builds. Congratulations.

Comment 20

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

3.5.2 even.

Comment 21

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

Raising priority because this has broken FF3.5.2 production. 

nthomas is trying a workaround for now, but this should be fixed asap.

Comment 22

[Axel Hecht [:Pike]]

Attachment: not call generate-snippets until we actually land that

Ouch, sorry. I lost track of what landed and what did not.

Guess the safest way to cut this is to comment out the generate-snippet call until we actually do something on the branch.

Nick?

Comment 23

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

Comment on attachment 391542 [details] [diff] [review]
not call generate-snippets until we actually land that

On GECKO1912_20090729_RELBRANCH:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d57f05bcdbe1

On default:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a83d5a98cdc6

Comment 24

[[e-mail] (:bhearsum)]

Comment on attachment 386972 [details] [diff] [review]
update browser/locales/Makefile.in on 1.9.1, add l10n.mk

This has been backed out of 1.9.1 because it breaks Mac DMG packaging.

Comment 25

[[e-mail] (:bhearsum)]

Comment on attachment 391542 [details] [diff] [review]
not call generate-snippets until we actually land that

This has been backed out of 1.9.1 because it breaks Mac DMG packaging.

Comment 26

[Axel Hecht [:Pike]]

Attachment: put the branding vars before l10n.mk and thus packager.mk

Such a looser, apparently the variable defines for the mac image stuff need to be before packager.mk. That's coming in through l10n.mk, so in front of that.

Comment 27

[[e-mail] (:bhearsum)]

Turns out this broke win32 repacks for 3.5.2 too - the win32 MARs got the en-US helper.exe instead of the localized one. The installers were OK.

Comment 28

[Axel Hecht [:Pike]]

This is not fixed .2, nor FIXED on trunk. Reopening.

Putting back on the triage for 1.9.1, I think we should start with a backout there. So far we only backed it out on the relbranch. I have a grip on the mac branding stuff by now, but not on the uninstaller.

Comment 29

[Axel Hecht [:Pike]]

Comment on attachment 391610 [details] [diff] [review]
put the branding vars before l10n.mk and thus packager.mk

Found the helper.exe issue, I think. Working on a new patch.

Comment 30

[Axel Hecht [:Pike]]

Attachment: move variables for mac up, add UNINSTALLER_PACKAGE_HOOK

This should do it. I moved the vars for the mac branding up, and added a UNINSTALLER_PACKAGE_HOOK, which I apparently added to l10n.mk just to have that helper.exe logic.

I'm not able to actually compile on windows, so I haven't been able to test the MAR code path. I expect this to be fine, but I could use some help in testing.

Comment 31

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

(In reply to comment #30)

> I'm not able to actually compile on windows, so I haven't been able to test the
> MAR code path. I expect this to be fine, but I could use some help in testing.

Can you verify using TryServer?

Comment 32

[Axel Hecht [:Pike]]

Nope, tryserver doesn't do l10n repacks, and not mars either.

Comment 33

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

We need to get this fixed on 1.9.1 one way or another (backing out is acceptable) before code freeze on August 11. Who's on point to get this fixed? Axel?

Comment 34

[Axel Hecht [:Pike]]

Backed out on 1.9.1, http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e03876b4e587.

Comment 35

[(not currently active) Ted Mielczarek]

Comment on attachment 391859 [details] [diff] [review]
move variables for mac up, add UNINSTALLER_PACKAGE_HOOK

+ifneq (,$(filter mac cocoa,$(MOZ_WIDGET_TOOLKIT)))

While you're here, just make this ifeq (cocoa,$(MOZ_WIDGET_TOOLKIT)).

Comment 36

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

Comment on attachment 386972 [details] [diff] [review]
update browser/locales/Makefile.in on 1.9.1, add l10n.mk

Clearing this approval flag since the patch was backed out.

Comment 37

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

This bug is no longer blocking 1.9.1.3 since the patch has been backed out. This bug is now "wanted" on 1.9.1, meaning we'll consider a well-tested patch. Can we get tests for any such patch so we don't break packaging in the future?

Comment 38

[Axel Hecht [:Pike]]

http://hg.mozilla.org/mozilla-central/rev/20fe6fe0d592, marking FIXED again.

Now we need to get the bustage fixes on to 1.9.2.

Comment 39

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

(In reply to comment #37)
> Can we get tests for any such patch so we don't break packaging in the future?

Axel: 

Not sure how easy tests would be for this patch. However, if you tell us when this patch is landed on mozilla1.9.1, and again when landed on mozilla-1.9.2, we can do a fake release in staging for both branches to verify all is ok.

Comment 40

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

beltzner/ss: gentle ping about approval192?

Comment 41

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 391859 [details] [diff] [review]
move variables for mac up, add UNINSTALLER_PACKAGE_HOOK

a192=beltzner

Comment 42

[Axel Hecht [:Pike]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a6db2a107e5c, landed on 1.9.2.

Comment 43

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

Per discussion with Axel in irc, this needs to also be landed in mozilla-1.9.1.

Axel, when you have reviewed patch approved and landed, please let us know.

Comment 44

[Armen [:armenzg]]

bug 489313 will test that the patch does not affect releases. Once tested it we can land it on 1.9.1