Last Comment Bug 554993 - Use l10n.mk in SeaMonkey (suite/locales/Makefile.in)
: Use l10n.mk in SeaMonkey (suite/locales/Makefile.in)
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Build Config (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: seamonkey2.1b3
Assigned To: Justin Wood (:Callek) (Away until Aug 29)
:
Mentors:
Depends on: 489313 587984 589232 636076
Blocks: 547518
  Show dependency treegraph
 
Reported: 2010-03-25 10:25 PDT by Serge Gautherie (:sgautherie)
Modified: 2011-03-07 00:23 PST (History)
6 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
(Av1) Just port it. (10.38 KB, patch)
2010-03-25 11:03 PDT, Serge Gautherie (:sgautherie)
kairo: review-
Details | Diff | Splinter Review
(Av1a) Just port bug 489313 (10.97 KB, patch)
2011-02-22 19:12 PST, Serge Gautherie (:sgautherie)
no flags Details | Diff | Splinter Review
WIP (8.69 KB, patch)
2011-02-23 21:44 PST, Justin Wood (:Callek) (Away until Aug 29)
no flags Details | Diff | Splinter Review
WIP 2 (12.54 KB, patch)
2011-02-23 22:27 PST, Justin Wood (:Callek) (Away until Aug 29)
no flags Details | Diff | Splinter Review
v1.0 (14.46 KB, patch)
2011-02-26 09:52 PST, Justin Wood (:Callek) (Away until Aug 29)
bugspam.Callek: review-
Details | Diff | Splinter Review
v2.0 (15.50 KB, patch)
2011-03-02 22:41 PST, Justin Wood (:Callek) (Away until Aug 29)
kairo: review-
Details | Diff | Splinter Review
v3.0 add langpack target back, change mac DMG creation (30.08 KB, patch)
2011-03-05 22:15 PST, Justin Wood (:Callek) (Away until Aug 29)
no flags Details | Diff | Splinter Review
v3.1 (30.15 KB, patch)
2011-03-06 05:59 PST, Justin Wood (:Callek) (Away until Aug 29)
no flags Details | Diff | Splinter Review
v3.2 -- This time without windows line endings. (15.54 KB, patch)
2011-03-06 17:45 PST, Justin Wood (:Callek) (Away until Aug 29)
kairo: review+
Details | Diff | Splinter Review

Description Serge Gautherie (:sgautherie) 2010-03-25 10:25:09 PDT

    
Comment 1 Serge Gautherie (:sgautherie) 2010-03-25 11:03:19 PDT
Created attachment 434915 [details] [diff] [review]
(Av1) Just port it.

That should be it: please, double-check.
Comment 2 Robert Kaiser 2010-03-25 13:44:57 PDT
Comment on attachment 434915 [details] [diff] [review]
(Av1) Just port it.

I'd like through testing of this, and I don't feel the reviewer should do all the testing here. I don't expect this to work - actually, I remember Axel telling that it probably doesn't, that's why I never even looked into porting this.
Comment 3 Serge Gautherie (:sgautherie) 2010-03-25 16:59:11 PDT
(In reply to comment #2)

> (From update of attachment 434915 [details] [diff] [review])
> I'd like through testing of this, and I don't feel the reviewer should do all
> the testing here.

Sorry, I know too little about l10n process to do more than prepare this patch.

> I don't expect this to work - actually, I remember Axel
> telling that it probably doesn't, that's why I never even looked into porting
> this.

Axel, what is the current status of this code?
Comment 4 Axel Hecht [pto-Aug-30][:Pike] 2010-03-26 10:04:57 PDT
I recall that seamonkey shipped something different as language pack than Firefox does, in particular, including the searchplugins and dictionaries. You don't ship the latter no more in langpacks, right? Not sure about searchplugins.

It'd mostly be differences in what langpacks really are, if there are problems.

Not sure if there are any treats that wouldn't work otherwise, that's up for running a bunch of test repacks, possibly including the release cases (which are more eager to break due to tricks with input and output and branding etc.)
Comment 5 Robert Kaiser 2010-03-26 10:54:15 PDT
(In reply to comment #4)
> I recall that seamonkey shipped something different as language pack than
> Firefox does, in particular, including the searchplugins and dictionaries. You
> don't ship the latter no more in langpacks, right? Not sure about
> searchplugins.

We're now shipping exactly the same stuff in langpacks as everyone else.

What I mostly feared to fail is stuff that expects us to live in the mozilla repo and not expecting to need different vars to point into mozilla/ somewhere.
Comment 6 Robert Kaiser 2010-08-19 05:33:09 PDT
Comment on attachment 434915 [details] [diff] [review]
(Av1) Just port it.

This probably has bitrotted and it surely didn't work back then, as l10n.mk was just fixed last night to even be usable for us.

That said, thanks for your work, we should look into moving this forward - and bug 587986 is doing the same for Thunderbird, btw.

r- until a patch is done that applies to current source, sorry I didn't get to test this up to now, but I was pretty sure l10n.mk would need some work to even be usable for us.
Comment 7 Serge Gautherie (:sgautherie) 2011-02-22 19:12:30 PST
Created attachment 514401 [details] [diff] [review]
(Av1a) Just port bug 489313

Av1, unbitrotted.
Still untested.
Not requesting review (yet): I'll try to do some simpler sync' first, so it's a little easier to see what remains then.
Comment 8 Justin Wood (:Callek) (Away until Aug 29) 2011-02-22 19:24:41 PST
Actually serge, since it had been so long since you worked on this, I had already started the port (from scratch) on my end. [Seeing a patch up is a surprise, though it is merely an un-bitrotted patch]

With the knowledge of buildbot/Makefile-fu, do you have any objection if I assign to me and round it out, or would you envision yourself getting a patch suitable for review by friday this week [my ETA for a patch up]
Comment 9 Serge Gautherie (:sgautherie) 2011-02-22 20:11:36 PST
(In reply to comment #8)

> Actually serge, since it had been so long since you worked on this

Well, your Status Meeting report just reminded me of this bug ;-)

> With the knowledge of buildbot/Makefile-fu, do you have any objection if I
> assign to me and round it out

Feel free to take this bug over whenever that suites you ;->
Comment 10 Justin Wood (:Callek) (Away until Aug 29) 2011-02-22 21:19:35 PST
(In reply to comment #9)
> (In reply to comment #8)
> > With the knowledge of buildbot/Makefile-fu, do you have any objection if I
> > assign to me and round it out
> 
> Feel free to take this bug over whenever that suites you ;->

For what its worth, I don't think doing interim changes to this Makefile is necessary, I'll tackle it all with my patch
Comment 11 Serge Gautherie (:sgautherie) 2011-02-22 22:16:16 PST
(In reply to comment #7)
> I'll try to do some simpler sync' first

Ftr, I attached 2 patches to bug 636076 in the meantime.

(In reply to comment #10)
> I'll tackle it all with my patch

Noted, I won't try to do more wrt to this file atm then.
Comment 12 Justin Wood (:Callek) (Away until Aug 29) 2011-02-23 21:44:16 PST
Created attachment 514728 [details] [diff] [review]
WIP

This is what I have so far, its tested so far as the rule magic I did for bookmarks is tested free-standing, other changes were relatively free-standing tested, plus code inspection; feel free to provide comments/concerns so far.
Comment 13 Justin Wood (:Callek) (Away until Aug 29) 2011-02-23 22:27:12 PST
Created attachment 514734 [details] [diff] [review]
WIP 2

Outstanding Question in this patch vs. last patch (marked with XXXCallek) is about searchplugin list.txt (adding feedback for this question, though happy if you want to poke at other parts of this patch early)

Also of note is the last change of mine for the night, |PKG_DMG_SOURCE| which actually does nothing where it is, since its defined AFTER packager.mk and not used in the file after its definition.
Comment 14 Justin Wood (:Callek) (Away until Aug 29) 2011-02-23 22:29:24 PST
Stefan, in WIP (near top) you'll see the MOZ_PKG_MAC_EXTRA definition, my question is if this is what we want (it was like this to begin with) or if we need to match Firefox/TB here?

See Firefox version at: http://mxr.mozilla.org/comm-central/source/mozilla/browser/locales/Makefile.in?mark=96-96#91

I suspect we want to keep our README here, but note that our symlink is also different.
Comment 15 Stefan [:stefanh] 2011-02-24 10:56:43 PST
(In reply to comment #14)
> Stefan, in WIP (near top) you'll see the MOZ_PKG_MAC_EXTRA definition, my
> question is if this is what we want (it was like this to begin with) or if we
> need to match Firefox/TB here?

You can remove the -format, looking at http://mxr.mozilla.org/mozilla-central/source/build/package/mac_osx/pkg-dmg#101, we now use UDBZ (bzip2) as default (we didn't back when we added that). Regarding the symlink, Karsten might remember why he choosed to have the symlink like that, since he was the one adding it in bug 406448 (though I doubt he remembers now). I tested the FF/TB "syntax", and I can't see any difference, so feel free to change it if you want to be in sync unless Karsten objects.

And yes, we want to keep our README :-)

Oh, remember to keep your changes in sync with /suite/installer/Makefile.in.
Comment 16 Justin Wood (:Callek) (Away until Aug 29) 2011-02-26 09:52:02 PST
Created attachment 515359 [details] [diff] [review]
v1.0

This should do it. Will test shortly
Comment 17 Stefan [:stefanh] 2011-02-26 10:10:59 PST
Comment on attachment 515359 [details] [diff] [review]
v1.0

+ifeq ($(MOZ_WIDGET_TOOLKIT),cocoa)
+MOZ_PKG_MAC_DSSTORE=$(_ABS_DIST)/branding/dsstore
+MOZ_PKG_MAC_BACKGROUND=$(_ABS_DIST)/branding/background.png
+MOZ_PKG_MAC_ICON=$(_ABS_DIST)/branding/disk.icns
+MOZ_PKG_MAC_EXTRA=--format UDBZ \
+                  --symlink "/Applications: " \
+                  --copy "$(LOCALE_SRCDIR)/installer/mac/README.txt"
+endif

Looks like you forgot to remove the UDBZ part. Did you want to sync the symlink part with the rest of the toolkit apps?
Comment 18 Justin Wood (:Callek) (Away until Aug 29) 2011-03-02 10:50:15 PST
Comment on attachment 515359 [details] [diff] [review]
v1.0

Ok, this doesn't work...

#1 it crashes stock mozbuild GNUMake, and the vpath stuff is horrid in what I need to do to locally get it working, firstword + wildcard will be better. And my preprocessor magic doesn't work as well as I'd like, I need another pass at this.
Comment 19 Justin Wood (:Callek) (Away until Aug 29) 2011-03-02 22:41:23 PST
Created attachment 516515 [details] [diff] [review]
v2.0

Major differences here vs 1.0:

* Dropped vpath junk for a new macro `find_locale_file` which I used instead
* The preprocessor magic I tried failed horribly, so reverted to the same general approach that already existed.

Also requesting feedback from bhearsum to find out if this approach (macro) is wanted on the browser/ side and if so (and if the macro is wanted in l10n.mk) we can simplify all of our apps Makefiles while we're at it. Don't need the feedback on the rest of my changes, but happy to take them.
Comment 20 Axel Hecht [pto-Aug-30][:Pike] 2011-03-03 06:34:13 PST
I suggest that you look into the patch in bug 525438, that has magic already.
Comment 21 Justin Wood (:Callek) (Away until Aug 29) 2011-03-03 09:07:57 PST
(In reply to comment #20)
> I suggest that you look into the patch in bug 525438, that has magic already.

Hrm, I somehow missed that bug in my bugspam... That magic there is almost the same magic I did here. just different approach/var names, but same end-result.

I am hoping I can still land this, since that bug has not landed yet, though. I'll adjust to match once that bug does land.

I am purposely still keeping those "non-mergable" files as required by the locale in the meantime as well.
Comment 22 Robert Kaiser 2011-03-05 09:00:38 PST
I can't even get as far as to test repackages as a "make package" already fails with this:
make -C ../../suite/locales langpack
make[3]: Entering directory `/mnt/mozilla/build/seamonkey/suite/locales'
make[3]: *** No rule to make target `langpack'.  Stop.
Comment 23 Robert Kaiser 2011-03-05 09:16:22 PST
Comment on attachment 516515 [details] [diff] [review]
v2.0

>+MOZ_PKG_MAC_EXTRA=--format UDBZ \
>+                  --symlink "/Applications: " \
>+                  --copy "$(LOCALE_SRCDIR)/installer/mac/README.txt"

Correct here and in installer/ (!) as discussed in this bug.

While you're there, make sure nothing you do here makes us come out of sync with thing that are in installer/ there.

> PROFILE_FILES = \
>+	localstore.rdf \
> 	mimeTypes.rdf \
>-	localstore.rdf \
> 	$(NULL)

You can remiove that one, I just reviewed a patch from Serge that does that. I hope the conflict will not be too bad from those nits he fixes int hat bug.

>+# the #include in the .in file requires all to be in the same dir, sadly.

You're right with the addition of "sadly" there. ;-)
Still I'm not 100% sure if it's better to put the comment here and not with the cp - but then, you're the one who probably has to maintain this going forward.

>-SEARCH_PLUGINS = $(shell cat \
>-   $(firstword $(wildcard $(LOCALE_SRCDIR)/searchplugins/list.txt) \
>-               @srcdir@/en-US/searchplugins/list.txt ) )

>+SEARCH_PLUGINS = $(shell cat $(LOCALE_SRCDIR)/searchplugins/list.txt)

Why this change?

>-# leave out $(STAGEPATH) as we never have a universal/ subdir here
>-PKG_DMG_SOURCE = $(MOZ_PKG_APPNAME)

You're sure we don't need this any more?

>-langpack-%: LANGPACK_FILE=$(_ABS_DIST)/$(PKG_LANGPACK_PATH)$(PKG_LANGPACK_BASENAME).xpi
>-langpack-%: AB_CD=$*
>-langpack-%: XPI_NAME=locale-$*
>-langpack-%: libs-%
>-	@echo "Making langpack $(LANGPACK_FILE)"
>-	@$(NSINSTALL) -D $(DIST)/$(PKG_LANGPACK_PATH)
>-	$(PYTHON) $(MOZILLA_SRCDIR)/config/Preprocessor.py $(DEFINES) $(ACDEFINES) -I$(call EXPAND_MOZLOCALE_SRCDIR,toolkit/locales)/defines.inc -I$(LOCALE_SRCDIR)/defines.inc $(srcdir)/generic/install.rdf > $(FINAL_TARGET)/install.rdf
>-	cd $(DIST)/xpi-stage/locale-$(AB_CD) && \
>-	  $(ZIP) -r9D $(LANGPACK_FILE) install.rdf chrome chrome.manifest -x chrome/$(AB_CD).manifest
>-
>-langpack: langpack-$(AB_CD)

Here's the target that "make package" chokes on. Not replicated in l10n.mk? How does FF do this?

>+.PHONY: repackage-extensions

Just let me say that I like that repackage-extensions solution - if it proves to work correctly.

r- mainly for the "make package" failure.
Comment 24 Justin Wood (:Callek) (Away until Aug 29) 2011-03-05 19:17:39 PST
(In reply to comment #22)
> I can't even get as far as to test repackages as a "make package" already fails
> with this:
> make -C ../../suite/locales langpack
> make[3]: Entering directory `/mnt/mozilla/build/seamonkey/suite/locales'
> make[3]: *** No rule to make target `langpack'.  Stop.

With the STOCK comm-central I get:

Justin@DELLXPS400-1 /c/t/repack2/comm-central/suite/locales
$ make package
make: *** No rule to make target `package'.  Stop.

|make langpack| works however, but for that its actually |make langpack-de| in your case.

I'll be sure to double check in a moment, after my "with patch" re-test is done, and answer your Q's then.
Comment 25 Justin Wood (:Callek) (Away until Aug 29) 2011-03-05 20:32:45 PST
(In reply to comment #24)
> (In reply to comment #22)
> > I can't even get as far as to test repackages as a "make package" already fails
> > with this:
> > make -C ../../suite/locales langpack
> > make[3]: Entering directory `/mnt/mozilla/build/seamonkey/suite/locales'
> > make[3]: *** No rule to make target `langpack'.  Stop.
> 
> With the STOCK comm-central I get:
> 
> Justin@DELLXPS400-1 /c/t/repack2/comm-central/suite/locales
> $ make package
> make: *** No rule to make target `package'.  Stop.
> 
> |make langpack| works however, but for that its actually |make langpack-de| in
> your case.
> 
> I'll be sure to double check in a moment, after my "with patch" re-test is
> done, and answer your Q's then.

Ok, |make langpack| does indeed work in Firefox, and was broken here; I accidentally removed that line (sorry).  My test did not hit that.

My port seems to have inherently fixed a bug. or so it seems anyway:

Exists Pre-Patch (does not exist post-patch):
dist/l10n-stage/seamonkey/extensions/debugQA@mozilla.org.xpi
dist/l10n-stage/seamonkey/extensions/inspector@mozilla.org.xpi
dist/l10n-stage/seamonkey/extensions/{59c81df5-...-4e0fdf64e5f2}.xpi
dist/l10n-stage/seamonkey/extensions/{f13157f-...-4815ddfdfeb8}.xpi

Exists Post-Patch (does not exist pre-patch):
dist/l10n-stage/optional/extensions/debugQA@mozilla.org.xpi
dist/l10n-stage/optional/extensions/inspector@mozilla.org.xpi
dist/l10n-stage/optional/extensions/{59c81df5-...-4e0fdf64e5f2}.xpi
dist/l10n-stage/optional/extensions/{f13157f-...-4815ddfdfeb8}.xpi

Which is actually the _correct_ way to repack for the windows installer.

(In reply to comment #23)
> >+MOZ_PKG_MAC_EXTRA=--format UDBZ \
> >+                  --symlink "/Applications: " \
> >+                  --copy "$(LOCALE_SRCDIR)/installer/mac/README.txt"
> 
> Correct here and in installer/ (!) as discussed in this bug.

Sure, (I was going to save that for another bug, but no problem.)

> 
> While you're there, make sure nothing you do here makes us come out of sync
> with thing that are in installer/ there.

Of course.

> > PROFILE_FILES = \
> >+	localstore.rdf \
> > 	mimeTypes.rdf \
> >-	localstore.rdf \
> > 	$(NULL)
> 
> You can remiove that one, I just reviewed a patch from Serge that does that. I
> hope the conflict will not be too bad from those nits he fixes int hat bug.

I'll rebase this (next) patch on top of serge's two in a moment.

> >+# the #include in the .in file requires all to be in the same dir, sadly.
> 
> You're right with the addition of "sadly" there. ;-)
> Still I'm not 100% sure if it's better to put the comment here and not with the
> cp - but then, you're the one who probably has to maintain this going forward.

I put it here, to avoid an extra shell command of the #, which happened to me on windows at least. it also makes this ever so slightly faster on windows.

> >-SEARCH_PLUGINS = $(shell cat \
> >-   $(firstword $(wildcard $(LOCALE_SRCDIR)/searchplugins/list.txt) \
> >-               @srcdir@/en-US/searchplugins/list.txt ) )
> 
> >+SEARCH_PLUGINS = $(shell cat $(LOCALE_SRCDIR)/searchplugins/list.txt)
> 
> Why this change?

As discussed before (IRC?) this was changed to match what Firefox currently has in code [though I recall a bug with a patch that makes SEARCH_PLUGINS in firefox, use both LOCALE_SRCDIR and the MERGE directory, so I chose to do that locally)

> >-# leave out $(STAGEPATH) as we never have a universal/ subdir here
> >-PKG_DMG_SOURCE = $(MOZ_PKG_APPNAME)
> 
> You're sure we don't need this any more?

actually, I'm not sure, and skimming a slave that did l10n-nightly repacks, I have a funny feeling we *do* need it. so re-adding it to be safe. I can revisit later.

> >+.PHONY: repackage-extensions
> 
> Just let me say that I like that repackage-extensions solution - if it proves
> to work correctly.

It certainly does work correctly, tested with sk-locale (which _does_ have a cZ locale, though not a DOMi one)
Comment 26 Justin Wood (:Callek) (Away until Aug 29) 2011-03-05 22:15:11 PST
Created attachment 517224 [details] [diff] [review]
v3.0 add langpack target back, change mac DMG creation
Comment 27 Robert Kaiser 2011-03-06 04:41:03 PST
(In reply to comment #24)
> With the STOCK comm-central I get:
> 
> Justin@DELLXPS400-1 /c/t/repack2/comm-central/suite/locales
> $ make package
> make: *** No rule to make target `package'.  Stop.

Sure. "make package" is to be executed from the toplevel, not from locales. ;-)
This is done by the normal builds, so if you only test what repack does, you miss doing it (but you need a package generated elsewhere as a start).


(In reply to comment #25)
> Ok, |make langpack| does indeed work in Firefox, and was broken here; I
> accidentally removed that line (sorry).  My test did not hit that.

Ah, OK. Good we caught it, then, as it would have broken the main builds, see above.

> My port seems to have inherently fixed a bug. or so it seems anyway:

Interesting, I wonder why we had it that way. Still should actually change again to make them be in dist/extensions, but that's yet another bug.

> I put it here, to avoid an extra shell command of the #, which happened to me
> on windows at least. it also makes this ever so slightly faster on windows.

Ah, interesting. Didn't realize that having a tab in front actually makes us invoke the shell for the comment. Sucks somewhat. Yay for make! :P

> > >-# leave out $(STAGEPATH) as we never have a universal/ subdir here
> > >-PKG_DMG_SOURCE = $(MOZ_PKG_APPNAME)
> > 
> > You're sure we don't need this any more?
> 
> actually, I'm not sure, and skimming a slave that did l10n-nightly repacks, I
> have a funny feeling we *do* need it. so re-adding it to be safe. I can revisit
> later.

That's why I asked, I remember adding this because repacks on Mac were broken and it was a bit of a pain to debug to come up with this one.
Comment 28 Robert Kaiser 2011-03-06 04:49:05 PST
(In reply to comment #25)
> I'll rebase this (next) patch on top of serge's two in a moment.

Could you please attach a new patch based on the current trunk where they did? I'd really like to test before giving an r+ but get a "hunk #1 failed" and that one's big enough that I don't feel like unbitrotting it myself...
Comment 29 Justin Wood (:Callek) (Away until Aug 29) 2011-03-06 05:59:31 PST
Created attachment 517253 [details] [diff] [review]
v3.1

This should have absolutely no patching-apply failures. [unless you have local changes]
Comment 30 Justin Wood (:Callek) (Away until Aug 29) 2011-03-06 17:45:31 PST
Created attachment 517329 [details] [diff] [review]
v3.2  -- This time without windows line endings.
Comment 31 Robert Kaiser 2011-03-06 17:52:33 PST
Comment on attachment 517329 [details] [diff] [review]
v3.2  -- This time without windows line endings.

OK, looks fine to me from what I can tell here - let's see if our buildbot machines agree ;-)
Comment 32 Justin Wood (:Callek) (Away until Aug 29) 2011-03-06 17:58:37 PST
http://hg.mozilla.org/comm-central/rev/4ba201da6c48
Comment 33 Justin Wood (:Callek) (Away until Aug 29) 2011-03-06 18:11:04 PST
somehow landed patch missed a few nits, followed up with http://hg.mozilla.org/comm-central/rev/cfb248080a91
Comment 34 Justin Wood (:Callek) (Away until Aug 29) 2011-03-06 19:17:59 PST
http://hg.mozilla.org/comm-central/rev/7b7a231b4a43
http://hg.mozilla.org/comm-central/rev/7e520bf6faad

Yea, if this fails now I'm backing out and revisiting tomorrow.
Comment 35 Justin Wood (:Callek) (Away until Aug 29) 2011-03-06 19:18:48 PST
(In reply to comment #34)
> http://hg.mozilla.org/comm-central/rev/7b7a231b4a43
> http://hg.mozilla.org/comm-central/rev/7e520bf6faad
> 
> Yea, if this fails now I'm backing out and revisiting tomorrow.

(Needed because I removed the cat before, which caused us to look for list.txt.xml instead of the various searchplugins)

Note You need to log in before you can comment on or make changes to this bug.