Closed
Bug 394946
Opened 17 years ago
Closed 16 years ago
BeOS Thunderbird builds fail in migration code
Categories
(Thunderbird :: Migration, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: doug, Assigned: doug)
Details
(Keywords: fixed1.8.1.8)
Attachments
(1 file, 1 obsolete file)
2.34 KB,
patch
|
sergei_d
:
review+
philor
:
review-
mscott
:
approval1.8.1.8+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (BeOS; U; BeOS BePC; en-US; rv:1.8.1.7pre) Gecko/20070903 BonEcho/2.0.0.7pre Build Identifier: 2.0.0.7pre Two sections of the 2.x migration code need small additions to allow BeOS building. Reproducible: Always Steps to Reproduce: 1. 2. 3. Actual Results: builds fail in mozilla/mail/components/migration/src Expected Results: clean build
Assignee | ||
Comment 1•17 years ago
|
||
Fixes allow Thunderbird to build under BeOS. Sergei, please review code to insure I didn't somehow mess up the conditionals.
Attachment #279655 -
Flags: review?(sergei_d)
Assignee | ||
Updated•17 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•17 years ago
|
Version: unspecified → 2.0
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → doug
Status: ASSIGNED → NEW
Comment 2•17 years ago
|
||
Comment on attachment 279655 [details] [diff] [review] Patch to allow Thunderbird 2.0.0.7pre to build under BeOS BeOS used "appreg" file, like unix, not "Application Registry". You can look onto simular already existing text of BeOS section in mozilla/browser/components/migration/src/nsNetscapeProfileMigratorBase.cpp
Attachment #279655 -
Flags: review?(sergei_d) → review-
Assignee | ||
Comment 3•17 years ago
|
||
Thanks, Sergei! I've changed the patch to the Dogbert migrator based on your guidance. Also, since you mentioned we use Unix structure, can I assume the patch to nsProfileMigrator.cpp is correct syntax to use unix code for unix and beos?
Attachment #279655 -
Attachment is obsolete: true
Attachment #280251 -
Flags: review?(sergei_d)
Comment 4•17 years ago
|
||
Comment on attachment 280251 [details] [diff] [review] Revised patch based on Sergei's guidance r=sergei_d This time looks OK for me. Only thing i wonder why browser and mail components use bit different style for conditional defines in migrators code - browser just puts whole MACOS case before UNIX, so no need for && !defined(MACOSX) case in UNIX section, while mail component does it in in different way. But that's question not for dougt:)
Attachment #280251 -
Flags: review?(sergei_d) → review+
Assignee | ||
Updated•17 years ago
|
Attachment #280251 -
Flags: approval-thunderbird2?
Assignee | ||
Comment 5•17 years ago
|
||
Thanks, Sergei! I was surprised to see essentially the same code in two different places in the tree. Even more surprised to see the two different approaches to the same task. But you're correct - an old dog like me won't ever understand that! Let's try to get it in the 2.0 branch now.
Assignee | ||
Comment 6•17 years ago
|
||
Comment on attachment 280251 [details] [diff] [review] Revised patch based on Sergei's guidance can we please apply this patch to the TBird 2.x branch in time for the next release?
Attachment #280251 -
Flags: approval1.8.1.8?
Comment 7•17 years ago
|
||
Comment on attachment 280251 [details] [diff] [review] Revised patch based on Sergei's guidance if you can get it in ASAP :)
Attachment #280251 -
Flags: approval1.8.1.8?
Attachment #280251 -
Flags: approval1.8.1.8+
Attachment #280251 -
Flags: approval-thunderbird2?
Assignee | ||
Comment 8•17 years ago
|
||
Sergei (or maybe Biesi) I am having trouble checking items in at the moment. Is there any chance one of you might be able to check in this patch before 1.8.1.8 hits?
Keywords: helpwanted
Comment 9•17 years ago
|
||
Checking in mozilla/mail/components/migration/src/nsDogbertProfileMigrator.cpp; /cvsroot/mozilla/mail/components/migration/src/nsDogbertProfileMigrator.cpp,v <-- nsDogbertProfileMigrator.cpp new revision: 1.3.12.1; previous revision: 1.3 done Checking in mozilla/mail/components/migration/src/nsProfileMigrator.cpp; /cvsroot/mozilla/mail/components/migration/src/nsProfileMigrator.cpp,v <-- nsProfileMigrator.cpp new revision: 1.8.4.1; previous revision: 1.8 done
Keywords: helpwanted → fixed1.8.1.8
Comment 10•17 years ago
|
||
Sergei/Doug: The Mac Thunderbird tinderbox (1.8) is burning due to your check-in. Please take a look.
Comment 11•17 years ago
|
||
Will look just now
Comment 12•17 years ago
|
||
I really disliked that style of defines used here in nsDogbertProfileMigrator.cpp as noticed in comment #4 I hope that #if defined((XP_UNIX) || (XP_BEOS)) && !defined(XP_MACOSX) will work better than current #if defined(XP_UNIX) || defined(XP_BEOS) && !defined(XP_MACOSX) but I cannot test at the moment nor BeOS build neither MAC build. Safest version is add separate XP_BEOS case let it be full duplicate of XP_UNIX. But for fast bustage fix I can also rollback changes to previous state of nsDogbertProfileMigrator.cpp if it is needed.
Comment 13•17 years ago
|
||
looks like #if (defined(XP_UNIX) || defined(XP_BEOS)) && !defined(XP_MACOSX) should be safer If it isn't, I can't get logic how it worked before.
Comment 14•17 years ago
|
||
Frank, looks like #if (defined(XP_UNIX) || defined(XP_BEOS)) && !defined(XP_MACOSX) should be safer, anf fix Mac tinderbox: Checking in mozilla/mail/components/migration/src/nsDogbertProfileMigrator.cpp; /cvsroot/mozilla/mail/components/migration/src/nsDogbertProfileMigrator.cpp,v <-- nsDogbertProfileMigrator.cpp new revision: 1.3.12.2; previous revision: 1.3.12.1 done If it isn't, I can't get logic how it worked before.
Comment 15•17 years ago
|
||
MacOSX Darwin 8.7.0 bm-xserve02 Dep Tb-Nightly tinderbox is green again. I hope, bustage is fixed.
Assignee | ||
Updated•17 years ago
|
Keywords: fixed1.8.1.8 → verified1.8.1.8
Assignee | ||
Updated•17 years ago
|
Status: NEW → RESOLVED
Closed: 17 years ago
Keywords: verified1.8.1.8 → fixed1.8.1.8
Resolution: --- → FIXED
Comment 16•17 years ago
|
||
How is this FIXED? Near as I can tell, trunk and branch had the same code before it was "fixed" on the branch, and not on the trunk. Am I missing something, that makes the fix unnecessary on the trunk?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 17•17 years ago
|
||
Comment on attachment 280251 [details] [diff] [review] Revised patch based on Sergei's guidance Just in case anyone decides to check this (or the bustage-fixed version) in on the trunk... please don't. UNIX !MAC is a single logical unit, because XP_MACOSX defines XP_UNIX as well, since most of the time that's what it needs. The correct bustage fix would have been (defined(XP_UNIX) && !defined(XP_MACOSX)) || defined(XP_BEOS), but the correct trunk fix is to move the XP_MACOSX section above the XP_UNIX || XP_BEOS section.
Attachment #280251 -
Flags: review-
Comment 18•17 years ago
|
||
(In reply to comment #16) >. Am I missing > something, that makes the fix unnecessary on the trunk? We didn't land patch on trunk, AFAIK, as trunk is dead atm for BeOS for several reasons, mainly due requirement of gcc >3.* and old ABI ib BeOS (system API is based on C++ there). What about changes you mentioned - I wondered here myself, why thunderbird code don't follow Firefox example: http://lxr.mozilla.org/seamonkey/source/browser/components/migration/src/nsDogbertProfileMigrator.cpp#74 where XP_MACOSX has separate section before all other cases
Assignee | ||
Comment 19•17 years ago
|
||
Sergei, it would be good to land our fixes on the trunk as a matter of policy. While the trunk is essentially dead for BeOS right now, Haiku becoming more stable by the day and opens the possibility of GCC4. If we find and fix problems on the branch, we should probably add the fixes on the trunk in preparation for the day when we can revive trunk builds.
Assignee | ||
Comment 20•17 years ago
|
||
Shall I open a separate bug for fixing this on the trunk, using Phil's proposed fix? If yes, we can change this resolution to "Fixed".
Comment 21•17 years ago
|
||
(In reply to comment #18) > We didn't land patch on trunk, AFAIK, as trunk is dead atm for BeOS Until you're actually dead (as in, all BeOS-specific files are removed, and all XP_BEOS ifdefs are removed, and we've all said goodbye), please act as though you're alive: you're just piling up more pain for yourself by not making changes like this that you can make without needing to be able to compile to test them on BeOS. > why thunderbird code don't follow Firefox example: I'd guess because Firefox's change was one of several dozen correctness fixes in the midst of bug 239929, which was very busy breaking Tbird in too many ways for anyone to notice that it was also doing good things to code that Tbird had forked. (In reply to comment #20) > Shall I open a separate bug for fixing this on the trunk Doesn't seem like a very good idea - you already have a perfectly good bug for it, with plenty of information about it: this one. All you need to do is decide whether you really want Firefox's fix (which, note, is not just the order of the ifdefs, but also http://lxr.mozilla.org/seamonkey/source/browser/components/migration/src/Makefile.in#79 where you just get excluded from building the file), or just to sort things into the right order. Has anyone actually tested the resulting migration code from the branch fix, to see whether it works, or just compiles?
Comment 22•17 years ago
|
||
I think best solution here is to submit two new patches, for branch and trunk, and ask Phil for review then. (What about usual #define code if no special section for MACOSX exists, really, it is #if (defined(XP_UNIX) && !defined(XP_MACOSX)) || defined(XP_BEOS). So, need response from some MacOS X branch TB user - if migration really works for them.) Also, there is task for doug here, about another missing BeOS defs - there is idea to search lxr.mozilla.org for text !defined(XP_MACOSX), and in result of that search look for defined(XP_UNIX) - and see if we still miss defined(XP_BEOS) in some important component
Assignee | ||
Comment 23•17 years ago
|
||
(In reply to comment #22) > Also, there is task for doug here, about another missing BeOS defs - there is > idea to search lxr.mozilla.org for text !defined(XP_MACOSX), and in result of > that search look for defined(XP_UNIX) - and see if we still miss > defined(XP_BEOS) in some important component > Great idea! Thanks for a task suited to my skills, Sergei!
Assignee | ||
Comment 24•17 years ago
|
||
(In reply to comment #21) >Has anyone actually tested the resulting migration code from the branch fix, to > see whether it works, or just compiles? I'll happily test, but there's usually not much to import in the BeOS environment that's addressed by this import code. I'll try importing contacts from .csv if I can find the file format that's required.
Comment 25•17 years ago
|
||
nsDogbertProfileMigrator imports Netscape 4.x profiles: if 4.x actually ran on BeOS, then either it should import your profile, or that should be fixed, or we should give up like Fx apparently did, and not build it at all for BeOS.
Comment 26•16 years ago
|
||
be aware, 4.x migration support is dropped on trunk.
Assignee | ||
Comment 27•16 years ago
|
||
Thanks for this notification, Wayne. Sergei and tqh, I suggest we close this bug since it's fixed on the branch and is no longer applicable to the trunk.
Comment 28•16 years ago
|
||
Fine by me.
Comment 29•16 years ago
|
||
=> fixed then. thanks
Status: REOPENED → RESOLVED
Closed: 17 years ago → 16 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•