Closed Bug 555512 Opened 15 years ago Closed 15 years ago

Indent removed-files.in for readability

Categories

(Firefox :: Installer, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3.7a5

People

(Reporter: khuey, Assigned: khuey)

References

Details

Attachments

(2 files, 3 obsolete files)

Allows whitespace at the beginning of line in removed-files.in. This strips the whitespace at preprocessing time to avoid needing to change all of the removed-files consumers.
Attachment #435440 - Flags: review?(robert.bugzilla)
Attached patch Reorganize removed-files.in (obsolete) (deleted) — Splinter Review
Indent and alphabetize.
Attachment #435441 - Flags: review?(robert.bugzilla)
Attachment #435440 - Flags: review?(robert.bugzilla) → review+
Comment on attachment 435441 [details] [diff] [review] Reorganize removed-files.in Since problems with this file are fairly catastrophic could you attach an updated patch? I should be able to get to it quickly now so it won't bitrot.
Attachment #435441 - Flags: review?(robert.bugzilla) → review-
Attached patch Reorganize removed-files.in (obsolete) (deleted) — Splinter Review
Attachment #435441 - Attachment is obsolete: true
Attachment #442810 - Flags: review?(robert.bugzilla)
Attached patch V2.1 (obsolete) (deleted) — Splinter Review
Attachment #442810 - Attachment is obsolete: true
Attachment #442823 - Flags: review?(robert.bugzilla)
Attachment #442810 - Flags: review?(robert.bugzilla)
Comment on attachment 442823 [details] [diff] [review] V2.1 >+#ifdef MOZ_ENABLE_LIBXUL >+ @DLL_PREFIX@xpcom_core@DLL_SUFFIX@ >+ components/@DLL_PREFIX@jar50@DLL_SUFFIX@ >+ #ifdef XP_WIN >+ components/jsd3250.dll >+ components/nsPostUpdateWin.js please move to the #ifdef XP_WIN without the #ifdef MOZ_ENABLE_LIBXUL >+ components/xpinstal.dll >+ #else >+ components/@DLL_PREFIX@jsd@DLL_SUFFIX@ >+ components/@DLL_PREFIX@xpinstall@DLL_SUFFIX@ >+ #endif >+#else >+ #ifdef XP_MACOSX >+ XUL >+ #else >+ @DLL_PREFIX@xul@DLL_SUFFIX@ >+ #endif >+#endif >+#ifndef MOZ_UPDATER >+ components/nsUpdateService.js >+ components/nsUpdateServiceStub.js >+#endif I'd also prefer it if the #ifdef PLATFORM sections were before all other #ifdef's > #ifndef XP_MACOSX >-components/autocomplete.xpt >+ components/autocomplete.xpt > #endif I believe this should be removed here and below and put in the all platforms section >... >+#ifdef XP_UNIX >+ #ifndef XP_MACOSX >+ chrome/icons/default/default.xpm >+ components/libimgicon.so >+ dictionaries/PL.aff >+ dictionaries/PL.dic >+ icons/mozicon16.xpm >+ icons/mozicon50.xpm >+ libjemalloc.so >+ readme.txt >+ #endif >+#endif >+#ifndef XP_WIN >+ res/fonts/mathfontSymbol.properties >+#endif please move to the #ifdef XP_WIN below Can you run these two patches through the try server and compare the removed-files before and after on Linux and Mac OS X? If not, I should be able to. I want to be 100% sure nothing is added / missing when this lands since that can cause havoc.
Attachment #442823 - Flags: review?(robert.bugzilla) → review-
btw: I verified the output on Windows already
Comment on attachment 442823 [details] [diff] [review] V2.1 >diff --git a/browser/installer/removed-files.in b/browser/installer/removed-files.in >--- a/browser/installer/removed-files.in >+++ b/browser/installer/removed-files.in >@@ -1,94 +1,829 @@ >+# Removed-files.in is processed at build time to create a list of files that >+# should be removed by the installer. These files are in alphabetical order, >+# except that files removed only on certain platforms are after all of the >+# regular files and obsolete Talkback and Inspector files are at the very end. s/should be removed by the installer/should be removed during an application update and by the Windows installer/
(In reply to comment #6) > (From update of attachment 442823 [details] [diff] [review]) > >+#ifdef MOZ_ENABLE_LIBXUL > >+ @DLL_PREFIX@xpcom_core@DLL_SUFFIX@ > >+ components/@DLL_PREFIX@jar50@DLL_SUFFIX@ > >+ #ifdef XP_WIN > >+ components/jsd3250.dll > >+ components/nsPostUpdateWin.js > please move to the #ifdef XP_WIN without the #ifdef MOZ_ENABLE_LIBXUL > > >+ components/xpinstal.dll > >+ #else > >+ components/@DLL_PREFIX@jsd@DLL_SUFFIX@ > >+ components/@DLL_PREFIX@xpinstall@DLL_SUFFIX@ > >+ #endif > >+#else > >+ #ifdef XP_MACOSX > >+ XUL > >+ #else > >+ @DLL_PREFIX@xul@DLL_SUFFIX@ > >+ #endif > >+#endif > >+#ifndef MOZ_UPDATER > >+ components/nsUpdateService.js > >+ components/nsUpdateServiceStub.js > >+#endif > I'd also prefer it if the #ifdef PLATFORM sections were before all other > #ifdef's > > > #ifndef XP_MACOSX > >-components/autocomplete.xpt > >+ components/autocomplete.xpt > > #endif > I believe this should be removed here and below and put in the all platforms > section > > >... > >+#ifdef XP_UNIX > >+ #ifndef XP_MACOSX > >+ chrome/icons/default/default.xpm > >+ components/libimgicon.so > >+ dictionaries/PL.aff > >+ dictionaries/PL.dic > >+ icons/mozicon16.xpm > >+ icons/mozicon50.xpm > >+ libjemalloc.so > >+ readme.txt > >+ #endif > >+#endif > >+#ifndef XP_WIN > >+ res/fonts/mathfontSymbol.properties > >+#endif > please move to the #ifdef XP_WIN below forget that one... misread it as ifdef when it is actually ifndef
I'll push this to try so the before / after removed-files can be compared
I've verified that the before and after remove-files for Linux and Windows are the same... just need to verify Mac OS X now. Looking good
Mac OS X is good to go as well. Please resubmit with the requested changes and I'll plus it after a quick look over to verify the ordering of the different sections. The EM rewrite backout changed removed-files.in so the patch will need to be updated for that as well. Let me know if you would like me to check this in... I don't recall whether you have commit access. Cheers and thanks
Attached patch V2.2 (deleted) — Splinter Review
Updated as asked. I've pushed this to tryserver again so we can make that verification. I can push this to m-c as well once it gets r+.
Attachment #442823 - Attachment is obsolete: true
Attachment #442992 - Flags: review?(robert.bugzilla)
Builds are at https://build.mozilla.org/tryserver-builds/me@kylehuey.com-try-0aaab16bb888/. Looks good on Windows and Linux, I don't know of any way to open a DMG image on Windows to verify the Mac version though.
Comment on attachment 442992 [details] [diff] [review] V2.2 >diff --git a/browser/installer/removed-files.in b/browser/installer/removed-files.in >--- a/browser/installer/removed-files.in >+++ b/browser/installer/removed-files.in >@@ -1,96 +1,835 @@ >... >+xpicleanup@BIN_SUFFIX@ >+#ifdef XP_MACOSX >+ ../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 >+ ../Resources/firefox-bin.rsrc >+ components/accessibility.xpt >+ components/alerts.xpt >+ components/appshell.xpt >+ components/appstartup.xpt >+ components/autocomplete.xpt You added components/autocomplete.xpt in the non ifdef section and removed it in #ifndef XP_MACOSX but forgot to remove the it in #ifdef XP_MACOSX dmg's can be opened by 7-zip. I took a look at the try server builds (the only ones with me@kylehuey.com in the name dated today) and something appears to be busted on at least Mac OS X and Windows... I didn't check Linux. Windows removed-files from try server xpicleanup.exe components/brwsrcmp.dll components/jsd3250.dll components/nsPostUpdateWin.js js3250.dll #ifdef MOZ_MEMORY Microsoft.VC80.CRT.manifest msvcm80.dll msvcp80.dll msvcr80.dll #else mozcrt19.dll #endif xpcom_core.dll components/jar50.dll #ifdef XP_WIN components/xpinstal.dll #else components/jsd.dll components/xpinstall.dll #endif extensions/talkback@mozilla.org/components/BrandRes.dll extensions/talkback@mozilla.org/components/fullsoft.dll extensions/talkback@mozilla.org/components/master.ini extensions/talkback@mozilla.org/components/talkback-l10n.ini extensions/talkback@mozilla.org/components/talkback.cnt extensions/talkback@mozilla.org/components/talkback.exe extensions/talkback@mozilla.org/components/talkback.hlp extensions/talkback@mozilla.org/InstallDisabled components/BrandRes.dll Mac OS X removed-files from try server xpicleanup ../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 <snip> updater.app/Contents/MacOS/updater.ini #ifndef XP_MACOSX chrome/icons/default/default.xpm components/libimgicon.so dictionaries/PL.aff dictionaries/PL.dic icons/mozicon16.xpm icons/mozicon50.xpm libjemalloc.so readme.txt #endif res/fonts/mathfontSymbol.properties libxpcom_core.dylib components/libjar50.dylib #ifdef XP_WIN components/xpinstal.dll #else components/libjsd.dylib components/libxpinstall.dylib #endif #ifdef XP_MACOSX extensions/talkback@mozilla.org/components/talkback/master.ini extensions/talkback@mozilla.org/components/talkback/talkback.dylib extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Info.plist extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/MacOS/Talkback extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/pbdevelopment.plist extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/PkgInfo extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Resources/delete.tiff extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Resources/disable.tiff extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Resources/enable.tiff extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Resources/English.lproj/ArchivingSettings.nib/classes.nib extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Resources/English.lproj/ArchivingSettings.nib/info.nib extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Resources/English.lproj/ArchivingSettings.nib/objects.nib extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Resources/English.lproj/InfoPlist.strings extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Resources/English.lproj/IntroWizard.nib/classes.nib extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Resources/English.lproj/IntroWizard.nib/info.nib extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Resources/English.lproj/IntroWizard.nib/objects.nib extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Resources/English.lproj/Localizable.strings extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Resources/English.lproj/MainMenu.nib/classes.nib extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Resources/English.lproj/MainMenu.nib/info.nib extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Resources/English.lproj/MainMenu.nib/objects.nib extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Resources/English.lproj/ProxySettings.nib/classes.nib extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Resources/English.lproj/ProxySettings.nib/info.nib extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Resources/English.lproj/ProxySettings.nib/objects.nib extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Resources/English.lproj/SendingSettings.nib/classes.nib extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Resources/English.lproj/SendingSettings.nib/info.nib extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Resources/English.lproj/SendingSettings.nib/objects.nib extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Resources/KeyInfoKeys.plist extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Resources/KeyInfoSections.plist extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Resources/send.tiff extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Resources/sort_ascending.tiff extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Resources/sort_descending.tiff extensions/talkback@mozilla.org/components/talkback/Talkback.app/Contents/Resources/Talkback.icns #else extensions/talkback@mozilla.org/components/talkback/talkback extensions/talkback@mozilla.org/components/talkback/XTalkback.ad extensions/talkback@mozilla.org/components/talkback/master.ini extensions/talkback@mozilla.org/components/talkback/talkback.so #endif components/BrandRes.dll My own try server builds to verify with slight modifications to the removed-files patch didn't exhibit this problem.
Attachment #442992 - Flags: review?(robert.bugzilla) → review-
The weirdness is because I forgot to push the first patch as well :-P
Comment on attachment 442992 [details] [diff] [review] V2.2 Doh! Thanks for the explanation and r=me with autocomplete.xpt comment fixed.
Attachment #442992 - Flags: review- → review+
Mossop, just a heads up regarding changes to removed-files.in for when you merge to addonsmgr branch or land the EM rewrite bugs again on m-c.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a5
Blocks: 563275
Blocks: 541824
No longer blocks: 563275
Flags: in-testsuite-
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: