Closed Bug 224230 Opened 22 years ago Closed 21 years ago

Calendar XPI structure differs from the standard xpi structure

Categories

(Calendar :: General, defect)

Product:
Calendar
Component:
General
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: omgs, Assigned: Callek)

References

(
URL
)

Details

Attachments

(3 files, 5 obsolete files)

Step 1 -- Add new local makefile to allmakefiles.sh
787 bytes, patch
benjamin
: first-review+
Details | Diff | Splinter Review
Step2 - Change make xpi and install.js ( revised )
5.00 KB, patch
Callek
: first-review-
Details | Diff | Splinter Review
Step2 - take5
4.31 KB, patch
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (X11; U; Linux i686; es-ES; rv:1.4) Gecko/20030625 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; es-ES; rv:1.4) Gecko/20030625 The xpi files for calendar, in the "locale" branch, have the structure "locale/en-US/files", which differs from the standard way, that should be "locale/en-US/calendar/files". Reproducible: Always Steps to Reproduce:
Shouldn't this be closed? Mostafah, please do, for if it's something related to bugzilla's recent migration. Or if you want me to close it.
The locales different from en-US, apart from being in a different directory, don't follow the same structure as the fixed en-US
Status: RESOLVED → UNCONFIRMED
Resolution: FIXED → ---
I have additional issues for this, apart that one started, I can't see any reason to finish the work properly. So, let's try to make calendar's installer the best ever seen :-) First, take a look at chatzilla xpi structure . |-- bin | |-- chrome | | `-- chatzillaeses.jar | `-- components | `-- chatzilla-service.js `-- install.js You can spread under "bin" for any structure Then, I'll attach my proposed install.js
Attached file Install.js not created by me (obsolete) —
This is another work, intended to somehow merge with attachment 144662 [details]
*** Bug 245209 has been marked as a duplicate of this bug. ***
Confirming and Assigning to me, added url to point to Ben's EM notes.
Assignee: mostafah → jw6057
Blocks: 245208
URL: http://www.bengoodger.com/software/mb...
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
This only converts Makefile.in to Makefile does not actually run it yet.
Comment on attachment 149780 [details] [diff] [review] Step 1 -- Add new local makefile to allmakefiles.sh This should follow standard calendar review (imo) and not need an sr. if you agree bsmedberg would you comit?
Attachment #149780 - Flags: first-review?(bsmedberg)
Comment on attachment 149780 [details] [diff] [review] Step 1 -- Add new local makefile to allmakefiles.sh Changes to allmakefiles.sh rarely need review...
Attachment #149780 - Flags: first-review?(bsmedberg) → first-review+
Comment on attachment 149780 [details] [diff] [review] Step 1 -- Add new local makefile to allmakefiles.sh Patch checked in on trunk, by timeless
Attached patch Step 2 -- (See c#14) (obsolete) — — Splinter Review
Modifies ~ calendar/resources/Makefile.in to ensure locale .jar's are built Modifies ~ calendar/Makefile.in target xpi:: to sort and store correctly the new structure. targets macxpi and openvmsxpi not updated in this patch. Modifies ~ calendar/install.js to add the new locale .jar files and use our new structure If you try and use macxpi and openvmsxpi with this as stands, the xpi will not work, directories are not layed out correctly. New directory structure: xpi/ | - install.js | - components/ | | - $(LIB_PREFIX)xpical$(DLL_SUFFIX) | | - calendar.xpt | | - calendarService.js | - chrome/ | | - calendar.jar | | - locale/ | | | - calendar-....jar (each local) those dir's are to be used in the EM I also stuck in "other_stuff" directory, which will expand when installed with EM, but not get installed anywhere. current file(s) in there are icons/ directory for calendar (which install.js does grab) and the linux init.d script
Attachment #144662 - Attachment is obsolete: true
Attachment #144663 - Attachment is obsolete: true
Attachment #149850 - Flags: first-review?(mostafah)
The previous patch put us in the right direction, but needed some tweaks to actaully work. Thanks This new patch is the revised version.
Attachment #149850 - Attachment is obsolete: true
Checking in patch and ccing matsuba@dream.com for input. Oscar: It would be good if you gave us feedback too.
Attachment #149850 - Flags: first-review?(mostafah)
Comment on attachment 149871 [details] [diff] [review] Step2 - Change make xpi and install.js ( revised ) This still needs a review for EM compatibility
Attachment #149871 - Flags: first-review?(jw6057)
Comment on attachment 149871 [details] [diff] [review] Step2 - Change make xpi and install.js ( revised ) Not marking review - for EM stuff, that looks fine > if ( File.exists(tmp_f) ) { >- registerChrome(LOCALE | DELAYED_CHROME, calendarLocale, addLocales[i] + "/"); >+ err = addFile( "Calendar Chrome-"+addLocales[i], >+ "chrome/calendar-"+addLocales[i]+".jar", // jar source folder >+ getFolder("Chrome"), // target folder >+ ""); >+ registerChrome(LOCALE | DELAYED_CHROME, getFolder("Chrome","calendar-"+addLocales[i]+".jar"), >+ "locale/"+addLocales[i]+"/calendar/"); > } Correct me if I am wrong, but would doing an err = addFile here ensure that if infact there is an error, that the registerChrome would then cause problems, since there won't be any file as listed. > // Check Mozilla Thunderbird (Mail/News) > chkJarFileName = addLocales[i] + "-mail.jar"; > tmp_f = getFolder("Chrome", chkJarFileName); > if ( File.exists(tmp_f) ) { >- registerChrome(LOCALE | DELAYED_CHROME, calendarLocale, addLocales[i] + "/"); >+ err = addFile( "Calendar Chrome-"+addLocales[i], >+ "chrome/calendar-"+addLocales[i]+".jar", // jar source folder >+ getFolder("Chrome"), // target folder >+ ""); >+ registerChrome(LOCALE | DELAYED_CHROME, getFolder("Chrome","calendar-"+addLocales[i]+".jar"), >+ "locale/"+addLocales[i]+"/calendar/"); Same here ------------------ Everything else looks good Except you missed my Makefile.in changes to "calendar/resources/" which told us to run the Makefile in locale (to create the .jar files) as it stands now, there are NO .jar files to create. Once these nits are fixed, I /think/ we can mark this bug fixed, since a new one can be made for icon when its ready in EM.
Attachment #149871 - Flags: first-review?(jw6057) → first-review-
Please update patch: cp $(DEPTH)/dist/bin/chrome/calendar-*.jar ./xpi/chrome --> cp $(DEPTH)/dist/bin/chrome/calendar.jar ./xpi/chrome ************************* *Created XPI (calendar.xpi) ************************* (*)Create xpi by using mozilla/calendar/Makefile. xpi/ | - install.js | | - components/ | | - $(LIB_PREFIX)xpical$(DLL_SUFFIX) => libxpical.so | | - calendar.xpt | | - calendarService.js | | - chrome/ | | - calendar.jar | | | - content/ | | | - skin/ | | | - classic/ | | | - modern/ | | | | | - locale/ | | | - en-US <---- Only | | | | - other_stuff/ | | - bin/ | | | - init.d/ | | | - S80calendar_fix_permissions_bug_230617 | | | | - icons/ | | | - default/ | | | - calendar-window.xpm | | | - calendar-window16.xpm
> Correct me if I am wrong, but would doing an err = addFile here ensure that if > infact there is an error, that the registerChrome would then cause problems, > since there won't be any file as listed. That would be better, yes. > Everything else looks good Except you missed my Makefile.in changes to > "calendar/resources/" which told us to run the Makefile in locale (to create > the .jar files) as it stands now, there are NO .jar files to create. I forgot to say why I removed that part. The reason is that by adding that line, everyone who builds with --enable-calendar will get the locale files too. That is not desired. It will result in a lot locale files being copied to dist/bin/chrome without use. The Makefile in resources/locale is to make it possible for *xpi builders* to easily get the langpacks by manually doing a make there. It would've been desirable to enforce that in the "make xpi" section but I didn't find a straight way to do so. I thought of "cd resources/locale;make" in "make xpi" but wasn't sure it would work for objdir builders.
> Please update patch: > > cp $(DEPTH)/dist/bin/chrome/calendar-*.jar ./xpi/chrome > --> cp $(DEPTH)/dist/bin/chrome/calendar.jar ./xpi/chrome That section is to make sure calendar-<locale>.jar files get copied into the xpi. Anyone who builds the xpi must perform an extra step which is "cd resources/locale;make". (See previous comment).
(In reply to comment #21) > That section is to make sure calendar-<locale>.jar files get copied into the > xpi. Anyone who builds the xpi must perform an extra step which is "cd > resources/locale;make". (See previous comment). Oh. I'm sorry. I understood it. Thanks.
Ok mostafah, let me try to address all these. As you currently did the addFile for locales, there is nothing which verifies that err is valid (I'll see if I can throw an update to that later. and I definately see your point on removing the DIR change, I can fix it in the xpi target easy, I'll make an updated patch based off of your comit.
Attached patch Step 2: take 3. (obsolete) — — Splinter Review
not obsolete-ing attachment #149871 [details] [diff] [review] (since that was checked in). This patch does a few things. First: In Makefile.in ensures that the other locale's get built into .jar files. Second: In Makefile.in allows about.html NOT to be shown as 'M' on local computer after an xpi build Third: In install.js modifies variable addLocales to existingLocales to better say what it now does, also added variables addLocales and registerLocales for error-testing. Fourth: Made sure that when addFile for locale's there is no error, and check it in the correct place. Fifth: Add error checking incase for some reason there is a -mail file in a users chrome directory as well as a normal SM/FF file. (unlikely but possible). Sixth: Clarified default error message.
Attachment #149966 - Flags: first-review?(mostafah)
Comment on attachment 149966 [details] [diff] [review] Step 2: take 3. >+ cp $(srcdir)/resources/content/about.html $(srcdir)/resources/content/about.html.bkup >+ rm -f $(srcdir)/resources/content/about.html.bkup Please don't change anything at all in the srcdir. Copy the file to the objdir forst or something.
I'm taking a look at the install.js. It seems the bug has changed its target (I'm not agains it), in whose case, if know we're looking for a multilanguage installer, I think there should be an option of not installing en-US. I'd like to clarify this, before going further.
mvl the only reason I do this in $(srcdir) is that the about.html is already modified by the xpi script, when that happens there will ALWAYS be a conflict when the actual xpi releases are made and comited (when about.html gets updated on cvs) this step absolves that issue, but copying about.html to a backup, running the command (which we need to do there because we are throwing it into a .jar) I wish it were that easy to not touch the $(srcdir) but its now. In response to c#26 en-US is installed by default, in the normal calendar.jar I don't think this will be changed, the new locale's were already defined and checked against before, though they were _always_ added before and added to the chrome if the files we checked for exist, now that we turn them into .jar files from the locale/Makefile we need two sets of checks like I did here, one to add and one to register. (One could argue we still add them anyway, though mostafah seemed to prefer adding them on a check if they should exist to begin with). Which is why I threw the check outside the chrome-registration section so that we can properly catch an error.
Corrections, because of typos: "I'm taking a look at the install.js. It seems the bug has changed its target (I'm not against it), in whose case, if we're now looking for a multilanguage installer, I think there should be an option of not installing en-US." Then, I think all languages should be treated at the same level, and not supposing things like "english is always needed", what's absolutely false, and if this was true, there would be only one language on earth and not so many different. The _only_ reason why english is installed by default it's that english speakers seem to be the main calendar users, but not the only ones. We (spanish team) provide one-language packs (i.e. es-ES only) because we're certain that most of the people can't speak english and/or feel more comfortable with his natural language. The main reason for filing this bug is that I pack other mozilla extensions, and I had some "standard" scripts made by myself that made my life when packaging much easier, but when I had to pack calendar, it was a headache because of its different structure, and I just wanted to fix this, just for english package to be as standard as other like chatzilla or inspector, nothing beyond that. I suggest to take a look at inspector structure as main example. Unless the rest of packages go to multilanguage installers with this philosophy, this way calendar will be still a headache to pack, because it will still be out of the standard XPI structure, as the summary of the bug claims.
Hmm Oscar, I don't take your comments light-hearted, but we at the least need the xpi structure fixed in the way I am working on for FF 0.9 to even be able to use it--the summary suggests this bug would be it as well (even though your initial description says locale alone), we can re-visit the language issue once FF 0.9 is working, IMO (as in I won't resolve the bug); or, alternatively, we can have another bug, pointing to comments # xxx in this bug with a summary like "en-US should not always be installed", but thats just me.
Attached patch Step2 - take4 (obsolete) — — Splinter Review
This is my suggestion for handling errors when installing locales. (I removed make -C resources/locale since I never understood its purpose). Also note the default_lang="en-US" change which should make it easier for changing the default language.
Attachment #150261 - Flags: first-review?(jw6057)
Comment on attachment 150261 [details] [diff] [review] Step2 - take4 >+const default_lang = "en-US"; (I like this) >- registerChrome(LOCALE | DELAYED_CHROME, getFolder("Chrome","calendar-"+addLocales[i]+".jar"), >- "locale/"+addLocales[i]+"/calendar/"); >+ logComment("addFile() for locale " + addLocales[i] + " returned: " + err); >+ if( err != SUCCESS ) >+ alert( "addFile() for locale " + addLocales[i] + " returned: " + err ); >+ registerChrome(LOCALE | DELAYED_CHROME, getFolder("Chrome","calendar-" +addLocales[i] + ".jar"), >+ "locale/" +addLocales[i] + "/calendar/"); Works for letting the user know, but WE STILL register the chrome even though there was an error, which is why I had originally pulled out >- registerChrome(LOCALE | DELAYED_CHROME, getFolder("Chrome","calendar-"+addLocales[i]+".jar"), >- "locale/"+addLocales[i]+"/calendar/"); >+ logComment("addFile() for locale " + addLocales[i] + " returned: " + err); >+ if( err != SUCCESS ) >+ alert( "addFile() for locale " + addLocales[i] + " returned: " + err ); >+ registerChrome(LOCALE | DELAYED_CHROME, getFolder("Chrome","calendar-" + addLocales[i]+".jar"), >+ "locale/" + addLocales[i]+"/calendar/"); see above. By the way, "make -C resources/locale" says, "run make but switch to directory 'resources/locale' first" so basically since we don't run the locale makefile on --enable-calendar, we need a way to run it before we try and add those jar's to our xpi, that solves it, (Though does also add them to the chrome for `you` when run, but it only happens on `make xpi` better than the other method and doesn't hork things with missing files). I grant review anyway, with those nits to keep in mind ;-)
Attachment #150261 - Flags: first-review?(jw6057) → first-review+
Attached patch Step2 - take5 — — Splinter Review
Thanks for the review. Revised patch to address those issues. I'll check this one in but it will still remain open for improvement. Next we will address the main concern/target of this bug: locales
Attachment #150261 - Attachment is obsolete: true
Comment on attachment 149966 [details] [diff] [review] Step 2: take 3. now obsolete.
Attachment #149966 - Attachment is obsolete: true
Attachment #149966 - Flags: first-review?(mostafah)
Hi, all. I found calendar_linux_20040603.xpi and calendar_linux_20040609.xpi in http://ftp.mozilla.org/pub/mozilla.org/calendar/xpi/linux/ . After installing my Mozilla 1.7rc3 with Japanese Language packs, It seems to be good. But, when I select "Help" -> "About" on Mozilla calendar, it hangup calendar. No error in JS console on Mozilla 1.7rc3. Is this known bug ? calendar_linux_20040609.xpi: other_stuff/ other_stuff/icons/ other_stuff/icons/default/ other_stuff/icons/default/calendar-window.xpm other_stuff/icons/default/calendar-window16.xpm other_stuff/bin/ other_stuff/bin/init.d/ other_stuff/bin/init.d/S80calendar_fix_permissions_bug_230617 chrome/ chrome/calendar.jar chrome/calendar-cs-CZ.jar chrome/calendar-cy-GB.jar chrome/calendar-de-AT.jar chrome/calendar-es-ES.jar chrome/calendar-fr-FR.jar chrome/calendar-hu-HU.jar chrome/calendar-ja-JP.jar chrome/calendar-lt-LT.jar chrome/calendar-nl-NL.jar chrome/calendar-pl-PL.jar chrome/calendar-pt-BR.jar chrome/calendar-sk-SK.jar chrome/calendar-sl-SI.jar chrome/calendar-sv-SE.jar chrome/calendar-wen-DE.jar components/ components/libxpical.so components/calendar.xpt components/calendarService.js install.js
-->In reply to Comment #34 That issue is beyond the scope of this bug, (xpi packaging has nothing to do with it) Would you create a new bug describing your problem, please (I couldnt find a dupe, but please search for one)
(In reply to comment #35) > Would you create a new bug describing your problem, > please (I couldnt find a dupe, but please search for one) Yes, I could not find this, too. I'm sorry. I file it, http://bugzilla.mozilla.org/show_bug.cgi?id=246535.
This bug seems fixed (from a glance) if someone disagree's please Re-Open and tell me why, (so it can be finally fixed)
Status: NEW → RESOLVED
Closed: 21 years ago → 21 years ago
Resolution: --- → FIXED
The bugspam monkeys have been set free and are feeding on Calendar :: General. Be afraid for your sanity!
QA Contact: gurganbl → general
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: