Closed Bug 456715 Opened 16 years ago Closed 16 years ago

Remove COMM_BUILD ifdefs from calendar code

Categories

(Calendar :: General, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ssitter, Assigned: ssitter)

References

()

Details

Attachments

(1 file, 1 obsolete file)

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/
Mark, Robert, please correct me if my assumptions are wrong.
(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.
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.
Attached patch remove COMM_BUILD ifdefs (obsolete) — Splinter Review
This patch removes only the obsolete COMM_BUILD ifdefs that were already added in CVS.
Assignee: nobody → ssitter
Status: NEW → ASSIGNED
Attachment #340219 - Flags: review?(ause)
(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?
(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?
(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.
Based on Philipps comment in Bug 455869 we probably should exclude the changes in calendar/providers/gdata and leave the files as is.
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)
committed without changes in calendar/providers/gdata
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.