Closed Bug 144551 Opened 18 years ago Closed 17 years ago

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

Categories

(SeaMonkey :: UI Design, defect, P1)

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.2alpha

People

(Reporter: yinglinxia, Assigned: jbetak)

References

Details

(Keywords: intl, Whiteboard: has r= / needs sr=)

Attachments

(1 file, 8 obsolete files)

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.
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)
Assignee: dveditz → ben
Status: UNCONFIRMED → NEW
Component: Installer: XPI Packages → XP Apps
Ever confirmed: true
for all, trunk and branch
Blocks: 141008
could you list the file name for each one. thanks
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.
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).

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.
Understood now!
yxia: again, please list the file names here.
>communicator-platform\*.*
what are *.* here?
please list them. so we can look at them.
communicator-platform\*.* means the directory itself and all files inside, 
contents.rdf and platformCommunicatorOverlay.dtd
> the directory itself and all files inside, 
could you list all the file inside ?
2 files listed above
nominating it nsbeta1
Keywords: nsbeta1
Nav triage team needs info: how does this affect end users?  What is the effect
of the missing files?  Ben?  Ying?
Whiteboard: [need info]
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...
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 :-)
yokoyama- could you please help to address the issue. thanks
Assignee: ben → yokoyama
Keywords: nsbeta1intl, nsbeta1+
Whiteboard: [need info] → [adt3]
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?
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
Who knows _how_ to do this?  I certainly don't.  Tao?
Hi, Don - would you please take a look? thx!
assign to don.
Assignee: yokoyama → dbragg
- 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
Whiteboard: [adt3] → [adt3] [ETA: 6/3/02]
Blocks: 144547
marking assigned to indicate work in progress...
Status: NEW → ASSIGNED
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...
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 on attachment 86639 [details] [diff] [review]
Patch for jar.mn and "make" file mods

r=tao
Attachment #86639 - Flags: review+
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.
Attachment #86639 - Attachment is obsolete: true
Attached patch New diff with a -N option (obsolete) — Splinter Review
This diff shows the removed files.
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)
Attachment #86890 - Attachment is obsolete: true
Comment on attachment 86897 [details] [diff] [review]
New -N diff with mod to xpfe/components per Dan's suggestion

r=tao
Attachment #86897 - Flags: review+
Comment on attachment 86897 [details] [diff] [review]
New -N diff with mod to xpfe/components per Dan's suggestion

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

oops, sr=dveditz
new owner ->tao
Assignee: dbragg → tao
Status: ASSIGNED → NEW
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.
Attachment #86897 - Flags: needs-work+
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.
Blocks: 154425
Blocks: 154922
Blocks: 62177
No longer blocks: 154922
-> jbetak
Status: NEW → ASSIGNED
second try
Assignee: tao → jbetak
Status: ASSIGNED → NEW
Blocks: 157673
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...
Status: NEW → ASSIGNED
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 ? 
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 :-)
Priority: -- → P1
Target Milestone: --- → mozilla1.2alpha
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?
Attachment #86897 - Attachment is obsolete: true
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.
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 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.
Attachment #95168 - Flags: needs-work+
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

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).
Attached patch updated patch (obsolete) — Splinter Review
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;
Attachment #95168 - Attachment is obsolete: true
> my $win32 = ($^O =~ /win|os2/i) ? 1 : 0;

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

Gerv
oh, right :-)
Attached patch yet another patch iteration (obsolete) — Splinter Review
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;
Attachment #97479 - Attachment is obsolete: true
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.
Attachment #97552 - Flags: needs-work+
thanks Chris - I'm changing back to my $win32 = ($^O =~
/((MS)?win32)|cygwin|os2/i) ? 1 : 0;
Attachment #97552 - Attachment is obsolete: true
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?
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.
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.
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/
Attached patch patch for Jar.pm (obsolete) — Splinter Review
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.
Attachment #97613 - Attachment is obsolete: true
Attachment #97709 - Attachment is obsolete: true
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!
Whiteboard: [adt3] [ETA: 6/3/02] → have patch / need r= and sr=
Comment on attachment 100488 [details] [diff] [review]
complete & updated patch

r=cls
Attachment #100488 - Flags: review+
thanks Chris! 

Simon would feel comfortable giving an sr=?
Whiteboard: have patch / need r= and sr= → has r= / needs sr=
not really. Maybe Dan V. can?
thanks Simon, I've sent him an email... I really hope this patch can make it
into 1.2.
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 on attachment 100488 [details] [diff] [review]
complete & updated patch

rs=sfraser
Attachment #100488 - Flags: superreview+
thanks Simon!
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
*** Bug 173502 has been marked as a duplicate of this bug. ***
Yeah!!! With today's trunk, it looks great to me :-)
MILLION THANKS to jbetak! This is going to benefit us a lot. 
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 ;-)
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.
Status: RESOLVED → VERIFIED
Thanks kairo!

ying, rchen, mcarlson - does l10n need this on the branch (bug 173552)? If so,
would early next week be a workable timeframe?
Thanks! early next week would be fine.
This is so great!
nominating for the branch -- please seek drivers and adt approval to land.
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.
Keywords: adt1.0.2adt1.0.2+
*** Bug 173552 has been marked as a duplicate of this bug. ***
Blocks: blackbird
Has this landed on the branch yet?
Dan, yes it has - see bug 173552. Updating keywords.
*** Bug 172075 has been marked as a duplicate of this bug. ***
Depends on: 180372
No longer blocks: 157673
Product: Core → Mozilla Application Suite
You need to log in before you can comment on or make changes to this bug.