Closed Bug 489313 Opened 11 years ago Closed 11 years ago

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

Categories

(Toolkit Graveyard :: Build Config, defect, blocker)

defect
Not set
blocker

Tracking

(status1.9.2 beta1-fixed, status1.9.1 wanted)

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta1-fixed
status1.9.1 --- wanted

People

(Reporter: Pike, Assigned: Pike)

References

Details

(Whiteboard: [actually fixed in mozilla1.9.2a2 on m-1.9.2])

Attachments

(5 files, 1 obsolete file)

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.
We're planning on shipping Fennec off of 1.9.1 so this will be needed for Fennec 1.0 l10n.
Flags: wanted-fennec1.0?
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.
Assignee: nobody → l10n
Status: NEW → ASSIGNED
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.
Attachment #377154 - Flags: review?(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.
Attachment #377154 - Flags: review?(ted.mielczarek) → review+
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.
Status: ASSIGNED → RESOLVED
tracking-fennec: --- → ?
Closed: 11 years ago
Resolution: --- → FIXED
Blocks: 496196
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #383452 - Flags: review?(ted.mielczarek)
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 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.
Severity: normal → major
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.
Attachment #383452 - Flags: review?(ted.mielczarek) → review+
http://hg.mozilla.org/mozilla-central/rev/a437dbe2197e, FIXED.

Still need to write a patch for 1.9.1.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
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.
Attachment #386972 - Flags: review+
Attachment #386972 - Flags: approval1.9.1.1?
Attachment #386972 - Flags: approval1.9.1.1? → approval1.9.1.2?
blocking1.9.1: --- → ?
Axel: Is this only a 1.9.2 thing now or do we still need it on 1.9.1?
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.
blocking1.9.1: ? → -
status1.9.1: --- → ?
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 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.
Attachment #386972 - Flags: approval1.9.1.2? → approval1.9.1.2+
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.
Fixed .2. Unrequesting fennec, as I think that's not coming from 1.9.1 anymore.
tracking-fennec: ? → ---
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.
3.5.2 even.
Raising priority because this has broken FF3.5.2 production. 

nthomas is trying a workaround for now, but this should be fixed asap.
Severity: major → blocker
blocking1.9.1: - → ?
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?
Attachment #391542 - Flags: review?(nthomas)
Attachment #391542 - Flags: review?(nthomas) → review+
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 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.
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.
Attachment #391610 - Flags: review?(ted.mielczarek)
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.
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.
Status: RESOLVED → REOPENED
Flags: wanted-fennec1.0?
Resolution: FIXED → ---
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.
Attachment #391610 - Attachment is obsolete: true
Attachment #391610 - Flags: review?(ted.mielczarek)
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.
Attachment #391859 - Flags: review?(ted.mielczarek)
(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?
Nope, tryserver doesn't do l10n repacks, and not mars either.
blocking1.9.1: ? → .3+
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?
Attachment #391859 - Flags: review?(ted.mielczarek) → review+
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)).
No longer blocks: 496196
Blocks: 480081
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.
Attachment #386972 - Flags: approval1.9.1.2+
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?
blocking1.9.1: .3+ → ---
http://hg.mozilla.org/mozilla-central/rev/20fe6fe0d592, marking FIXED again.

Now we need to get the bustage fixes on to 1.9.2.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Flags: blocking1.9.2?
Resolution: --- → FIXED
Attachment #391859 - Flags: approval1.9.2?
(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.
beltzner/ss: gentle ping about approval192?
Comment on attachment 391859 [details] [diff] [review]
move variables for mac up, add UNINSTALLER_PACKAGE_HOOK

a192=beltzner
Attachment #391859 - Flags: approval1.9.2? → approval1.9.2+
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a6db2a107e5c, landed on 1.9.2.
Flags: blocking1.9.2?
Keywords: fixed1.9.2
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.
Depends on: 512300
bug 489313 will test that the patch does not affect releases. Once tested it we can land it on 1.9.1
No longer depends on: 512300
Blocks: 511460
Flags: in-testsuite-
Whiteboard: [actually fixed in mozilla1.9.2a2 on m-1.9.2]
Target Milestone: --- → mozilla1.9.3a1
Version: unspecified → Trunk
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.