Sunbird Trunk fails to build since 2007-07-03 03:00 PDT Log file shows: mozilla/calendar/sunbird/app/nsCalendarApp.cpp(45) : fatal error C1083: Cannot open include file: 'nsBuildID.h': No such file or directory This seems to be caused by Bug 383167. Sunbird requires the same changes to make it work again.
Is this similar to bug #386653 ?
Created attachment 271376 [details] [diff] [review] rev0 - Port browser and mail changes Patch is for TRUNK only and ports the changes made to browser and mail in Bug 383167. I can successfully build and run Sunbird on Windows 2000 system. Also fixes the issue with the now empty BuildID in Lightning.
Comment on attachment 271376 [details] [diff] [review] rev0 - Port browser and mail changes I'd also like to get a look from Benjamin because he wrote the original patch for browser and mail.
(In reply to comment #1) > Is this similar to bug #386653 ? It appears this is the Sunbird-specific version of that Camino-specific bug, yes.
Created attachment 271700 [details] [diff] [review] rev1 - Port browser and mail changes Minor change to add changes from bug 386658.
Increasing severity because this blocks development and testing work on Trunk.
Comment on attachment 271700 [details] [diff] [review] rev1 - Port browser and mail changes patch looks fine to me (and works fine, too!) But it looks trunk-only. Is there a way to make it work on branch too, thus preventing a fork of the files?
(In reply to comment #7) Yes, this patch is Trunk only. If requested I could try to provide a patch for Trunk and MOZILLA_1_8_BRANCH. But I worry about the files (especially the make files) being too crowded and unreadable afterwards.
At least packages-static should not be forked. That file looks easy enough to add an #ifdef to, and we change that file all the time. Having it forked will be a pain later on. For the other files, i'm not sure. They indeed look like they will become somewhat unreadable by adding a lot of #ifdef's...
Created attachment 272512 [details] [diff] [review] rev2 - patch for TRUNK only This is the TRUNK only part of the rev1 patch containing the changes for the Sunbird specific files. This files are going to be changed not very often so it should be OK to fork this files.
Created attachment 272513 [details] [diff] [review] rev2 - patch for TRUNK and MOZILLA_1_8_BRANCH This is the TRUNK and MOZILLA_1_8_BRANCH part of the rev1 patch containing the changes for Lightning and packages-static to avoid forking this files.
Comment on attachment 271700 [details] [diff] [review] rev1 - Port browser and mail changes Canceling review due to the follow up patches to avoid forking if possible.
The Trunk is now busted since almost one month. mvl, any chance to do the review in reasonable time-frame? Otherwise I would move the review request to daniel or ctalbert.
I can confirm the patches work well on my MacOSX trunk build, although I am wondering why we shouldn't rework the "TRUNK only" patch to use ifdefs, too, and keep trunk and branch in sync. One thing I am experiencing is that I can start sunbird only the first time; any further start reminds me that "a copy of sunbird is already open" (seems to be a stale lock file). Possibly some other trunk bug, I assume.
I'm not sure about the need for keeping trunk and branch in sync. After all, they are split in cvs anyway, so why try to duplicated that cvs functionality into preprocessed code? The code is not functionality, but just to keep things running. I think we can get away with forking. But I'll leave that final decision to Daniel. If the decision is to not fork, you should just wrap some larger blocks in an ifdef. I think that's doable. Besides that, the code looks fine.
When we switch Trunk from Talkback to Breakpad, we'll have many Trunk only changes in the calendar/installer/ directory. In my opinion we should at least fork the whole installer directory (at the moment it's forked and/or ifdefed depending on the file).
(In reply to comment #16) > I'm not sure about the need for keeping trunk and branch in sync. After all, > they are split in cvs anyway, so why try to duplicated that cvs functionality > into preprocessed code? The code is not functionality, but just to keep things > running. I think we can get away with forking. I don't know if I get your point, but in general I think it's sensible to keep trunk and branch in sync since it makes code changes easier (and you could rely on almost the same program logic). Although I don't feel in the position to judge on this (because I mainly concentrate on the branch), my attitude is that as long as branch and trunk don't diverge too much, but only have slight differences, it makes sense to use ifdefs and the like if it doesn't hurt the overall readability. But if files do diverge too much (nsCalendarApp.cpp, coming breakpad changes?), it makes of course no sense. Regarding diverging code files a viable option may be abstracting functionality into an internal API (implemented separately for trunk and branch). Summed up, I propose to decide on a per file basis with respect to *readability*. Just my 2 cents...
(In reply to comment #15 - comment #18) I split up the patch with regard to readability and number of expected changes, see Comment #10 and Comment #11.
(In reply to comment #19) > (In reply to comment #15 - comment #18) > > I split up the patch with regard to readability and number of expected changes, > see Comment #10 and Comment #11. > No doubt about that, no offense intended really. I just wanted to know about the general opinion when handling branch vs trunk.
I agree with daniel in comment 18. I don't think we should force huge chunks of code to go into an ifdef, especially when the file won't be touched much (like the startup code). The difference between trunk and branch can be handled perfectly fine by cvs, we don't need to try to do that always on our own with ifdef. But in some cases, it is indeed easier to use those ifdefs, so patches are easier to checkin.
Comment on attachment 272512 [details] [diff] [review] rev2 - patch for TRUNK only r=mvl
Comment on attachment 272513 [details] [diff] [review] rev2 - patch for TRUNK and MOZILLA_1_8_BRANCH r=mvl
Checked in on HEAD resp. HEAD and MOZILLA_1_8_BRANCH.