Closed
Bug 456715
Opened 16 years ago
Closed 16 years ago
Remove COMM_BUILD ifdefs from calendar code
Categories
(Calendar :: General, defect)
Calendar
General
Tracking
(Not tracked)
RESOLVED
FIXED
1.0b1
People
(Reporter: ssitter, Assigned: ssitter)
References
()
Details
Attachments
(1 file, 1 obsolete file)
3.24 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
Calendar code is now located in comm-central repository. Therefore it should be save to ensure that comm-central build system is always used. The COMM_BUILD ifdefs that were required as long as calendar was built from CVS can most probably be removed. http://mxr.mozilla.org/comm-central/search?string=COMM_BUILD&find=/calendar/
Assignee | ||
Comment 1•16 years ago
|
||
Mark, Robert, please correct me if my assumptions are wrong.
Comment 2•16 years ago
|
||
(In reply to comment #1) > Mark, Robert, please correct me if my assumptions are wrong. I think you're fine to remove those ifdefs that currently are displayed on mxr. Be aware of the new COMM_BUILD ifdefs that I checked in today as part of bug 453330 - you don't want to remove those.
Comment 3•16 years ago
|
||
Yes, those COMM_BUILD ifdefs I introduced when the code was still in CVS are obsolete now, we should only leave the case in that was used when COMM_BUILD is set. The few special top-level files where Mark introduced those ifdefs need to keep them though.
Assignee | ||
Comment 4•16 years ago
|
||
This patch removes only the obsolete COMM_BUILD ifdefs that were already added in CVS.
(In reply to comment #2) > (In reply to comment #1) > > Mark, Robert, please correct me if my assumptions are wrong. > > I think you're fine to remove those ifdefs that currently are displayed on mxr. > Be aware of the new COMM_BUILD ifdefs that I checked in today as part of bug > 453330 - you don't want to remove those. what's the idea behind those?
Comment 6•16 years ago
|
||
(In reply to comment #5) > (In reply to comment #2) > > (In reply to comment #1) > > > Mark, Robert, please correct me if my assumptions are wrong. > > > > I think you're fine to remove those ifdefs that currently are displayed on mxr. > > Be aware of the new COMM_BUILD ifdefs that I checked in today as part of bug > > 453330 - you don't want to remove those. > > what's the idea behind those? Some files in comm-central are read by the build system in mozilla-central. When that happens, we need mozilla-central to pick up different settings at times, some of this is due to the different directory locations, or, in the case of makefiles, we don't want the mozilla-central build system to generate the makefiles for us as it doesn't know about (= work with) anything outside its src directory too well.
unfortunately the previous patch no longer applies. i tried to fix that. how to proceed with reviews now?
Comment 8•16 years ago
|
||
(In reply to comment #7) > Created an attachment (id=342059) [details] > previous patch updated > > unfortunately the previous patch no longer applies. i tried to fix that. how to > proceed with reviews now? How about (given that I'm one of the comm-central build config team) I take the review? It all looks fine, I've just gotta give it a quick run.
Assignee | ||
Comment 9•16 years ago
|
||
Based on Philipps comment in Bug 455869 we probably should exclude the changes in calendar/providers/gdata and leave the files as is.
Comment 10•16 years ago
|
||
Comment on attachment 342059 [details] [diff] [review] [checked in] previous patch updated This looks fine r=Standard8, but leave calendar/providers/gdata alone as per comment 9.
Attachment #342059 -
Flags: review+
Attachment #342059 -
Attachment description: previous patch updated → [checked in] previous patch updated
Attachment #340219 -
Attachment is obsolete: true
Attachment #340219 -
Flags: review?(ause)
Comment 11•16 years ago
|
||
committed without changes in calendar/providers/gdata
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Target Milestone: --- → 1.0
Updated•14 years ago
|
Target Milestone: 1.0 → 1.0b1
You need to log in
before you can comment on or make changes to this bug.
Description
•