Closed Bug 440932 Opened 12 years ago Closed 12 years ago
toolkit dlmgr should be buildable by suite
+++ 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)
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 → ---
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.
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 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: approval220.127.116.11?
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!
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)
Attachment #326826 - Flags: approval18.104.22.168? → approval22.214.171.124?
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 ago → 12 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: approval126.96.36.199?
You need to log in before you can comment on or make changes to this bug.