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

RESOLVED FIXED in seamonkey2.1b3

Status

SeaMonkey
Build Config
--
trivial
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: sgautherie, Assigned: Callek)

Tracking

Trunk
seamonkey2.1b3
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

7 years ago
Created attachment 434915 [details] [diff] [review]
(Av1) Just port it.

That should be it: please, double-check.
Attachment #434915 - Flags: review?(kairo)
(Reporter)

Updated

7 years ago
Blocks: 547518
No longer blocks: 506493

Comment 2

7 years ago
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.
(Reporter)

Comment 3

7 years ago
(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

7 years ago
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

7 years ago
(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

7 years ago
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.
Attachment #434915 - Flags: review?(kairo) → review-

Updated

7 years ago
Depends on: 587984

Updated

7 years ago
Summary: Port |Bug 489313 - Use toolkit/locales/l10n.mk for fx, land on the 1.9.1 branch| to SeaMonkey → Use l10n.mk in SeaMonkey (suite/locales/Makefile.in)
(Reporter)

Updated

6 years ago
Depends on: 589232
(Reporter)

Comment 7

6 years ago
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.
Attachment #434915 - Attachment is obsolete: true
(Assignee)

Comment 8

6 years ago
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]
(Reporter)

Comment 9

6 years ago
(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 ;->
(Assignee)

Updated

6 years ago
Assignee: sgautherie.bz → bugspam.Callek
(Assignee)

Comment 10

6 years ago
(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
(Reporter)

Comment 11

6 years ago
(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.
(Assignee)

Comment 12

6 years ago
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.
Attachment #514401 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Target Milestone: seamonkey2.1a1 → ---
(Assignee)

Comment 13

6 years ago
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.
Attachment #514728 - Attachment is obsolete: true
Attachment #514734 - Flags: feedback?(kairo)
(Assignee)

Comment 14

6 years ago
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

6 years ago
(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.
(Assignee)

Comment 16

6 years ago
Created attachment 515359 [details] [diff] [review]
v1.0

This should do it. Will test shortly
Attachment #514734 - Attachment is obsolete: true
Attachment #515359 - Flags: review?(kairo)
Attachment #514734 - Flags: feedback?(kairo)

Comment 17

6 years ago
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?
(Assignee)

Comment 18

6 years ago
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.
Attachment #515359 - Flags: review?(kairo) → review-
(Assignee)

Comment 19

6 years ago
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.
Attachment #515359 - Attachment is obsolete: true
Attachment #516515 - Flags: review?(kairo)
Attachment #516515 - Flags: feedback?(bhearsum)

Comment 20

6 years ago
I suggest that you look into the patch in bug 525438, that has magic already.
(Assignee)

Comment 21

6 years ago
(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

6 years ago
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

6 years ago
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.
Attachment #516515 - Flags: review?(kairo) → review-
(Assignee)

Comment 24

6 years ago
(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.
(Assignee)

Comment 25

6 years ago
(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)
(Assignee)

Comment 26

6 years ago
Created attachment 517224 [details] [diff] [review]
v3.0 add langpack target back, change mac DMG creation
Attachment #516515 - Attachment is obsolete: true
Attachment #517224 - Flags: review?(kairo)
Attachment #516515 - Flags: feedback?(bhearsum)
(Reporter)

Updated

6 years ago
Depends on: 636076

Comment 27

6 years ago
(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

6 years ago
(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...
(Assignee)

Comment 29

6 years ago
Created attachment 517253 [details] [diff] [review]
v3.1

This should have absolutely no patching-apply failures. [unless you have local changes]
Attachment #517224 - Attachment is obsolete: true
Attachment #517253 - Flags: review?(kairo)
Attachment #517224 - Flags: review?(kairo)
(Assignee)

Comment 30

6 years ago
Created attachment 517329 [details] [diff] [review]
v3.2  -- This time without windows line endings.
Attachment #517253 - Attachment is obsolete: true
Attachment #517329 - Flags: review?(kairo)
Attachment #517253 - Flags: review?(kairo)

Comment 31

6 years ago
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 ;-)
Attachment #517329 - Flags: review?(kairo) → review+
(Assignee)

Comment 32

6 years ago
http://hg.mozilla.org/comm-central/rev/4ba201da6c48
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 33

6 years ago
somehow landed patch missed a few nits, followed up with http://hg.mozilla.org/comm-central/rev/cfb248080a91
(Assignee)

Comment 34

6 years ago
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.
(Assignee)

Comment 35

6 years ago
(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)
(Reporter)

Updated

6 years ago
Target Milestone: --- → seamonkey2.1b3
You need to log in before you can comment on or make changes to this bug.