554993 - Use l10n.mk in SeaMonkey (suite/locales/Makefile.in)

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Just port it.

That should be it: please, double-check.

Comment 2

[Robert Kaiser]

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)]

(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 [:Pike]]

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]

(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]

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)]

Attachment: (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)]

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)]

(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)]

(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)]

(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)]

Attachment: 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)]

Attachment: 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)]

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]]

(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)]

Attachment: v1.0

This should do it. Will test shortly

Comment 17

[Stefan [:stefanh]]

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)]

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)]

Attachment: 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 [:Pike]]

I suggest that you look into the patch in bug 525438, that has magic already.

Comment 21

[Justin Wood (:Callek)]

(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]

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]

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)]

(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)]

(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/[e-mail]
dist/l10n-stage/seamonkey/extensions/[e-mail]
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/[e-mail]
dist/l10n-stage/optional/extensions/[e-mail]
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)]

Attachment: v3.0 add langpack target back, change mac DMG creation

Comment 27

[Robert Kaiser]

(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]

(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)]

Attachment: v3.1

This should have absolutely no patching-apply failures. [unless you have local changes]

Comment 30

[Justin Wood (:Callek)]

Attachment: v3.2 -- This time without windows line endings.

Comment 31

[Robert Kaiser]

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)]

http://hg.mozilla.org/comm-central/rev/4ba201da6c48

Comment 33

[Justin Wood (:Callek)]

somehow landed patch missed a few nits, followed up with http://hg.mozilla.org/comm-central/rev/cfb248080a91

Comment 34

[Justin Wood (:Callek)]

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)]

(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)