Closed Bug 445143 Opened 16 years ago Closed 16 years ago

clean mozilla-central from unneeded references to suite, mail, etc.

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.1a2

People

(Reporter: kairo, Assigned: Swatinem)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 444686 creates an own build system for SeaMonkey, Thunderbird and later Sunbird in its own comm-central repository, but mozilla-central still contains references to those products even if they can't actually be built from it any more due to their source not living in mozilla-central.
We should clean up those references.
Attached patch some cleanup (obsolete) — Splinter Review
I have cleaned up some references I found when looking over configure.in and other files. I hope this is all correct, I'm new to hacking on configures/makefiles.

mxr shows there are quite a few ifdefs inside other makefiles and sources throughout the tree for MOZ_MAIL_NEWS and friends. I haven't touched those. Should those be removed entirely or are they needed after all?
Firstly, thanks for taking this on.

Secondly, I don't know what the standalone composer is doing, so I don't know that we should touch it at this stage. I think that should really be done in a separate bug anyway (as this is just about tidy up after moving SeaMonkey/Thunderbird/Sunbird to comm-central).

The same applies to camino - a separate bug please.

Thirdly, when it gets to review time, please can we get reviews from a normal mozilla-central and a comm-central build config folks? I'd just like to confirm from both sides that we don't need the unncessary code.

The client.mk comment is now actually obsolete - MOZ_CO_PROJECT doesn't work any more, it was only relevant to cvs.

(In reply to comment #1)
> Created an attachment (id=332932) [details]
> some cleanup
> 
> I have cleaned up some references I found when looking over configure.in and
> other files. I hope this is all correct, I'm new to hacking on
> configures/makefiles.

You could also remove the LDAP enable/disable configure/makefile items - though not from extensions/pref yet please.

> mxr shows there are quite a few ifdefs inside other makefiles and sources
> throughout the tree for MOZ_MAIL_NEWS and friends. I haven't touched those.
> Should those be removed entirely or are they needed after all?

Apart from the ones in Makefiles/configures, the only ones I see are in extensions/cookie. These are still required and we need to verify that they are still active within SeaMonkey (and probably Thunderbird) after your proposed removals.

At some stage we'll need to get a better solution for extensions/cookie, but that will definitely be in a different bug (I'll raise one later).
Thanks for your comments.
(In reply to comment #2)
> Firstly, thanks for taking this on.
> 
> Secondly, I don't know what the standalone composer is doing, so I don't know
> that we should touch it at this stage. I think that should really be done in a
> separate bug anyway (as this is just about tidy up after moving
> SeaMonkey/Thunderbird/Sunbird to comm-central).
> 
> The same applies to camino - a separate bug please.

I opened bug 449777 for that.

> The client.mk comment is now actually obsolete - MOZ_CO_PROJECT doesn't work
> any more, it was only relevant to cvs.

I will take another look at client.mk, but understanding that code is quite hard.

> You could also remove the LDAP enable/disable configure/makefile items - though
> not from extensions/pref yet please.

I will take a look at this, I also found some more references to MOZ_STATIC_MAIL_BUILD that will be removed in another patch.
Are there any other places I need to look that I missed the first time?
New patch coming tomorrow.
(In reply to comment #3)
> > The client.mk comment is now actually obsolete - MOZ_CO_PROJECT doesn't work
> > any more, it was only relevant to cvs.
> 
> I will take another look at client.mk, but understanding that code is quite
> hard.

Sorry, I should have been clearer:

 #    1. hg clone ssh://hg.mozilla.org/mozilla-central mozilla
 #    2. cd mozilla
 #    3. create your .mozconfig file with
-#       mk_add_options MOZ_CO_PROJECT=
-#         suite,browser
+#       mk_add_options MOZ_CO_PROJECT=browser

I'd just change that last line to:

+#       ac_add_options --enable-application=browser

(and add a note about xulrunner)

> > You could also remove the LDAP enable/disable configure/makefile items - though
> > not from extensions/pref yet please.
> 
> I will take a look at this, I also found some more references to
> MOZ_STATIC_MAIL_BUILD that will be removed in another patch.
> Are there any other places I need to look that I missed the first time?
> New patch coming tomorrow.
> 
I think any reference to MOZ_STATIC_MAIL_BUILD in mozilla-central can be removed.

There are MOZ_THUNDERBIRD and MOZ_SUITE ifdefs, but they can't be fully removed yet (except from the bits you've already done).

As far as I can recall there's no other specific ifdefs, but I'll have to have a look around sometime and see if I can remember any others.
Comment on attachment 332932 [details] [diff] [review]
some cleanup

>-MOZ_SUNBIRD	= @MOZ_SUNBIRD@

removing this you'll [probably] need to add an app-config.mk as in http://mxr.mozilla.org/comm-central/source/suite/app-config.mk and http://mxr.mozilla.org/comm-central/source/mail/app-config.mk but be sure it gets checked into calendar CVS (as calendar is not on comm-central)

>@@ -4928,17 +4856,8 @@
>     MOZ_APP_DISPLAYNAME=Firefox
>     ;;
> 
>-calendar)
>-mail)
> *)]
>-    AC_MSG_ERROR([Official branding is only available for Firefox Sunbird and Thunderbird.])
>+    AC_MSG_ERROR([Official branding is only available for Firefox.])

You probably don't want to remove these from here (at least not with an ERROR). My gut is you can probably turn that into an warning.

diff -r 73c191bedd9a toolkit/toolkit-makefiles.sh

If you remove the calendar lines from here, you should probably get them into calendar/ as well in makefiles.sh. See: http://mxr.mozilla.org/comm-central/source/suite/makefiles.sh

These comments in addition to the ones Mark already mentioned.
(In reply to comment #5)
> (From update of attachment 332932 [details] [diff] [review])
> >-MOZ_SUNBIRD	= @MOZ_SUNBIRD@
> 
> removing this you'll [probably] need to add an app-config.mk as in
> http://mxr.mozilla.org/comm-central/source/suite/app-config.mk and
> http://mxr.mozilla.org/comm-central/source/mail/app-config.mk but be sure it
> gets checked into calendar CVS (as calendar is not on comm-central)

Actually we don't need to do this until calendar (Sunbird in this case) moves to comm-central. Yes we will need it then, but it won't affect them at the moment as we don't support building Sunbird from comm-central currently.

> >@@ -4928,17 +4856,8 @@
> >     MOZ_APP_DISPLAYNAME=Firefox
> >     ;;
> > 
> >-calendar)
> >-mail)
> > *)]
> >-    AC_MSG_ERROR([Official branding is only available for Firefox Sunbird and Thunderbird.])
> >+    AC_MSG_ERROR([Official branding is only available for Firefox.])
> 
> You probably don't want to remove these from here (at least not with an ERROR).
> My gut is you can probably turn that into an warning.

Actually I think we do want this change as-is. In comm-central's configure.in we explicit pass --disable-official-branding which overrides what the user passes to comm-central's configure.in.

Obviously this needs double checking with a Thunderbird build with --enable-official-branding present.

> diff -r 73c191bedd9a toolkit/toolkit-makefiles.sh
> 
> If you remove the calendar lines from here, you should probably get them into
> calendar/ as well in makefiles.sh. See:
> http://mxr.mozilla.org/comm-central/source/suite/makefiles.sh

These are already in calendar's shared_makefiles.sh so no need to do this - see

http://mxr.mozilla.org/seamonkey/source/calendar/makefiles.sh
http://mxr.mozilla.org/seamonkey/source/calendar/shared_makefiles.sh
IMHO, the first patch was OK but only resolves a smaller part of what I had in mind. Removing other things that go nice and easy with this would have been something I also would have looked into if I hadn't been on the Summit and now on vacation instead. :)

This looks like a good first step, but did you try if SeaMonkey and Thunderbird builds stay completely unchanged as a result of this patch?
Attached patch [checked in] v2Splinter Review
This patch also removes the MOZ_STATIC_MAIL_BUILD and LDAP stuff.
I've successfully built and run Thunderbird with --enable-official-branding with this patch. I haven't tried with SeaMonkey however.
Attachment #332932 - Attachment is obsolete: true
Attachment #333080 - Flags: review?(bugzilla)
Comment on attachment 333080 [details] [diff] [review]
[checked in] v2

I've been building this with various TB/SM mixtures today and not had any apparent problems. I've also verified that MOZ_MAIL_NEWS is correctly getting set when it comes to extensions/cookie so that seems fine as well.

Please get Ted to review this as well as he's the official mozilla-central build config person.
Attachment #333080 - Flags: review?(bugzilla) → review+
(In reply to comment #7)
> IMHO, the first patch was OK but only resolves a smaller part of what I had in
> mind. Removing other things that go nice and easy with this would have been
> something I also would have looked into if I hadn't been on the Summit and now
> on vacation instead. :)

Robert, what else did you have in mind?
Attachment #333080 - Flags: review?(ted.mielczarek)
Please also patch the block starting in http://mxr.mozilla.org/comm-central/source/configure.in#7862 to not --disable-calendar on the Mozilla configure as we don't need this once your patch is in. You only need to kill the comment about it and remove the option from the ac_configure_args there.

(In reply to comment #10)
> Robert, what else did you have in mind?

I'll look into that in the next days, not sure what's still left to do.
Cleanup as per comment 11.
Assignee: nobody → arpad.borsos
Status: NEW → ASSIGNED
Attachment #334080 - Flags: review?
Attachment #334080 - Flags: review? → review?(kairo)
Comment on attachment 334080 [details] [diff] [review]
[checked in] comm-central cleanup

Yes, that's exactly what should be done here :)
Attachment #334080 - Flags: review?(kairo) → review+
Comment on attachment 333080 [details] [diff] [review]
[checked in] v2

 [  --enable-application=APP
                           Options include:
-                            suite
                             browser (Firefox)
-                            mail (Thunderbird)
                             composer
-                            calendar (Sunbird)
                             xulrunner
                             camino

Might as well remove camino here too.
Attachment #333080 - Flags: review?(ted.mielczarek) → review+
(In reply to comment #15)
> Might as well remove camino here too.

Thats what I've done in bug 449777. Thanks for the review. checkin-needed
Keywords: checkin-needed
Comment on attachment 333080 [details] [diff] [review]
[checked in] v2

Pushed as 17103:c10e93ebe6bb.
Target Milestone: --- → mozilla1.9.1a2
Comment on attachment 334080 [details] [diff] [review]
[checked in] comm-central cleanup

Sorry, I only just saw this needed checkin into comm-central.

Checked in: changeset id 231:3d34c59428af
Attachment #334080 - Attachment description: comm-central cleanup → [checked in] comm-central cleanup
Comment on attachment 333080 [details] [diff] [review]
[checked in] v2

This has already been checked in per comment 17.
Attachment #333080 - Attachment description: v2 → [checked in] v2
(In reply to comment #10)
> (In reply to comment #7)
> > IMHO, the first patch was OK but only resolves a smaller part of what I had in
> > mind. Removing other things that go nice and easy with this would have been
> > something I also would have looked into if I hadn't been on the Summit and now
> > on vacation instead. :)
> 
> Robert, what else did you have in mind?

Would be good to know. Otherwise this bug is FIXED.
Let's mark it FIXED then and when other things come up, we'll file new bugs :)
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.