Closed
Bug 440932
Opened 15 years ago
Closed 15 years ago
toolkit dlmgr should be buildable by suite
Categories
(Toolkit Graveyard :: Build Config, defect)
Toolkit Graveyard
Build Config
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: Callek, Assigned: Callek)
References
Details
Attachments
(2 files, 2 obsolete files)
5.14 KB,
patch
|
Details | Diff | Splinter Review | |
1.71 KB,
patch
|
ted
:
review+
|
Details | Diff | 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)
Assignee | ||
Comment 1•15 years ago
|
||
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 | ||
Updated•15 years ago
|
Assignee: nobody → bugspam.Callek
Assignee | ||
Updated•15 years ago
|
Status: NEW → ASSIGNED
Comment 2•15 years ago
|
||
Is anyone else still using the xpfe download manager?
Assignee | ||
Comment 3•15 years ago
|
||
re: c#2, not that I could tell.
Assignee | ||
Updated•15 years ago
|
Attachment #326074 -
Flags: review?(sdwilsh) → review?(ted.mielczarek)
Comment 4•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
Comment on attachment 326074 [details] [diff] [review] patch to mozilla-central Address Neil's comments first, thanks.
Attachment #326074 -
Flags: review?(ted.mielczarek) → review-
Comment 7•15 years ago
|
||
(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.
Assignee | ||
Comment 8•15 years ago
|
||
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 9•15 years ago
|
||
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 10•15 years ago
|
||
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+
Assignee | ||
Comment 11•15 years ago
|
||
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
Assignee | ||
Updated•15 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 12•15 years ago
|
||
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?
![]() |
||
Comment 13•15 years ago
|
||
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!
![]() |
||
Updated•15 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
![]() |
||
Comment 14•15 years ago
|
||
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)
Updated•15 years ago
|
Attachment #326826 -
Flags: approval1.9.0.1? → approval1.9.0.2?
Comment 15•15 years ago
|
||
Comment on attachment 327280 [details] [diff] [review] supplemantal patch for including config.mk early enough Looks good.
Attachment #327280 -
Flags: review?(ted.mielczarek) → review+
![]() |
||
Comment 16•15 years ago
|
||
supplementary patch checked in as changeset e7939e8a558e, so should be really fixed now.
Status: REOPENED → ASSIGNED
![]() |
||
Updated•15 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
Comment 17•15 years ago
|
||
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.
Assignee | ||
Comment 18•15 years ago
|
||
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?
Updated•4 years ago
|
Product: Toolkit → Toolkit Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•