Last Comment Bug 633394 - Update removed-files for findings from 2.1b2 update checks
: Update removed-files for findings from 2.1b2 update checks
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Build Config (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: seamonkey2.1b3
Assigned To: Edmund Wong (:ewong)
:
:
Mentors:
Depends on: 575559
Blocks: 716392 574467 643199
  Show dependency treegraph
 
Reported: 2011-02-10 17:19 PST by Robert Kaiser
Modified: 2012-01-11 13:59 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
[Linux] log/report from updates to 2.1b2 (4.35 KB, text/plain)
2011-02-10 17:19 PST, Robert Kaiser
no flags Details
[Windows] log/report from updates to 2.1b2 (2.12 KB, text/plain)
2011-02-11 09:03 PST, Justin Wood (:Callek)
no flags Details
[Mac] log/report from manual updates to 2.1b2 (5.55 KB, text/plain)
2011-02-13 13:13 PST, Adrian Kalla [:adriank]
no flags Details
Updated removed-files.in for findings from 2.1b2 update checks. (v1) (2.11 KB, patch)
2011-02-14 19:49 PST, Edmund Wong (:ewong)
kairo: feedback-
Details | Diff | Splinter Review
Updated removed-files.in for findings from 2.1b2 update checks. (v2) (5.21 KB, patch)
2011-02-17 18:27 PST, Edmund Wong (:ewong)
bugspam.Callek: review-
Details | Diff | Splinter Review
Update removed-files.in for findings from 2.1b2 update checks. (4.22 KB, patch)
2011-02-20 22:00 PST, Edmund Wong (:ewong)
bugspam.Callek: review-
Details | Diff | Splinter Review
Update removed-files.in for findings from 2.1b2 update checks. (v3) (4.89 KB, patch)
2011-03-03 06:52 PST, Edmund Wong (:ewong)
no flags Details | Diff | Splinter Review
Update removed-files.in for findings from 2.1b2 update checks. (v4) (4.89 KB, patch)
2011-03-09 06:51 PST, Edmund Wong (:ewong)
bugspam.Callek: review-
Details | Diff | Splinter Review
Updated removed-files.in for findings from 2.1b2 update checks. (v5) (5.23 KB, patch)
2011-03-09 22:38 PST, Edmund Wong (:ewong)
bugspam.Callek: review+
Details | Diff | Splinter Review

Description Robert Kaiser 2011-02-10 17:19:49 PST
Created attachment 511579 [details]
[Linux] log/report from updates to 2.1b2

I have installed all previous 2.1* milestone builds and 2.0.12 in en-US and de as available (all Linux), updated them either via auto-update or manually using the complete.mar to 2.1b2 and diffed a sorted output of |find .| of plain 2.1b2 installs against those updated builds.

This showed not just that the partial update for 2.1b2 has problems (will file another bug for that) but also that complete MAR works fine in terms of builds, but a few files are left in the system that we still should clean up with removed-files. I ignored left-over empty directories as updater can't currently remove them (bug is IIRC on file), and there is an open bug on even Firefox 4 not correctly removing old locale files (so let's leave that part to a different bug as well). Everything else is something we should add.

I'm attaching my plain update log that includes the relevant diff parts.
Comment 1 Justin Wood (:Callek) 2011-02-11 09:03:20 PST
Created attachment 511749 [details]
[Windows] log/report from updates to 2.1b2

I just performed the same matrix on windows.
Comment 2 Adrian Kalla [:adriank] 2011-02-13 13:13:33 PST
Created attachment 512050 [details]
[Mac] log/report from manual updates to 2.1b2

report from manual 2.0.12,2.1b1 updates to 2.1b2 (I couldn't get the autoupdates to work...)
Comment 3 Edmund Wong (:ewong) 2011-02-14 19:49:01 PST
Created attachment 512390 [details] [diff] [review]
Updated removed-files.in for findings from 2.1b2 update checks. (v1)

For Linux and Windows.
Comment 4 Justin Wood (:Callek) 2011-02-14 22:10:10 PST
Comment on attachment 512390 [details] [diff] [review]
Updated removed-files.in for findings from 2.1b2 update checks. (v1)

I'll give it a real review later, but one-off comments. [Hold on new attachment until my real review though]

> #endif
>+modules/HUDService.jsm
> modules/JSON.jsm
> modules/dbViewWrapper.js
> modules/mailViewManager.js
> modules/quickSearchManager.js
> modules/searchSpec.js
>+modules/autoconfigUtils.jsm
> #ifdef MOZ_STATIC_JS
> @DLL_PREFIX@mozjs@DLL_SUFFIX@
> #endif

please sort this section (e.g. put autoconfigUtils.jsm above dbViewWrapper.js)

>@@ -498,6 +519,9 @@ xpicleanup@BIN_SUFFIX@
>   components/interfaces.manifest
>   components/mail.xpt
>   components/components.manifest
>+  #ifdef XP_UNIX
>+    components/nsFilePicker.js
>+  #endif
>   defaults/profile/bookmarks.html
>   defaults/profile/mimeTypes.rdf
>   defaults/profile/chrome

This was already listed above.
Comment 5 Robert Kaiser 2011-02-15 07:20:56 PST
Comment on attachment 512390 [details] [diff] [review]
Updated removed-files.in for findings from 2.1b2 update checks. (v1)

I also want to take a look at this patch. And thanks for writing it up!
Comment 6 Robert Kaiser 2011-02-16 16:59:00 PST
Comment on attachment 512390 [details] [diff] [review]
Updated removed-files.in for findings from 2.1b2 update checks. (v1)

>+chrome/en-US.manifest

No, this should be chrome/@AB_CD@.manifest - but we need to see that replacing this variable actually works even in the locale repack scenario.

>+#ifdef XP_UNIX
>+components/nsFilePicker.js
>+#endif

That one's already in the file, but with an ifdef that isn't defined in this scope. Either change to your ifdef or copy the definition from package-manifest.in over or (which I think would be best) migrate this to be added to the defines in the Makefile instead.

> modules/dbViewWrapper.js
> modules/mailViewManager.js
> modules/quickSearchManager.js
> modules/searchSpec.js
>+modules/autoconfigUtils.jsm

Sort this to be before dbViewWrapper, please.

>+  #ifdef XP_UNIX
>+    components/nsFilePicker.js
>+  #endif

No need to add this twice.

Also, please add the entries from the 2.0.12 update test in attachment 512050 [details] - at least those inside Contents/MacOS/ (of course using an #ifdef XP_MACOSX). Not sure if we can do anything about those outside the MacOS/ dir there, but a look into the Firefox removed-files.in might show something up there.

feedback- for addressing those comments. Many thanks for looking into this, though, the packaging stuff is tedious work, I know...

(We really should figure out the locales stuff, but I think that needs a look into repack logic and that's not for the faint of the heart, and I think FF has the same "problems" there, so I still hop to see a solution from them and us just following. Probably should all go into its own bug, though.)
Comment 7 Justin Wood (:Callek) 2011-02-16 18:44:22 PST
(In reply to comment #6)
> Comment on attachment 512390 [details] [diff] [review]
> Updated removed-files.in for findings from 2.1b2 update checks. (v1)
> >+chrome/en-US.manifest
> No, this should be chrome/@AB_CD@.manifest - but we need to see that replacing
> this variable actually works even in the locale repack scenario.

Actually I am fairly sure this WONT work in Firefox; since I recall reading a bug there that mentioned this was wrong, I'll plan to lookup that bug.
Comment 8 Robert Kaiser 2011-02-17 06:52:37 PST
(In reply to comment #7)
> Actually I am fairly sure this WONT work in Firefox; since I recall reading a
> bug there that mentioned this was wrong, I'll plan to lookup that bug.

True. This still doesn't make using @AB_CD@ here wrong, as for the default case it resolves fine to en-US - and then we'll only need to make the repack case working, but will already have the correct thing in here.
Comment 9 Edmund Wong (:ewong) 2011-02-17 18:27:57 PST
Created attachment 513343 [details] [diff] [review]
Updated removed-files.in for findings from 2.1b2 update checks. (v2)
Comment 10 Robert Kaiser 2011-02-20 12:29:41 PST
Comment on attachment 513343 [details] [diff] [review]
Updated removed-files.in for findings from 2.1b2 update checks. (v2)

> #ifdef XP_MACOSX
[...]
>+  chrome/classic.manifest
etc.

No need to list those again that are already listed for all platforms and added globally in this patch!

This includes

>+  extensions/langpack-pl@chatzilla.mozilla.org/chrome.manifest
>+  extensions/langpack-pl@chatzilla.mozilla.org/chrome/chatzilla.jar
>+  extensions/langpack-pl@chatzilla.mozilla.org/chrome/chatzilla.manifest
>+  extensions/langpack-pl@chatzilla.mozilla.org/install.js
>+  extensions/langpack-pl@chatzilla.mozilla.org/install.rdf
>+  extensions/langpack-pl@venkman.mozilla.org/chrome.manifest
>+  extensions/langpack-pl@venkman.mozilla.org/chrome/venkman.jar
>+  extensions/langpack-pl@venkman.mozilla.org/chrome/venkman.manifest
>+  extensions/langpack-pl@venkman.mozilla.org/install.js
>+  extensions/langpack-pl@venkman.mozilla.org/install.rdf

which fall under the whole @AB_CD@ "problem" though ("pl" is the AB_CD value for Polish).

>+#ifdef XP_UNIX
>+#ifndef XP_MACOSX
>+#define UNIX_NOT_MAC
>+#endif
>+#endif
>+

Please put that at the start of the file, just like it's done for package-manifest.in as well.
Comment 11 Justin Wood (:Callek) 2011-02-20 21:37:49 PST
Comment on attachment 513343 [details] [diff] [review]
Updated removed-files.in for findings from 2.1b2 update checks. (v2)

I suspect I'll have nits for a v4, but KaiRo's comments warrant another go at this before I stick my nose too deep.
Comment 12 Edmund Wong (:ewong) 2011-02-20 22:00:46 PST
Created attachment 513930 [details] [diff] [review]
Update removed-files.in for findings from 2.1b2 update checks.
Comment 13 Justin Wood (:Callek) 2011-03-02 20:57:00 PST
Comment on attachment 513930 [details] [diff] [review]
Update removed-files.in for findings from 2.1b2 update checks.

Switch the def at top from UNIX_NOT_MAC to UNIX_BUT_NOT_MAC please

>+#ifdef XP_UNIX
>+components/nsFilePicker.js
>+#endif

use UNIX_BUT_NOT_MAC here please.

>+extensions/inspector@mozilla.org/components/components.list
> #ifdef UNIX_BUT_NOT_MAC
^^^ We missed this tidbit before [was never defined] (not an issue from your patch)

> #ifdef XP_MACOSX
>- ../Plug-Ins/PrintPDE.plugin/Contents/Info.plist

Keep all these (../ beginning) lines at the start of this ifdef please.

>+  components/mozldap.xpt
>+  components/nsAboutAbout.js

This [nsAboutAbout.js] (and others) that are universally removed (elsewhere) that you are adding, please don't re-add them here.

>+  plugins/DefaultPlugin.plugin/Contents/Resources/English.lproj

I'm less certain on these plugins/ changes, but I would feel 100% confident here if we just copied what Firefox has at http://mxr.mozilla.org/mozilla-central/source/browser/installer/removed-files.in?mark=1212-1234#1212 to this spot.

while we are at it, lets add |updater.app/Contents/MacOS/updater.ini| to the mac ifdef here, (Firefox has it). I'm not sure if we need it, but I do know it won't hurt.

r- based on the above. I think we're almost there!
Comment 14 Edmund Wong (:ewong) 2011-03-03 06:52:08 PST
Created attachment 516584 [details] [diff] [review]
Update removed-files.in for findings from 2.1b2 update checks. (v3)
Comment 15 Robert Kaiser 2011-03-06 18:00:59 PST
Comment on attachment 516584 [details] [diff] [review]
Update removed-files.in for findings from 2.1b2 update checks. (v3)

This is very close, but...

>+  ../Plug-Ins/PrintPDE.plugin/Contents/Resources/English.lproj
>+  ../Plug-Ins/PrintPDE.plugin/Contents/Resources/English.lproj/PrintPDE.nib
>+  ../Resources/seamonkey-bin.rsrc
>+  ../Plug-Ins/PrintPDE.plugin/Contents/Info.plist
>+  ../Plug-Ins/PrintPDE.plugin/Contents/MacOS/PrintPDE
>+  ../Plug-Ins/PrintPDE.plugin/Contents/Resources/English.lproj/Localizable.strings
>+  ../Plug-Ins/PrintPDE.plugin/Contents/Resources/English.lproj/PrintPDE.nib/classes.nib
>+  ../Plug-Ins/PrintPDE.plugin/Contents/Resources/English.lproj/PrintPDE.nib/info.nib
>+  ../Plug-Ins/PrintPDE.plugin/Contents/Resources/English.lproj/PrintPDE.nib/objects.xib

Could you please order those the first two (which are directories, so make them end in /) actually go before the entries inside those dirs?

>-plugins/DefaultPlugin.plugin/
>+../plugins/DefaultPlugin.plugin/

No, this was correct.

> plugins/DefaultPlugin.plugin/Contents/
> plugins/DefaultPlugin.plugin/Contents/Info.plist
> plugins/DefaultPlugin.plugin/Contents/MacOS/

And those are duplicated above and here - no chance to have them in a single section only?

The rest looks good to me from a fast glance!
Comment 16 Edmund Wong (:ewong) 2011-03-09 06:51:27 PST
Created attachment 518053 [details] [diff] [review]
Update removed-files.in for findings from 2.1b2 update checks. (v4)
Comment 17 Justin Wood (:Callek) 2011-03-09 21:30:29 PST
Comment on attachment 518053 [details] [diff] [review]
Update removed-files.in for findings from 2.1b2 update checks. (v4)

Mostly good. (Note I tried out reviewboard for once, so I could get more context easily; the URL for this is http://reviews.visophyte.org/r/518053/ note the first two reviews posted there by me were failures on my part to understand that system. -- The line #'s below are referencing an already patched file)

on file: suite/installer/removed-files.in line 146
> #ifdef XP_UNIX
> components/nsFilePicker.js
> #endif

Use UNIX_BUT_NOT_MAC here please


on file: suite/installer/removed-files.in line 257
>   ../Resources/seamonkey-bin.rsrc

Sort this alphabetically to below the ../Plug-Ins please


on file: suite/installer/removed-files.in line 275
>   plugins/DefaultPlugin.plugin/Contents/Resources/English.lproj/InfoPlist.strings
>   plugins/DefaultPlugin.plugin/Contents/Resources/plugin.png

You missed two lines here from the Firefox section I identified:
http://mxr.mozilla.org/mozilla-central/source/browser/installer/removed-files.in?mark=1217-1220,1217,1218#1212


on file: suite/installer/removed-files.in line 300
> plugins/DefaultPlugin.plugin/
> plugins/DefaultPlugin.plugin/Contents/
> plugins/DefaultPlugin.plugin/Contents/Info.plist
> plugins/DefaultPlugin.plugin/Contents/MacOS/
> plugins/DefaultPlugin.plugin/Contents/MacOS/DefaultPlugin
> plugins/DefaultPlugin.plugin/Contents/Resources/
> plugins/DefaultPlugin.plugin/Contents/Resources/English.lproj/
> plugins/DefaultPlugin.plugin/Contents/Resources/English.lproj/InfoPlist.strings

Please also remove these since they are above.
Comment 18 Edmund Wong (:ewong) 2011-03-09 22:38:02 PST
Created attachment 518289 [details] [diff] [review]
Updated removed-files.in for findings from 2.1b2 update checks. (v5)
Comment 19 Justin Wood (:Callek) 2011-03-09 22:43:01 PST
Comment on attachment 518289 [details] [diff] [review]
Updated removed-files.in for findings from 2.1b2 update checks. (v5)

Perfect, thank you.
Comment 20 Robert Kaiser 2011-03-20 08:39:16 PDT
Pushed as http://hg.mozilla.org/comm-central/rev/c74e6965bd50 - thanks a lot for working on this one!

Note You need to log in before you can comment on or make changes to this bug.