Closed Bug 394946 Opened 17 years ago Closed 15 years ago

BeOS Thunderbird builds fail in migration code

Categories

(Thunderbird :: Migration, defect)

x86
BeOS
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: doug, Assigned: doug)

Details

(Keywords: fixed1.8.1.8)

Attachments

(1 file, 1 obsolete file)

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
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)
Status: NEW → ASSIGNED
Version: unspecified → 2.0
Assignee: nobody → doug
Status: ASSIGNED → NEW
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-
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 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+
Attachment #280251 - Flags: approval-thunderbird2?
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.
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 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?
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
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: helpwantedfixed1.8.1.8
Sergei/Doug: The Mac Thunderbird tinderbox (1.8) is burning due to your check-in. Please take a look.
Will look just now
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.
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.
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.
MacOSX Darwin 8.7.0 bm-xserve02 Dep Tb-Nightly tinderbox is green again.
I hope, bustage is fixed.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
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 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-
(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
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.
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".
(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?
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
(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!
(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.


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.
be aware, 4.x migration support is dropped on trunk.
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.
Fine by me.
=> fixed then.  thanks
Status: REOPENED → RESOLVED
Closed: 17 years ago15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.