Closed Bug 440932 Opened 12 years ago Closed 12 years ago

toolkit dlmgr should be buildable by suite

Categories

(Toolkit Graveyard :: Build Config, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Callek, Assigned: Callek)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached patch patch to mozilla-central (obsolete) — Splinter Review
+++ 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)
Attachment #326074 - Flags: superreview?(neil)
Attachment #326074 - Flags: review?(sdwilsh)
whops, correcting summary and component...
Assignee: bugspam.Callek → nobody
Status: ASSIGNED → NEW
Component: Download Manager → Build Config
Product: Mozilla Application Suite → Toolkit
QA Contact: download-manager → build-config
Summary: Make SeaMonkey download manager use toolkit backend → toolkit dlmgr should be buildable by suite
Target Milestone: seamonkey1.0alpha → ---
Assignee: nobody → bugspam.Callek
Status: NEW → ASSIGNED
Is anyone else still using the xpfe download manager?
re: c#2, not that I could tell.
Attachment #326074 - Flags: review?(sdwilsh) → review?(ted.mielczarek)
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 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 on attachment 326074 [details] [diff] [review]
patch to mozilla-central

Address Neil's comments first, thanks.
Attachment #326074 - Flags: review?(ted.mielczarek) → review-
(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.
Attached patch patch to mozilla-central (obsolete) — Splinter Review
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)
Attachment #326074 - Attachment is obsolete: true
Attachment #326683 - Flags: superreview?(neil)
Attachment #326683 - Flags: review?(ted.mielczarek)
Attachment #326074 - Flags: superreview?(neil)
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
Attachment #326683 - Flags: superreview?(neil) → superreview+
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"
Attachment #326683 - Flags: review?(ted.mielczarek) → review+
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
Attachment #326683 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 326826 [details] [diff] [review]
Patch for checkin

baking fine on mozilla-central. Should only affect seamonkey build.
Attachment #326826 - Flags: approval1.9.0.1?
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!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #327280 - Flags: review?(ted.mielczarek)
Blocks: 437643
Attachment #326826 - Flags: approval1.9.0.1? → approval1.9.0.2?
Comment on attachment 327280 [details] [diff] [review]
supplemantal patch for including config.mk early enough

Looks good.
Attachment #327280 - Flags: review?(ted.mielczarek) → review+
supplementary patch checked in as changeset e7939e8a558e, so should be really fixed now.
Status: REOPENED → ASSIGNED
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
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 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.
Attachment #326826 - Flags: approval1.9.0.2?
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.