440932 - toolkit dlmgr should be buildable by suite

Status: RESOLVED FIXED

(Toolkit Graveyard :: Build Config, defect)

Description

[Justin Wood (:Callek)]

Attachment: patch to mozilla-central

+++ This bug was initially created as a clone of Bug #381157 +++

Bug 381157 requires some toolkit/ changes to enable the DLMGR.

This bug will track those changes, adding a new define/makefile variable that Suite's build process can use.

Due to current 1.9.0 rules this needs to apply and be pushed to mozilla-central first.

r? sdwilsh for toolkit; sr? from neil for xpfe/

(If anyone knows a cleaner way to do these makefile ifdef's please tell me)

Comment 1

[Justin Wood (:Callek)]

whops, correcting summary and component...

Comment 2

[[e-mail]]

Is anyone else still using the xpfe download manager?

Comment 3

[Justin Wood (:Callek)]

re: c#2, not that I could tell.

Comment 4

[[e-mail]]

In that case, might it be easier to have a variable SUITE_USING_XPFE_DM which is turned on by default for suite so you can replace the toolkit ifndef MOZ_SUITE with ifndef SUITE_USING_XPFE_DM rather than the triple ifdef in your patch?

Comment 5

[[e-mail]]

Comment on attachment 326074 [details] [diff] [review]
patch to mozilla-central

> ifneq ($(MOZ_BUILD_APP),camino)
>+ifdef MOZ_SUITE
>+ifdef SUITE_USING_TOOLKIT_DM
> DIRS += download-manager
>+endif 
>+else
>+DIRS += download-manager
>+endif 
> endif
I don't think the ifdef MOZ_SUITE is necessary as nobody else should be running this Makefile except for camino which is already excluded. I also think your ifdef SUITE_USING_TOOLKIT_DM should be an ifndef.

>+#if !defined(SUITE_USING_TOOLKIT_DM)
I'd prefer #ifndef

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 326074 [details] [diff] [review]
patch to mozilla-central

Address Neil's comments first, thanks.

Comment 7

[Mark Banner (:standard8)]

(In reply to comment #5)
> I don't think the ifdef MOZ_SUITE is necessary as nobody else should be running
> this Makefile except for camino which is already excluded. I also think your
> ifdef SUITE_USING_TOOLKIT_DM should be an ifndef.

Not quite right, everyone is still running this Makefile, but its only Suite/Camino currently running that part of the Makefile.

Comment 8

[Justin Wood (:Callek)]

Attachment: patch to mozilla-central

Here is the updated mozilla-central patch...

(Note: suite/app-config.mk is included in this patch though would not be pushed to mozilla-central, but it would be applied to 1.9.0.x when it comes time)

Comment 9

[[e-mail]]

Comment on attachment 326683 [details] [diff] [review]
patch to mozilla-central

>+++ b/suite/app-config.mk
>@@ -0,0 +1,2 @@
>+SUITE_USING_XPFE_DM=1
>+DEFINES+="SUITE_USING_XPFE_DM=1"
I don't think quotes are right here, and I think spaces would be a good idea.

>\ No newline at end of file
Fix please!

>-ifndef MOZ_SUITE
>-# XXX Suite doesn't want these just yet
> ifdef MOZ_RDF
>+ifndef SUITE_USING_XPFE_DM
[Twice] Please leave the ifdefs in the same order i.e. suite first, then RDF.

>-endif
>-endif
>+endif # SUITE_USING_XPFE_DM
>+endif # MOZ_RDF
And I don't see the point of reannotating these, it's only a 1-line ifdef!

> ifneq ($(MOZ_BUILD_APP),camino)
>+ifdef SUITE_USING_XPFE_DM
You could probably remove the camino check; they won't set SUITE_USING_XPFE_DM

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 326683 [details] [diff] [review]
patch to mozilla-central

+DEFINES+="SUITE_USING_XPFE_DM=1"

I think this needs to be:
DEFINES+="-DSUITE_USING_XPFE_DM=1"

Comment 11

[Justin Wood (:Callek)]

Attachment: Patch for checkin

Pushed to mozilla-central, (minus suite/app-config.mk) going to let it bake a bit before committing to CVS (and requesting approval)

pushing to ssh://hg.mozilla.org/mozilla-central/
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 7 changes to 7 files

Comment 12

[Justin Wood (:Callek)]

Comment on attachment 326826 [details] [diff] [review]
Patch for checkin

baking fine on mozilla-central. Should only affect seamonkey build.

Comment 13

[Robert Kaiser]

Unfortunately, this patch is incomplete and I think the bug should be reopened. As app-config.mk gets included by config.mk, we need to have the latter loaded before using any such var in a Makefile.in!

Comment 14

[Robert Kaiser]

Attachment: supplemantal patch for including config.mk early enough

I'm not sure if it's the ideal place for every one of those includes, but those includes of config.mk are needed for the new flag and MOZ_SUITE/MOZ_THUNDERBIRD to work in the new world, as it's now config.mk which loads those.

Comment 15

[(not currently active) Ted Mielczarek]

Comment on attachment 327280 [details] [diff] [review]
supplemantal patch for including config.mk early enough

Looks good.

Comment 16

[Robert Kaiser]

supplementary patch checked in as changeset e7939e8a558e, so should be really fixed now.

Comment 17

[Samuel Sidler (old account; do not CC)]

Is this patch needed in CVS (1.9.0) since SeaMonkey is moving to hg? If so, please provide a combined patch with both the original patch and the follow up one.

Comment 18

[Justin Wood (:Callek)]

Comment on attachment 326826 [details] [diff] [review]
Patch for checkin

removing approval request as it does appear we won't need this in 1.9.0 due to SeaMonkey's move happening even sooner than I expected.