144551 - inconsistent en-mac/en-unix in different platform langenus.xpi files

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect, P1)

Description

[Ying-Lin Xia]

Currently, the en-mac.jar and en-unix.jar are different between different 
platform langenus.xpi. For example, in win32 langenus.xpi, the en-mac.jar 
contains 7 files, but in mac langenus.xpi, the en-mac.jar contains 9 files.

The xpi packaging needs to pick up those files from same place, to keep them 
consistent. Otherwise, the langenus.xpi won't be cross-platform.

Comment 1

[Daniel Veditz [:dveditz]]

which version? trunk? branch? I can confirm the problem on the trunk, at least,
but don't know if we need to fix this for mozilla1.0 if it's on the branch. 
Please nominate as appropriate if this is expected to be fixed.

Looks like bug 104485 was the culprit, over to Ben

(note, this has nothing to do with the XPI packaging, it's the build system that
creates the en-<plat>.jar files)

Comment 2

[Ying-Lin Xia]

for all, trunk and branch

Comment 3

[Frank Tang]

could you list the file name for each one. thanks

Comment 4

[Ying-Lin Xia]

There are actually two problems in the en-mac/unix.jar.
1) In these 2 jar files, there are some inconsistent files between different
platforms' langenus.xpi:
  - en-mac.jar: communicator-platform\*.* are in Mac xpi but not Win xpi;
  - en-unix.jar: platformCommunicatorOverlay.dtd is in Unix xpi, but not Win xpi.
2) In these 2 jar files, all file format (line breaks) are different between
different platforms' langenus.xpi.

The first problem could make the language pack (langXXXX.xpi) not really
cross-platform. The second problem confused our localization tool, the software
think all the files are total different.

Ideally, all the en-win/mac/unix.jar should be exactly totally same in all
platforms' langenus.xpi, file amount and file format.

Comment 5

[tao]

I am lost here. Isn't the whole purpose of en-[platform].jar is to intelligently
install only needed files on a specific platform? We are supposed to put 
win-specific files (such as winhook, n2p, etc) in en-win.jar but not the rest
(en-mac.jar or en-unix.jar).

Comment 6

[Ying-Lin Xia]

regardless installation, en-win/mac/unix.jar should be together in the 
langenus.xpi, which suppose to be cross-platform. For example, we always post 
Win32 langXXXX.xpi as language pack for all platforms.

Comment 7

[tao]

Understood now!

Comment 8

[Frank Tang]

yxia: again, please list the file names here.
>communicator-platform\*.*
what are *.* here?
please list them. so we can look at them.

Comment 9

[Ying-Lin Xia]

communicator-platform\*.* means the directory itself and all files inside, 
contents.rdf and platformCommunicatorOverlay.dtd

Comment 10

[Frank Tang]

> the directory itself and all files inside, 
could you list all the file inside ?

Comment 11

[Ying-Lin Xia]

2 files listed above

Comment 12

[Michele Carlson]

nominating it nsbeta1

Comment 13

[Samir Gehani]

Nav triage team needs info: how does this affect end users?  What is the effect
of the missing files?  Ben?  Ying?

Comment 14

[Ying-Lin Xia]

To end user: for example, if a Mac user download a language pack from our web, 
I think he will see some blank menu items:
   <!ENTITY closeCmd.label
   <!ENTITY printSetupCmd.label
   <!ENTITY quitApplicationCmd.label
   <!ENTITY redoCmd.label
Because we always post win32 language pack on the web, and in that pack, the 
Mac platformCommunicatorOverlay.dtd is not in the en-mac.jar.

I have not try it yet though...

Comment 15

[Daniel Veditz [:dveditz]]

No, missing DTD files to not result in blank strings, they result in blank
windows because the XML doesn't validate. missing .properties files might result
in the more benign blank strings. DTD's are way too fragile to have been a good
choice for localization mechanism. Maybe we need to handle XUL in a Quirks mode
that doesn't enforce the rule that all entities must exist :-)

Comment 16

[Frank Tang]

yokoyama- could you please help to address the issue. thanks

Comment 17

[Roy Yokoyama]

what am I suppose to do here?  I am kinda new in xpi arena.

So by reading comments, we have to do:
- add contents.rdf and platformCommunicatorOverlay.dtd
  into en-mac.jar and en-unix.jar
- then update langenus.xpi

Is this correct? If so, then do you really need my help?
dan, can you own this bug?

Comment 18

[Daniel Veditz [:dveditz]]

This is not a xpi problem. The Xpi process is correctly slurping up exactly what
the build process on each platform produces. The problem is that there are now
some mac-only and linux-only jar.mn files so that the en-mac etc produced on the
different platforms no longer match (ignoring line-ending differences, which
don't matter to our parser).

You need to make the windows and linux build process look like the mac process
for en-mac. You need to make the windows and mac build process look like linux
for en-unix, and you need to make the mac and linux build process look like
windows for en-win.

These all used to be the same, but a few new platform-only jar.mn files were
introduced. These should probably get moved back up into the parent jar.mn

Comment 19

[Roy Yokoyama]

Who knows _how_ to do this?  I certainly don't.  Tao?

Comment 20

[tao]

Hi, Don - would you please take a look? thx!

Comment 21

[Roy Yokoyama]

assign to don.

Comment 22

[Daniel Veditz [:dveditz]]

- Use lxr to search for the files in question
- find which jar.mn they're in
- work up the tree to find the common jar.mn they should be moved to
- move the references to the new jar.mn, fixing up the paths as req'd
- remove old single-platform jar.mn
- remove platform build system references to those jar.mn

Comment 23

[tao]

marking assigned to indicate work in progress...

Comment 24

[Robert Kaiser]

Hmm, isn't this more or les a dupe of bug 133849?
I filed that one when I found out the inconsistency problem is much bigger than
bug 118140 that is still one of the biggest headache for L10n people...

Comment 25

[dbragg]

Attachment: Patch for jar.mn and "make" file mods

After discussion with dveditz I felt that the best place for the jar references
for the platform-specific file was in the "root" jar.mn files.	There's already
a whole bunch of chrome-related files there so there won't be much of a problem
for developers who just want to build chrome.  They already had to go to this
level anyway.  The one exception to this is the winhooks.  This is only one
file so I added the directory to the unix makefile.in and a CreateFileFromJar
entry in the MozillaBuildList.pm

Comment 26

[tao]

Comment on attachment 86639 [details] [diff] [review]
Patch for jar.mn and "make" file mods

r=tao

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 86639 [details] [diff] [review]
Patch for jar.mn and "make" file mods

Please do another diff with the -N option -- I want to see the obsolete
jar.mn files and asssociated makefiles go away -- otherwise people can
get suckered into updating the wrong files.

Have you tested (or had build buddies test) on all platforms? 

>Index: mailnews/jar.mn
>===================================================================
>+en-win.jar:
>+     locale/en-US/messenger-mapi/pref-mailnewsOverlay.dtd               (mapi/resources/locale/en-US/pref-mailnewsOverlay.dtd)
>+     locale/en-US/messenger-mapi/mapi.properties                        (mapi/resources/locale/en-US/mapi.properties)
>+     locale/en-US/messenger-mapi/contents.rdf                           (mapi/resources/locale/en-US/contents.rdf)

I don't see these removed from anywhere else. Will these get processed
twice on windows? At a quick glance looks like you could remove
"locale" from DIRS in both makefiles in mozilla/mailnews/mapi/resources/,
and then remove all the makefiles in locales and below.

>Index: xpfe/communicator/jar.mn
>===================================================================
>+
>+en-win.jar:
>+    locale/en-US/communicator-platform/contents.rdf		                 (win/contents-platform.rdf)
>+    locale/en-US/communicator-platform/platformCommunicatorOverlay.dtd (win/platformCommunicatorOverlay.dtd)
>+
>+en-mac.jar:
>+    locale/en-US/communicator-platform/contents.rdf		                 (mac/contents-platform.rdf)
>+    locale/en-US/communicator-platform/platformCommunicatorOverlay.dtd (mac/platformCommunicatorOverlay.dtd)
>+
>+en-unix.jar:
>+    locale/en-US/communicator-platform/contents.rdf		                 (unix/contents-platform.rdf)
>+    locale/en-US/communicator-platform/platformCommunicatorOverlay.dtd (unix/platformCommunicatorOverlay.dtd)

These too, where do they come out of, and what makefile entries
now need to be removed?

There's a mozilla/xpfe/communicator/resources/locale/en-US/os2 directory,
but according to the en-US Makefile.in the os2 folks use the win versions.

Should we remove the os2 stuff to avoid confusion? Leave them but snip
the makefile stuff up to the resources dir level (because you have to leave
the content stuff platform-specific).

>Index: xpfe/components/Makefile.in
>===================================================================
>-DIRS		= bookmarks directory download-manager filepicker find history search sidebar related regviewer xfer prefwindow shistory timebomb console autocomplete updates urlbarhistory intl resetPref killAll
>+DIRS		= bookmarks directory download-manager filepicker find history search sidebar related regviewer xfer prefwindow shistory timebomb console autocomplete updates urlbarhistory intl resetPref killAll winhooks 
> 
> ifeq ($(OS_ARCH),WINNT)
>-DIRS		+= winhooks urlwidget alerts
>+DIRS		+= urlwidget alerts
> endif

I don't see any mods to the winhooks Makefile.in, which up to now
has assumed only win folks get there (thus my question above about
platform build buddies).

Wouldn't it be better to zap the winhooks/jar.mn and just move the
lines up to mozilla/xpfe/components/jar.mn?

>Index: xpfe/global/jar.mn

Same thing, you've added stuff here but not removed the old mechanism.
We'll eventually end up with the same problem again only harder to
diagnose because it'll look like we've done the right thing.

xpfe/browser appears to have the same problematic structure, although
at a quick glance at least win and linux traverse through all three
platform directories. I guess if that part's working we shouldn't
mess with it on the branch, but I'd like a trunk patch to deal with it.

Comment 28

[dbragg]

Attachment: New diff with a -N option

This diff shows the removed files.

Comment 29

[dbragg]

Attachment: New -N diff with mod to xpfe/components per Dan's suggestion

Dan made a good suggestion about the xpfe/components/winhooks dir.  It would be
a can of worms to open up that tree to Unix.  It's better to just move the one
file that was listed in the jar.mn to the xpfe/components' jar.mn file.  I
didn't have that in the last diff.  This also removes one modification to the
Mac's MozillaBuildList.pm file. (a good thing)

Comment 30

[tao]

Comment on attachment 86897 [details] [diff] [review]
New -N diff with mod to xpfe/components per Dan's suggestion

r=tao

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 86897 [details] [diff] [review]
New -N diff with mod to xpfe/components per Dan's suggestion

looks good

Comment 32

[Daniel Veditz [:dveditz]]

Comment on attachment 86897 [details] [diff] [review]
New -N diff with mod to xpfe/components per Dan's suggestion

oops, sr=dveditz

Comment 33

[tao]

new owner ->tao

Comment 34

[tao]

Comment on attachment 86897 [details] [diff] [review]
New -N diff with mod to xpfe/components per Dan's suggestion

The new chrome regsitry system has the side-effect to this patch; en-mac.jar
becomes the provider on win/unix. We need to find a place to explicitly
designate the chrome provider.

Comment 35

[Daniel Veditz [:dveditz]]

That problem is a build system artifact, the installed versions don't have that
problem. Therefore it can be fixed by the build system. Figure out whether it's
the first-registered or last registered that takes precedence (I'm guessing
first). Order the jar.mn such that mac will get registered. Then in the windows
and gmake scripts echo the appropriate registration line into
installed-chrome.txt explicitly such that the correct platform is registered
instead. Remember that the gmake system also handles win(and os2) and Mac os x
so you'll have to ifdef which platform gets registered there.

Comment 36

[[e-mail] (away - not reading bugmail)]

-> jbetak

Comment 37

[[e-mail] (away - not reading bugmail)]

second try

Comment 38

[[e-mail] (away - not reading bugmail)]

tao, dan:

if en-mac.jar is used as a chrome provider, it would show up in installed-
chrome.txt, correct? I tried to verify that claim on my current gmake win32 
build and two Mac builds (old-style and Fizilla OS X), but it doesn't seem to 
be the case.

However, it seems like en-mac.jar is *still* out of sync. I'm seeing 
platformglobaloverlay.dtd in Mac builds - it's missing on win32 builds...

Comment 39

[tao]

Hi, Juraj:


In a win32 or linux buid, we don't use en-mac.jar. Therefore, you shouldn't see
it registered on non-Mac build. 

For classic OS9 build, you are right that en-mac.jar should be registered. Are
you referring to a build with the patch
http://bugzilla.mozilla.org/attachment.cgi?id=86897&action=view ? 

Comment 40

[[e-mail] (away - not reading bugmail)]

Hi Tao,

I just applied the patch, realizing that your comment might have been related 
to its effects, and not the current state of the build scripts :-)

Comment 41

[[e-mail] (away - not reading bugmail)]

Attachment: updated patch, resolves the incorrect chrome provider issue raised by tao

dveditz:

I've updated dbragg's patch and and opted to modify add-chrome.pl and
make-jars.pl instead of "echoing" the correct platform jar into
installed-chrome.txt to override previous entries. Do you think we could take
such a change into the tree?

Comment 42

[Robert Kaiser]

jbetak:
I can't help with r= as you requested in bug 118140 Comment #39 because I don't
know the original code good enough to really check what the patch does in
detail. Sorry. Can't await having this all fixed though.

Comment 43

[[e-mail] (away - not reading bugmail)]

Verification builds look good on all platforms. Tao idicated we should run this by

sfraser and seawood - cc'ing them for review. Could we get this into Moz 1.2a?

Comment 44

[hacker formerly known as [e-mail]]

Comment on attachment 95168 [details] [diff] [review]
updated patch, resolves the incorrect chrome provider issue raised by tao

I'm not sure who the right person to review this behavior change is since
warren left.  My guess would be one of the xpapps guys.  

The OS detection really needs to be fixed though.  It doesn't properly detect
cygwin perl ($^O == cygwin) and linux isn't the only unix platform that we
support.  We should default to unix if win32 & macos are not detected.

Comment 45

[[e-mail] (away - not reading bugmail)]

Chris, thanks for having a look at this. 

I agree with the fix for other Unix platforms -- I haven't thought of that at
all. In addition to this we could list the Unix jar files last in the
corresponding jar.mn. This would prevent the wrong platform file from becoming
chrome provider even if the detection failed.

I'm not sure that I agree with the $^O == cygwin comment. I believe I tried that
before and ran into some issues, the current Windows/cygwin detection seems to
work quite well.

I have to admit that I plagiarized Gerv's code, which means that there are at
least two precedents in our code base:

http://lxr.mozilla.org/seamonkey/source/config/make-chromelist.pl#71
http://lxr.mozilla.org/seamonkey/source/embedding/config/gen_mn.pl#15

Comment 46

[hacker formerly known as [e-mail]]

Are you sure that you're using cygwin perl and that $win32 is being set to 1?

ARCHANGEL:/cygdrive/c/root/mozilla> perl -e '$win32 = ($^O =~ /((MS)?win32)|os2/
i) ? 1 : 0; print "$win32\n"'
0
ARCHANGEL:/cygdrive/c/root/mozilla> perl -v
This is perl, v5.6.1 built for cygwin-multi

The other scripts that use this method of platform detection should be fixed as
well (though not necessarily in this bug).

Comment 47

[[e-mail] (away - not reading bugmail)]

Attachment: updated patch

Chris, you are obviously right. I filed a new bug 166105 for cygwin perl
support in make-chromelist.pl and gen_mn.pl.

I still don't like the platform detection on Windows, what would you say to
something less kludgy? 

my $win32 = ($^O =~ /win|os2/i) ? 1 : 0;

Comment 48

[Gervase Markham [:gerv]]

> my $win32 = ($^O =~ /win|os2/i) ? 1 : 0;

You can't do this - remember "darwin".

Gerv

Comment 49

[[e-mail] (away - not reading bugmail)]

oh, right :-)

Comment 50

[[e-mail] (away - not reading bugmail)]

Attachment: yet another patch iteration

Gerv, thanks for pointing out my oversight in regards to Darwin... 
I hope that this could finally be what we've been looking for.

my $macos = ($^O =~ /MacOS|darwin/i) ? 1 : 0;
my $win32 = (!$macos && ($^O =~ /win|os2/i)) ? 1 : 0;
my $unix  = !($win32 || $macos) ? 1 : 0;

Comment 51

[hacker formerly known as [e-mail]]

Comment on attachment 97552 [details] [diff] [review]
yet another patch iteration

I preferred the previous win32 detection string though it still needed cygwin
added to the list.  Just grepping for /win/ seems to be asking for trouble.

Comment 52

[[e-mail] (away - not reading bugmail)]

Attachment: reverting to more explicit Windows detection

thanks Chris - I'm changing back to my $win32 = ($^O =~
/((MS)?win32)|cygwin|os2/i) ? 1 : 0;

Comment 53

[Simon Fraser [no longer active]]

For Mac, these changes scare me. Make sure they are thoroughly tested for both
MacPerl-based CFM builds, and Mach-O based builds, using both Classic and Modern
skins. This seems like it could cause a bunch of Mac chrome regressions.

Who runs add-chrome.pl and make-jars.pl for MacPerl builds? Is this a
packaging-only thing?

+my $macos = ($^O =~ /MacOS|darwin/i) ? 1 : 0;

Does $^O work in MacPerl?

Comment 54

[Simon Fraser [no longer active]]

One more comment:

> 2) In these 2 jar files, all file format (line breaks) are different between
different platforms' langenus.xpi.

...
> The second problem confused our localization tool, the software
think all the files are total different.

None of the changes in this bug address this issue, and it's really an issue
with your localization tools. The tools _have_ to be smart about linebreaks.

Comment 55

[Ying-Lin Xia]

Well, as long as the first cross-platform problem can be fixed, we can live with
it then. Because for localization, we can just simply use the en-[platform].jar
in the win32 langenus.xpi, for all other platforms - then we won't have
line-break problem anymore. This means we will use the DOS-linebreak format
files in our localized Mac builds, which won't be a problem - as long as the
en-[platform].jar inconsistency problem can be fixed, we will be safe to use the
win32 en-mac.jar in Mac build.

Comment 56

[[e-mail] (away - not reading bugmail)]

Simon:

Mach-O-based builds work great. It appears that for the MacPerl-based build, we
need to change Jar.pm
http://lxr.mozilla.org/seamonkey/source/build/mac/build_scripts/Moz/Jar.pm#407
The only reason MacPerl builds worked for me is that the mac-specific files are
listed last in installed-chrome.txt. This is due to  the reordering of static
jar manifest entries introduced by the rest of the patch. (Competing chrome
providers are awarded precedence based on their position in the
installed-chrome.txt files.)

$^O works great with both MacPerl and on OS X. 

I do realize the risk of regressions and since the platform detection is
"merely" aiming at elimination of unnecessary platform files in
installed-chrome.txt, failure is not catastrophic. The most fragile platform
(Unix? Mac?) could be simply listed last in that file by the virtue of
reordering static jar manifest entries.

locale,install,url,jar:resource:/chrome/en-unix.jar!/locale/en-US/navigator-platform/
locale,install,url,jar:resource:/chrome/en-win.jar!/locale/en-US/navigator-platform/
locale,install,url,jar:resource:/chrome/en-mac.jar!/locale/en-US/navigator-platform/

Comment 57

[[e-mail] (away - not reading bugmail)]

Attachment: patch for Jar.pm

sfraser: 

this is an analogous change for Jar.pm, it excludes platform-dependent chrome
files from being registered via installed-chrome.txt. In theory this minimizes
the probability of a foreign platform file from becoming chrome provider.

Comment 58

[[e-mail] (away - not reading bugmail)]

Attachment: complete & updated patch

Comment 59

[[e-mail] (away - not reading bugmail)]

Would anyone dare to r=/sr=? Please?

This has been tested to death and I can't see any issues in verification builds.
The packaging change is on the top of the L10n and L12y wish list for 1.2. Let's
get this in in time for the 1.2 release. Giving it a longer trunk baking time
and QA exposure can only help to prevent any potential regressions. 

The original incarnation of this patch was reviewed by tao and dveditz. We had
to broaden the scope due to some build system relicts pointed out by dveditz.
Thanks Dan!

Comment 60

[hacker formerly known as [e-mail]]

Comment on attachment 100488 [details] [diff] [review]
complete & updated patch

r=cls

Comment 61

[[e-mail] (away - not reading bugmail)]

thanks Chris! 

Simon would feel comfortable giving an sr=?

Comment 62

[Simon Fraser [no longer active]]

not really. Maybe Dan V. can?

Comment 63

[[e-mail] (away - not reading bugmail)]

thanks Simon, I've sent him an email... I really hope this patch can make it
into 1.2.

Comment 64

[[e-mail] (away - not reading bugmail)]

I hate to be nagging, but l10n really needs this. 

Dan has given sr= on the original patch and might be swamped right now. He is
not responding to my emails. I'll try to track him down in time for the 1.2b
freeze, but if anyone gave an sr= on the perl script changes, that would be
great :-)

Comment 65

[Simon Fraser [no longer active]]

Comment on attachment 100488 [details] [diff] [review]
complete & updated patch

rs=sfraser

Comment 66

[[e-mail] (away - not reading bugmail)]

thanks Simon!

Comment 67

[Daniel Veditz [:dveditz]]

*** Bug 173502 has been marked as a duplicate of this bug. ***

Comment 68

[Ying-Lin Xia]

Yeah!!! With today's trunk, it looks great to me :-)
MILLION THANKS to jbetak! This is going to benefit us a lot. 

Comment 69

[[e-mail] (away - not reading bugmail)]

Ying, that's great! This was not a trivial change, and I'm still half expecting
someone to slam me with some nasty regression. And I hope to hear from kairo and
his experience with the change.

I'm cc'ing dbragg for the bulk of the credit. I'd say that he and dveditz did
most of the work. I only happened to be driving towards the end ;-)

Comment 70

[Robert Kaiser]

I built today's CVS (after I re-pulled the files I had conflicts with, as I had
118140's patch applied in my tree), and all the files look fine on Linux here.
I hope this really is resolved now. Thanks!

Marking verified.

Comment 71

[[e-mail] (away - not reading bugmail)]

Thanks kairo!

ying, rchen, mcarlson - does l10n need this on the branch (bug 173552)? If so,
would early next week be a workable timeframe?

Comment 72

[Michele Carlson]

Thanks! early next week would be fine.
This is so great!

Comment 73

[Daniel Veditz [:dveditz]]

nominating for the branch -- please seek drivers and adt approval to land.

Comment 74

[Michael Buckland]

Plussing for adt.  Need checkin by cob 10/16. Please make sure to get all
required  approvals (drivers) before checkin if this is not already done.

Comment 75

[Michael Buckland]

*** Bug 173552 has been marked as a duplicate of this bug. ***

Comment 76

[Daniel Veditz [:dveditz]]

Has this landed on the branch yet?

Comment 77

[[e-mail] (away - not reading bugmail)]

Dan, yes it has - see bug 173552. Updating keywords.

Comment 78

[Daniel Veditz [:dveditz]]

*** Bug 172075 has been marked as a duplicate of this bug. ***