Open Bug 526333 Opened 15 years ago Updated 2 years ago

Having to create patches for each app's package manifest and then get reviews from each app is a PITA

Categories

(Firefox Build System :: General, defect)

defect

Tracking

(Not tracked)

People

(Reporter: robert.strong.bugs, Unassigned)

References

Details

Attachments

(2 files, 13 obsolete files)

2.98 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
35.00 KB, patch
Details | Diff | Splinter Review
This will make it much easier to get patches landed such as bug 311965 where we need to update each toolkit app and therefore get reviews from each toolkit app.
Changing summary to reflect the problem instead of a proposed solution

Old summary was a proposed solution of:
Add a toolkit package manifest and removed-files so app can include these in their package manifests and removed-files
Summary: Add a toolkit package manifest and removed-files so app can include these in their package manifests and removed-files → Having to create patches for each app's package manifest and removed-files and then get reviews from each app is a PITA
Although yes, your original statement sounds like the right solution.
Version: unspecified → Trunk
Ted, would you prefer having a .mk for adding the DEFINES needed for preprocessing or should they just be added via configure.in? For example, MOZ_UPDATER needs to be added to the DEFINES currently for the preprocessing. Also, even if you want to have a .mk to add the DEFINES would you be ok with me adding an AC_DEFINE for MOZ_UPDATER to configure.in?
OS: Windows Vista → All
Hardware: x86 → All
General purpose things would be fine in configure.in, MOZ_UPDATER for sure. For other things, we could just stick the DEFINES in package-name.mk.
Attached patch patch in prgress (obsolete) — Splinter Review
Ted, is this what you had in mind?
Attachment #414196 - Flags: review?(ted.mielczarek)
Attached patch patch rev1 (obsolete) — Splinter Review
Assignee: nobody → robert.bugzilla
Attachment #414196 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #414465 - Flags: review?(ted.mielczarek)
Attachment #414196 - Flags: review?(ted.mielczarek)
Attached patch patch rev2 (obsolete) — Splinter Review
some cruft snuck in to the previous patch
Attachment #414465 - Attachment is obsolete: true
Attachment #414467 - Flags: review?(ted.mielczarek)
Attachment #414465 - Flags: review?(ted.mielczarek)
Comment on attachment 414467 [details] [diff] [review]
patch rev2

Still have a tad more that should be done with this so clearing review.
Attachment #414467 - Attachment is obsolete: true
Attachment #414467 - Flags: review?(ted.mielczarek)
Attached patch patch (obsolete) — Splinter Review
Ted, this doesn't allow setting defines from outside of the app's Makefile.in since packager.mk needs the preprocessed package-manifest prior to it being included. It would be extremely simple to just add a new include prior to the preprocessing. Thoughts?
Attachment #415739 - Flags: review?(ted.mielczarek)
Attachment #415739 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 415739 [details] [diff] [review]
patch

Looks good, just a few nits:

>diff --git a/browser/installer/package-manifest.in b/browser/installer/package-manifest.in
>--- a/browser/installer/package-manifest.in
>+++ b/browser/installer/package-manifest.in

I think there's a bunch more in here that can be shifted over to the toolkit manifest. Basically anything that builds outside of browser/ should go there, I think. The entire [xpcom] block, almost everything in components/, probably most of the file. You'll probably have to be careful with the [xpcom] section if you want to preserve it as being in that section (but I'm not sure it actually matters nowadays).

>@@ -2,16 +2,19 @@
> ;
> ; File format:
> ;
> ; [] designates a toplevel component. Example: [xpcom]
> ; - in front of a file specifies it to be removed from the destination

While you're here, can you remove this comment line (and its twin in the new file you created)? I'm pretty sure it doesn't actually work nowadays anyway.

Looks good otherwise, r=me with those changes.
"-" does actually still work, though after all the times bsmedberg has yelled at me for wanting to depend on packaging making something different than dist/bin, I guess I shouldn't argue that this patch, taking us back to having separate manifests not all under the app's control, is just why it existed.

I suspect that moving the (Firefox) [xpcom] block would work out okay, since I think the only point to "top level components" these days is "stick all the XPTs in the one that you want to be the name of your linked .xpt file." It makes me a little nervous since your [xpcom] block is not my [xpcom] block, and won't be until I can build libxul, but mine's a superset of yours, so I expect that it'll work out.

There's (at least) a couple of missing #ifdefs in the patch - MOZ_PLACES for the places components, and the one that doesn't exist for MOZ_TOOLKIT_SEARCH, which is built inside an #ifndef MOZ_THUNDERBIRD #ifndef MOZ_SUITE instead of having the define it should have.

nsSafebrowsingApplication.js isn't a toolkit file at all.

I'm not entirely convinced that crashreporter-override.ini, the Firefox overrides for toolkit's crashreporter.ini, actually belongs in the toolkit manifest. 

On the bright side, I'm almost done moving Tbird to a single package-manifest.in in bug 533043, so you won't have to muck around with us, much.
As usual, Phil is much more thorough than I am.

Phil: if we move things like [xpcom] to a toolkit file, we can certainly ifdef MOZ_ENABLE_LIBXUL to accomodate both configurations.
Depends on: 547653
Carrying forward r+ for the configure.in changes for MOZ_UPDATER

The remainder will need to be reworked per comment #11
Attachment #415739 - Attachment is obsolete: true
Attachment #449968 - Flags: review+
Need this for Bug 535520 so I pushed to mozilla-central the configure.in changes that were already reviewed

http://hg.mozilla.org/mozilla-central/rev/7533fc10b5df
Attachment #449968 - Attachment is obsolete: true
Attachment #450237 - Flags: review+
Note: This is just so the patch applies "as is" to trunk, but not updated so it actually makes any sense on trunk, needs further work for that, which I'll be working on for the next patch...
Drive by comment, I think we should change the package manifest makefile incantations to accept multiple files rather than include the file like that.  This would match the removed-files.in behavior.
Comment on attachment 461010 [details] [diff] [review]
Re-split of the package-manifest, by doing a diff against thunderbird

:khuey it is only the package-manifest.in that have been updated.  I'll work on the removed-files.in while you have a look at the package-manifest...
Attachment #461010 - Flags: feedback?(me)
Attachment #461010 - Attachment is obsolete: true
Attachment #461010 - Flags: feedback?(me)
Attachment #461231 - Flags: review?(me)
Comment on attachment 461231 [details] [diff] [review]
Now with the removed-files split too

>diff --git a/browser/installer/package-manifest.in b/browser/installer/package-manifest.in
>+; toolkit files should be added to the toolkit's package-manifest which is
>+; included at the end of this file.
>+; 

Lets do the same thing we're doing for MOZ_PKG_REMOVALS and include toolkit's package-manifest there.  That probably requires modifying the makefile glue to cat its arguments into the preprocessor like MOZ_PKG_REMOVALS does.

> [browser]
...
>-@BINPATH@/application.ini
>-@BINPATH@/platform.ini
These should stay
...
>-@BINPATH@/README.txt
Stay

>-@BINPATH@/components/components.manifest
Stay.

>-@BINPATH@/components/inspector.xpt
This one should move to toolkit, but should probably also be #ifndef MOZ_NO_INSPECTOR_APIS

> #ifndef WINCE
> @BINPATH@/components/migration.xpt
> #endif
Can we remove the WINCE ifdefs entirely?

>-@BINPATH@/components/shellservice.xpt
Stay.

> #ifdef MOZ_USE_NATIVE_UCONV
> @BINPATH@/components/ucnative.xpt
> #endif
Go.

>-@BINPATH@/components/BrowserFeeds.manifest
Stay.

>-@BINPATH@/components/fuelApplication.manifest
Stay.

>-@BINPATH@/components/BrowserComponents.manifest
Stay.

>-@BINPATH@/components/nsSetDefaultBrowser.manifest
>-@BINPATH@/components/nsSetDefaultBrowser.js
Stay.

>-@BINPATH@/components/BrowserPlaces.manifest
>-@BINPATH@/components/nsPrivateBrowsingService.manifest
>-@BINPATH@/components/nsPrivateBrowsingService.js
>-@BINPATH@/components/toolkitsearch.manifest
Stay.

>-@BINPATH@/components/nsSidebar.manifest
> @BINPATH@/components/nsSidebar.js
Stay.

>-@BINPATH@/components/nsSessionStore.manifest
Stay.

>-#ifndef XP_OS2
>-@BINPATH@/components/@DLL_PREFIX@browsercomps@DLL_SUFFIX@
>-#else
>-@BINPATH@/components/brwsrcmp@DLL_SUFFIX@
>-#endif
Stay.

>-@BINPATH@/components/toolkitplaces.manifest
>-@BINPATH@/components/nsLivemarkService.js
>-@BINPATH@/components/nsTaggingService.js
>-@BINPATH@/components/nsPlacesDBFlush.js
>-@BINPATH@/components/nsPlacesAutoComplete.manifest
>-@BINPATH@/components/nsPlacesAutoComplete.js
>-@BINPATH@/components/nsPlacesExpiration.js
These might need to be #ifdef MOZ_PLACES once in toolkit.

>-; Modules
>-@BINPATH@/modules/*
Stay.

> ; [Browser Chrome Files]
>-@BINPATH@/chrome/browser.jar
>-@BINPATH@/chrome/browser.manifest
>-@BINPATH@/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/install.rdf
>-@BINPATH@/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/icon.png
>-@BINPATH@/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/preview.png
Definitely stay.

>-#ifdef XP_UNIX
>-#ifndef XP_MACOSX
>-@BINPATH@/chrome/icons/default/default16.png
>-@BINPATH@/chrome/icons/default/default32.png
>-@BINPATH@/chrome/icons/default/default48.png
>-#endif
>-#endif
>-
Probably stay.  I guess this is something you only get with --enable-official-branding because I don't have it in my objdir.

> ; shell icons
> #ifdef XP_UNIX
> #ifndef XP_MACOSX
> @BINPATH@/icons/*.xpm
>-@BINPATH@/icons/*.png
> #endif
> #endif
Stay.

> #ifdef XP_MACOSX
> @BINPATH@/res/cursors/*
> #endif
Go.

> ; [ActiveX]
> #ifdef WINCE
> #ifndef MOZ_NO_ACTIVEX_SUPPORT
>-@BINPATH@/components/nsAxSecurityPolicy.js
> @BINPATH@/@PREF_DIR@/activex.js
> @BINPATH@/plugins/npmozax.dll
> @BINPATH@/plugins/nsIMozAxPlugin.xpt
> #endif
> #endif
Can we just axe this completely?

>-; [FastStart]
>-#ifdef WINCE
>-#ifdef MOZ_FASTSTART
>-@BINPATH@/firefoxfaststart.exe
>-@BINPATH@/components/FastStartup.manifest
>-@BINPATH@/components/FastStartup.js
>-#endif
>-#endif
Ditto.

>+; toolkit files
>+#include ../../toolkit/mozapps/installer/package-manifest.in
As I said before, we should do this like removed-files.in

>diff --git a/toolkit/xre/Makefile.in b/toolkit/xre/Makefile.in
>-DEFINES += -DMOZ_UPDATER
Necessary?

I didn't look closely at removed-files.in, or the toolkit files.  The list of removed files is going to be really tough to get right and I'm not convinced it's worth splitting the existing files.

r-, but coming along nicely.  rs should probably have a look at this next time too.
Attachment #461231 - Flags: review?(me) → review-
(In reply to comment #21)
>... 
> >diff --git a/toolkit/xre/Makefile.in b/toolkit/xre/Makefile.in
> >-DEFINES += -DMOZ_UPDATER
> Necessary?
Not necessary but it shouldn't be needed now that it has been added to configure.in
(In reply to comment #21)
> > ; [ActiveX]
> > #ifdef WINCE
> > #ifndef MOZ_NO_ACTIVEX_SUPPORT
> >-@BINPATH@/components/nsAxSecurityPolicy.js
> > @BINPATH@/@PREF_DIR@/activex.js
> > @BINPATH@/plugins/npmozax.dll
> > @BINPATH@/plugins/nsIMozAxPlugin.xpt
> > #endif
> > #endif
> Can we just axe this completely?

Dunno? - Who should I ask?

> 
> >-; [FastStart]
> >-#ifdef WINCE
> >-#ifdef MOZ_FASTSTART
> >-@BINPATH@/firefoxfaststart.exe
> >-@BINPATH@/components/FastStartup.manifest
> >-@BINPATH@/components/FastStartup.js
> >-#endif
> >-#endif
> Ditto.

Same question...

> I didn't look closely at removed-files.in, or the toolkit files.  The list of
> removed files is going to be really tough to get right and I'm not convinced
> it's worth splitting the existing files.

What is the purpose of that file anyway?
(In reply to comment #23)
>...
> > I didn't look closely at removed-files.in, or the toolkit files.  The list of
> > removed files is going to be really tough to get right and I'm not convinced
> > it's worth splitting the existing files.
> 
> What is the purpose of that file anyway?
These are the files to remove during an update
(In reply to comment #24)

> > What is the purpose of that file anyway?
> These are the files to remove during an update

That was what I feared...  Isn't it possible to create that list when we do the update, if we keep a list of installed files?

As it is now, the removed-files.in list has #defines in it, meaning that if any of these are changed between the versions the user installs, then the list will not be correct anyway - or is it more clever than that?

Secondly, wont this list just grow forever, usually not considered a good design option...

Can I assume there already is a bug somewhere to improve on this?
(In reply to comment #25)
> (In reply to comment #24)
> 
> > > What is the purpose of that file anyway?
> > These are the files to remove during an update
> 
> That was what I feared... 
That's what it says at the top of the file as well. :)

> Isn't it possible to create that list when we do the
> update, if we keep a list of installed files?
How? Updates can be from any previous version. I suppose someone could come up with a clever way of tracking the files and pushing the list to hg but it would likely be prone to errors.

> As it is now, the removed-files.in list has #defines in it, meaning that if any
> of these are changed between the versions the user installs, then the list will
> not be correct anyway - or is it more clever than that?
Not sure what you mean by this since the defines are compile time and do the right thing for the client when they change (see MOZ_MEMORY for example). The installer uses the uninstall log to remove the previous installation so isn't a problem.

> Secondly, wont this list just grow forever, usually not considered a good
> design option...
It will grow as long as it is never maintained but as can be seen the main growth has been things like searchplugins where we want them to be what we have promised to deliver for a release. I'm not saying it shouldn't have the files that are no longer applicable removed by any means and have thought about doing so when time permits.

> Can I assume there already is a bug somewhere to improve on this?
Nope

As far as this bug goes, the only files that should be of concern are toolkit and possibly core files. For example:

components/nsUpdateService.js
components/nsUpdateServiceStub.js
components/nsExtensionManager.js

#ifdef XP_MACOSX
updater.app/Contents/MacOS/updater.ini
#endif

#ifdef XP_WIN
components/nsPostUpdateWin.js
#endif

etc.

Seems like the inverse of the package-manifest implementation should be all that is needed and that it shouldn't be any more difficult than that implementation.
(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #24)
> > 
> > > > What is the purpose of that file anyway?
> > > These are the files to remove during an update
> > 
> > That was what I feared... 
> That's what it says at the top of the file as well. :)
*GG* My mistake

But before I answer the rest of the comments, I am getting confused, assume, I have installed version A of the software, and I want to upgrade to version B of the software.  Is it then the files in A->removed-files.in or B->removed-files.in that are deleted if they exist?

I assume its B->removed-files.in - but I'll get back to why this confuses me...
It is B->removed-files.in that is used and added to the B->update mar file.
(In reply to comment #28)
> It is B->removed-files.in that is used and added to the B->update mar file.

I assume that the b->update mar file is the file that is actually put in the package.

(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #24)
> 
> > Isn't it possible to create that list when we do the
> > update, if we keep a list of installed files?
> How? Updates can be from any previous version. I suppose someone could come up
> with a clever way of tracking the files and pushing the list to hg but it would
> likely be prone to errors.

I was considering storing the list of installed files on the users computer, and diff'ing this against the list of files in a given release, to produce the list of files that should be removed.  But ofcause this would lead to problems if the user somehow gets this file deleted...

> > As it is now, the removed-files.in list has #defines in it, meaning that if any
> > of these are changed between the versions the user installs, then the list will
> > not be correct anyway - or is it more clever than that?
> Not sure what you mean by this since the defines are compile time and do the
> right thing for the client when they change (see MOZ_MEMORY for example). The
> installer uses the uninstall log to remove the previous installation so isn't a
> problem.

This suddenly makes sense :)  But what a nightmare that must be to maintain correctly, every time we add a file in a #define in the package-manifest the removed-files.in must also be updated with the inverse #define and when we remove a file, well.. that is the easy case :)

btw - I see a large section in the package-manifest.in protected by "#if MOZ_UPDATE_CHANNEL == beta" why shouldn't there be a corresponding section with "#if MOZ_UPDATE_CHANNEL != beta" in the removed-files.in file? 

> > Secondly, wont this list just grow forever, usually not considered a good
> > design option...
> It will grow as long as it is never maintained but as can be seen the main
> growth has been things like searchplugins where we want them to be what we have
> promised to deliver for a release. I'm not saying it shouldn't have the files
> that are no longer applicable removed by any means and have thought about doing
> so when time permits.

How can maintenance stop it growing?  And how can files be no longer applicable for being in the removed-files.in ? (unless they are added to the release again)

----

Thank you for taking the time to explain this to me, I appreciate it!
(In reply to comment #23)
> (In reply to comment #21)
> > > ; [ActiveX]
> > > #ifdef WINCE
> > > #ifndef MOZ_NO_ACTIVEX_SUPPORT
> > >-@BINPATH@/components/nsAxSecurityPolicy.js
> > > @BINPATH@/@PREF_DIR@/activex.js
> > > @BINPATH@/plugins/npmozax.dll
> > > @BINPATH@/plugins/nsIMozAxPlugin.xpt
> > > #endif
> > > #endif
> > Can we just axe this completely?
> 
> Dunno? - Who should I ask?
> 
> > 
> > >-; [FastStart]
> > >-#ifdef WINCE
> > >-#ifdef MOZ_FASTSTART
> > >-@BINPATH@/firefoxfaststart.exe
> > >-@BINPATH@/components/FastStartup.manifest
> > >-@BINPATH@/components/FastStartup.js
> > >-#endif
> > >-#endif
> > Ditto.
> 
> Same question...

Blassey?
(In reply to comment #29)
> (In reply to comment #28)
> > It is B->removed-files.in that is used and added to the B->update mar file.
> 
> I assume that the b->update mar file is the file that is actually put in the
> package.
A mar file is just the file extension for the file that contains all of the patches for the files to be patched for a partial mar or all of the files for a complete mar.

> (In reply to comment #26)
>...
> and diff'ing this against the list of files in a given release, to produce the
> list of files that should be removed.  But ofcause this would lead to problems
> if the user somehow gets this file deleted...
We'd have to keep a list of files installed and the mar would have to have a list of the files for that build. We'd also have to have a list of one-off's like the searchplugins.

> This suddenly makes sense :)  But what a nightmare that must be to maintain
> correctly, every time we add a file in a #define in the package-manifest the
> removed-files.in must also be updated with the inverse #define and when we
> remove a file, well.. that is the easy case :)
Actually, when a file is removed it needs to be added to remove-files.in is the easy case. Files added to package-manifest with a #define for the OS don't need to be added to removed-files.in. Also, if the file is listed in removed-files.in and it isn't on the user system it is just a noop on the client during update.

> btw - I see a large section in the package-manifest.in protected by "#if
> MOZ_UPDATE_CHANNEL == beta" why shouldn't there be a corresponding section with
> "#if MOZ_UPDATE_CHANNEL != beta" in the removed-files.in file? 
Because the beta channel only updates to the beta channel so it always has those files.

btw: that large section should be a one liner surrounded by an ifdef now.

>...
> How can maintenance stop it growing?  And how can files be no longer applicable
> for being in the removed-files.in ? (unless they are added to the release
> again)
We currently don't supply updates to old builds to the latest builds so the files removed to update from 2.0 to 3.0 or 3.5 shouldn't need to stay in the removed-files.in. I would need to verify this would continue to be the policy before removing the entries.

> Thank you for taking the time to explain this to me, I appreciate it!
You're welcome.
(In reply to comment #31)
> (In reply to comment #29)
> > (In reply to comment #28)
> >...
> > How can maintenance stop it growing?  And how can files be no longer applicable
> > for being in the removed-files.in ? (unless they are added to the release
> > again)
> We currently don't supply updates to old builds to the latest builds so the
> files removed to update from 2.0 to 3.0 or 3.5 shouldn't need to stay in the
> removed-files.in. I would need to verify this would continue to be the policy
> before removing the entries.

Just to be clear, there are many clients of Mozilla (SeaMonkey included) that occassionally have to "skip" a Gecko rev. If removed-files.in will be merged along with the rest of this bug, I do not think we should kill _all_ old update paths on the removed-files. Though that is a different bugs discussion, I just wanted to state it for posterity.
What I was referring to is Firefox's app specific removed-files.in and not the toolkit one which is relatively small in comparison.
(In reply to comment #22)
> (In reply to comment #21)
> >... 
> > >diff --git a/toolkit/xre/Makefile.in b/toolkit/xre/Makefile.in
> > >-DEFINES += -DMOZ_UPDATER
> > Necessary?
> Not necessary but it shouldn't be needed now that it has been added to
> configure.in

Shouldn't it go into a separate patch then?
Attached patch Updated with review comments (obsolete) — Splinter Review
Attachment #461231 - Attachment is obsolete: true
(In reply to comment #34)
>...
> Shouldn't it go into a separate patch then?
Likely easier to just clean it up since it is an extremely simple / safe change in this bug.
I agree its easier to piggy bag it on this one.
Attachment #464073 - Attachment is obsolete: true
Attachment #464377 - Flags: review?(robert.bugzilla)
Comment on attachment 464377 [details] [diff] [review]
Now includes the -DEFINES += -DMOZ_UPDATER again

>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/installer/package-manifest.in
>@@ -0,0 +1,463 @@
>+; Package file for the Firefox build. 

Not really, that's the point.

>+@BINPATH@/components/layout_base.xpt
>+#ifdef NS_PRINTING
>+@BINPATH@/components/layout_printing.xpt
>+#endif
>+@BINPATH@/components/layout_xul_tree.xpt
>+@BINPATH@/components/layout_xul.xpt

Bug 587530 added a layout_forms.xpt (and took away the layout.xpt that you weren't packaging, because Firefox's package-manifest was failing to package it).
Comment on attachment 464377 [details] [diff] [review]
Now includes the -DEFINES += -DMOZ_UPDATER again

Bug 556644 will make it so removed-files won't need to be updated all that often as soon as omnijar is turned on by default so I don't feel all that strongly about having a toolkit removed-files.in. Since it is quite likely that it will be turned on by default after Firefox beta 4 it would probably be best to just support a toolkit package manifest. Sorry about this but I didn't know about the omnijar changes related to removed-files until recently. My Firefox 4 work will make it difficult for me to find time to review this for at least another week since this isn't blocking or wanted so if you'd like to get it reviewed sooner perhaps Ted has some time.
(In reply to comment #39)
> Comment on attachment 464377 [details] [diff] [review]
> Now includes the -DEFINES += -DMOZ_UPDATER again
> 
> Bug 556644 will make it so removed-files won't need to be updated all that
> often as soon as omnijar is turned on by default so I don't feel all that
> strongly about having a toolkit removed-files.in. Since it is quite likely that
> it will be turned on by default after Firefox beta 4 it would probably be best
> to just support a toolkit package manifest. Sorry about this but I didn't know
> about the omnijar changes related to removed-files until recently. My Firefox 4
> work will make it difficult for me to find time to review this for at least
> another week since this isn't blocking or wanted so if you'd like to get it
> reviewed sooner perhaps Ted has some time.

NP. I'll remove the split of the removed-files files from the patch.

I don't think it will be a problem to delay the review another week or so (btw. thank you for letting me know about it) - As the package files seems to be updating regularly, I'm spending significant time trying to keep the patch from bit rotting - how about you ping me, like a workday before you will have time to review it, I'll then post an updated version that applies to trunk?
Should probably hold off on updating/reviewing this thing until Bug 575894 is done.  It's going to make XPTs and manifests disappear from the packaging manifest (except for the ones we actually ship).
Depends on: 575894
Comment on attachment 464377 [details] [diff] [review]
Now includes the -DEFINES += -DMOZ_UPDATER again

Removing review due to holding off on Bug 575894 per khuey's request. I wonder if this shouldn't happen until after we branch anyway
Attachment #464377 - Flags: review?(robert.bugzilla)
Yeah, good idea to remove the flag while we wait - sorry I didn't think about that...
Requesting blocking to get a decision on whether we want this fixed before we fix bug 575751 .
blocking2.0: --- → ?
Er, for fennec of course..
blocking2.0: ? → ---
tracking-fennec: --- → ?
(In reply to comment #45)
> Requesting blocking to get a decision on whether we want this fixed before we
> fix bug 575751 .

I think we should block on this so fennec does not need to play "catch up" when components are updated in Firefox.
tracking-fennec: ? → 2.0+
tracking-fennec: 2.0+ → 2.0b2+
Fennec is adding it's own full manifest in bug 575751, but this bug would be really nice for maintenance.
tracking-fennec: 2.0b2+ → 2.0b3+
Attachment #464377 - Flags: review?(robert.bugzilla)
Comment on attachment 464377 [details] [diff] [review]
Now includes the -DEFINES += -DMOZ_UPDATER again

Please update the patch so it applies.
Attachment #464377 - Flags: review?(robert.bugzilla) → review-
tracking-fennec: 2.0b3+ → 2.0-
Attached patch Updated (obsolete) — Splinter Review
Applies on top of: 

changeset:   58210:dde5e4be82c1
tag:         tip
user:        Chris Jones <jones.chris.g@gmail.com>
date:        Wed Nov 24 21:33:37 2010 -0600
summary:     Bug 612573: Make sure shadowable layers aren't destroyed in the middle of transactions. r=karlt a=2.0

---------

There were a lot of additions to package-manifest.in and removed-files.in, I have kept these in the browser version of the files - which is probably not correct...
Attachment #464377 - Attachment is obsolete: true
Attachment #493250 - Flags: feedback?(robert.bugzilla)
Comment on attachment 493250 [details] [diff] [review]
Updated

Took a quick look at the updated patch and you still add a toolkit removed-files even though we decided it shouldn't be necessary per comment #39 and comment #40.

There are also a couple of files listed in the toolkit's package-manifest that obviously don't belong there... for example, firefoxfaststart.exe. There are others that aren't as obvious... for example, nsSafebrowsingApplication.js lives under browser and shouldn't be added to the toolkit's package-manifest.
Attachment #493250 - Flags: feedback?(robert.bugzilla) → feedback-
Fits:

changeset:   58316:5f9204fe5cd5
tag:         tip
user:        Oleg Romashin <romaxa@gmail.com>
date:        Mon Nov 29 09:33:34 2010 +0200
summary:     Bug 614751 - can't compile nsHapticFeedback.cpp on maemo6 r=blassey.bugs a=npodb
----

Too long time since I touched this last, had forgotten about the removed files stuff.

The reason I asked for feedback and not review was that there were a lot of changes in the files, and I still don't know how to judge if a file should be in one or the other? - Any hints are welcome.
Attachment #493250 - Attachment is obsolete: true
Attachment #493662 - Flags: feedback?(robert.bugzilla)
There are probably exceptions where we've screwed up, but the simple first cut is "does the source live in browser/? then it belongs in the Firefox package-manifest, otherwise it belongs in the toolkit one."

Once you get past that, then you can start worrying about the weirder things, like firefoxfaststart.exe, which does sound like it would belong in the Firefox one, but the source lives in toolkit/, so eventually you find that the way it's implemented is that in a place like http://mxr.mozilla.org/mozilla-central/source/browser/app/Makefile.in#300 an app copies faststartstub.exe to %APPNAME%faststart.exe, and so it should be packaged like SeaMonkey does, http://mxr.mozilla.org/comm-central/source/suite/installer/package-manifest.in#786, and it really should be in the toolkit package-manifest.
(In reply to comment #53)
>...
> Once you get past that, then you can start worrying about the weirder things,
> like firefoxfaststart.exe, which does sound like it would belong in the Firefox
> one, but the source lives in toolkit/, so eventually you find that the way it's
> implemented is that in a place like
> http://mxr.mozilla.org/mozilla-central/source/browser/app/Makefile.in#300 an
> app copies faststartstub.exe to %APPNAME%faststart.exe, and so it should be
> packaged like SeaMonkey does,
> http://mxr.mozilla.org/comm-central/source/suite/installer/package-manifest.in#786,
> and it really should be in the toolkit package-manifest.
I don't think that is the right approach for this scenario though I suspect I could easily be convinced otherwise. Whether files like this exist is optional and is decided at the app level. It also could be a bummer for an app to not be able to rename it something other than what is chosen in the toolkit manifest. Also of interest
http://mxr.mozilla.org/mobile-browser/source/app/Makefile.in#197
Yeah, SeaMonkey is wrong, too, and should be using $(MOZ_APP_NAME)faststart$(BIN_SUFFIX) (or maybe toolkit should be compiling it as that), but the lack of any results in http://mxr.mozilla.org/comm-central/search?find=%2Fmail%2F&string=faststart sort of betrays my lack of strong interest.
Comment on attachment 493662 [details] [diff] [review]
This time without the removed-files split

> @BINPATH@/components/directory.xpt
> @BINPATH@/components/dom_notification.xpt

Should be toolkit.

> @BINPATH@/components/jetpack.xpt

Should be toolkit, in an ifdef MOZ_IPC block

> @BINPATH@/components/layout_forms.xpt
> @BINPATH@/components/necko_wyciwyg.xpt
> @BINPATH@/components/ConsoleAPI.manifest
> @BINPATH@/components/ConsoleAPI.js

Should be toolkit

> @BINPATH@/components/toolkitsearch.manifest
> @BINPATH@/components/nsSearchService.js
> @BINPATH@/components/nsSearchSuggestions.js

Should be toolkit, in an ifdef MOZ_TOOLKIT_SEARCH - that was comment 11 and bug 547653

> @BINPATH@/components/PlacesCategoriesStarter.js

That's a bit of toolkit ifdef MOZ_PLACES

> @BINPATH@/components/nsInputListAutoComplete.js
> ; Modules
> @BINPATH@/modules/*

Toolkit

> ; Safe Browsing
> @BINPATH@/components/nsSafebrowsingApplication.manifest
> @BINPATH@/components/nsSafebrowsingApplication.js
> @BINPATH@/components/nsURLClassifier.manifest
> @BINPATH@/components/nsUrlClassifierListManager.js
> @BINPATH@/components/nsUrlClassifierLib.js
> @BINPATH@/components/url-classifier.xpt

In keeping with our general policy of being confusing, the first two are browser, the last four are toolkit, ifdef MOZ_URL_CLASSIFIER. And they seem to have succeeded in being confusing, since you've got all six in both the browser/ and toolkit/ files.

>+; Style Sheets, Graphics and other Resources used by the layout engine.
> @BINPATH@/res/html/*

That's just as core as all the rest, -> toolkit

>+++ b/toolkit/mozapps/installer/package-manifest.in
...
>+; [] designates a toplevel component. Example: [xpcom]
...
>+[browser]
...
>+; Type libraries

This is a bit ugly, because "toplevel component" has just one meaning: it gives its name to all the .xpts within it when you are running xptlink. Right now, all the .xpts in Firefox's package-manifest are in the [browser] chunk, so we get a single file named browser.xpt with the contents of all the loose .xpts. I'd sort of hope (without knowing for sure) that the result of this patch, with two separate [browser] hunks, would be the same thing, one file with all the bits from both, though it might be just the stuff from one hunk. I'm pretty sure the result for Thunderbird would be two files, mail.xpt with our bits, and the bizarrely-named browser.xpt with the core/toolkit bits.

Various unpalatable solutions to that occur to me:

* wait for bug 575894 to land (either by landing after it, or by not moving any .xpts), at which point you won't have to deal with xpts at all

* do what the first patch here did, just put toolkit .xpts at the start without a toplevel component, and ensure that every app's package-manifest leaves open the toplevel component that it wants to use as the linked xpt filename

* screw over Firefox by using a [core] component so it ships a browser.xpt and a core.xpt, and find out whether or not that makes a measurable perf difference

One thing to keep in mind while choosing a solution is that while Firefox doesn't have to care what changes you make to its linked xpt file, since it's shipping it in omni.jar, apps that aren't need to worry about adding to removed-files.in, so a core change that gives Thunderbird a foo.xpt followed by another core change that takes it away will leave it in a bad state if nobody remembers that they need to remove the stale one.
I have only updated it with the first part until the comment says "This is a bit ugly, ..."
Attachment #493662 - Attachment is obsolete: true
Attachment #493662 - Flags: feedback?(robert.bugzilla)
tracking-fennec: 2.0- → ---
It is important to note that all of the block dependencies Serge has added aren't actually blocked by this bugs. At best this bug makes it easier in that common files are maintained for all applications though it is more difficult to decide which entries should be in the common removed-files. Also, we won't add support for removed-files in this bug per comment #39 which makes it so we shouldn't have to add entries to removed-files when upgrading from Firefox 5 and above. When we require MSVC 2010 we will require users to upgrade to Firefox 10 (possibly 9) so they pick up bug 668436 at which time we can stop adding entries to removed-files for the vast majority of cases and remove the old entries.

Adjusting summary accordingly.

Serge, can you clean up the block dependency list you just added? Thanks
Summary: Having to create patches for each app's package manifest and removed-files and then get reviews from each app is a PITA → Having to create patches for each app's package manifest and then get reviews from each app is a PITA
I'm not going to have time to work on this so resetting to default assignee.
Assignee: robert.bugzilla → nobody
No longer blocks: 657208
No longer blocks: 595759
No longer blocks: 694353
No longer blocks: 660727
No longer blocks: 694371
What needs doing here to finish this off, is it just updating the patch to what is current?
Depends on: new-packager
Depends on: 825872
No assignee, updating the status.
Status: ASSIGNED → NEW
No assignee, updating the status.
Component: Build Config → General
Product: Toolkit → Firefox Build System
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.