272194 - Firefox/Sunbird/Thunderbird builds fail when built from trunk

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Andy Willis (abwillis)]

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.8a6) Gecko/20041127
Build Identifier: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.8a6) Gecko/20041127

Opening bug to submit patch.

Reproducible: Always
Steps to Reproduce:
1.  Pull code for browser/sunbird/thunderbird
2.  Build one of the above.
3.

Actual Results:  
E:/cvs/work/mozilla/toolkit/xre/nsNativeAppSupportOS2.cpp:303: candidate is:
   virtual void nsNativeAppSupportOS2::Quit()
E:/cvs/work/mozilla/toolkit/xre/nsNativeAppSupportOS2.cpp:943: `nsresult
   nsNativeAppSupportOS2::Quit()' and `virtual void
   nsNativeAppSupportOS2::Quit()' cannot be overloaded
make.exe[4]: *** [nsNativeAppSupportOS2.o] Error 1 
Caused by change in https://bugzilla.mozilla.org/show_bug.cgi?id=237745 
and 
E:/cvs/work/mozilla/toolkit/xre/nsNativeAppSupportOS2.cpp: In member function `
   virtual nsresult nsNativeAppSupportOS2::StartDDE()':
E:/cvs/work/mozilla/toolkit/xre/nsNativeAppSupportOS2.cpp:883: error: invalid
   conversion from `const char* const' to `char*'
make.exe[1]: *** [nsNativeAppSupportOS2.o] Error 1
make.exe[1]: Leaving directory `E:/cvs/work/mozilla/ffobj/toolkit/xre' 
caused by change in https://bugzilla.mozilla.org/show_bug.cgi?id=258217 
Opening bug to attach patch to fix both these issues in
mozilla/toolkit/xre/nsNativeAppSupportOS2.cpp


Expected Results:  
Build without error.

Most Firefox builds are done from the Aviary Branch but the above bugs were not
applied against the Branch, just the Trunk so it likely hasn't been seen.

Comment 1

[Andy Willis (abwillis)]

Attachment: mozilla/toolkit/xre/nsNativeAppSupportOS2.cpp

Comment 2

[Felix Miata]

looks like confirmed by absence of trunk nightlies since 23 Nov

Comment 3

[Peter Weilbacher]

No, the suite in unaffected by this (it's seems that the people administrating
the nightly builds machine did not yet cope with the .mozconfig change).

While the patch is a start I think someone needs to make a more detailed
comparison between toolkit/xre/nsNativeAppSupportWin.cpp and
toolkit/xre/nsNativeAppSupportOS2.cpp (and
xpfe/bootstrap/nsNativeAppSupportOS2.cpp), there seems to be quite a lot that
never made it into the OS/2 version (like dependence of nsIObserver), I am not
sure that e.g. the semi-single profile ever made it to the trunk (version 1.8 of
the file as seen in
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/toolkit/xre/nsNativeAppSupportWin.cpp).

Comment 4

[Mike Kaply [:mkaply]]

Actually, I did do a toolkit resync of nsNativeAppSupport. Guess I'll do it again.

Comment 5

[Andy Willis (abwillis)]

It looks like quite a bit of the 1.8 stuff is in there already.  The things that
I don't see seem to be rather different than the windows version was originally
so might not be applicable...  that determination is beyond my knowledge though.

Comment 6

[Andy Willis (abwillis)]

Attachment: change for aviary to work with branding

Comment 7

[Andy Willis (abwillis)]

Not sure if we are wanting to use this bug for updating nsnativesupportos2 or
for getting the aviaries to build from trunk.  Here is a patch for the latter
from a post in the newsgroup from Rich.

Comment 8

[Mike Kaply [:mkaply]]

Comment on attachment 167320 [details] [diff] [review]
mozilla/toolkit/xre/nsNativeAppSupportOS2.cpp

r=mkaply, sr=blizzard (platform specific)

Comment 9

[Mike Kaply [:mkaply]]

Weird. I don't see the RC problem at all.

Comment 10

[Mike Kaply [:mkaply]]

That change was definitely in the firefox branch and not on trunk, so I put it
on trunk.

I think this bug is done then.

Comment 11

[Andy Willis (abwillis)]

This should be done for sunbird as well:
at line 156 in mozilla\calendar\sunbird\app\makefile.in
RCFLAGS += -DSUNBIRD_ICO=\"$(DIST)/branding/sunbird.ico\"
should be:
RCFLAGS += -DSUNBIRD_ICO="$(DIST)/branding/sunbird.ico"
Apparently it is branded but Thunderbird is not at this time (at least it
doesn't have the same format as sunbird and firefox).
The patch that was originally submitted to the aviary branch requires changes in
the makefile.in as well as to rules.mk.  The patch here was a subsequent patch
by Rich that did not require changes in the makefile.in file.
Andy

Comment 12

[Mike Kaply [:mkaply]]

I don't want the change you referenced.

My goal was to use a rules.mk change so that the -D lines between Windows and
OS/2 could stay the same, so that eventually they could be shared.

I don't think that rules.mk change was correct - what we need to figure out is
why I don't need the rules.mk change and other people do - my RC.EXE handles
forward slashes no problem.

Comment 13

[Peter Weilbacher]

Yes, I remember that we compared versions before and you have a newer rc.exe
than in available for general use. If you could make yours available somehow
that would certainly solve the problem.

Comment 14

[Mike Kaply [:mkaply]]

Try this one:

http://www.kaply.com/work/rcos2-5.00.007.zip

Comment 15

[Andy Willis (abwillis)]

That seems to fix it for me.  In that case, do we want to put the \ back in the
makefile.in from browser\app?  I think that the makefile.in in browser and
sunbird should use the same format, whichever one is chosen.

Comment 16

[Mike Kaply [:mkaply]]

OK, so the only thing left for this is to make my rc.exe public, correct?

Comment 17

[Andy Willis (abwillis)]

Yes.  It is everything is working well since I got the new rc.exe.

Comment 18

[Mike Kaply [:mkaply]]

Attachment: Special case rules.mk

I believe Rich Walsh had this idea originally.

Basically forward slash to back slash and \" to "

This is working with my old RC compiler. I'll check some others.

Comment 19

[Rich Walsh]

My original version (which is in your patch) had the first backslash doubled, i.e

$(RC) $(subst /,\\,$(subst \",",$(RCFLAGS:-D%=-d %))) -i $(subst /,\,$(srcdir))
-r $< $@


The version I use currently doesn't.  I have no idea whether it matters (plus
I'm on the newer rc.exe)

$(RC) $(subst /,\,$(subst \",",$(RCFLAGS:-D%=-d %))) -i $(subst /,\,$(srcdir))
-r $< $@

Comment 20

[Mike Kaply [:mkaply]]

(In reply to comment #19)
> My original version (which is in your patch) had the first backslash doubled, 

I knew your patch was somewhere, I just couldn't find it. I'm slow :)

I think the double slash is needed on the old RC.

I did some testing, and I'm still trying to figure out what "old RC" even means
though.

The RC in the os2tk45 crashes right off the bat.

I think with the rules.mk patch, we can just tell people to use this one:

ftp://ftp.software.ibm.com/ps/products/warpzilla/os2tk40rc.zip

The reason I haven't uploaded a new one is that I don't have access to that FTP
site anymore.

Comment 21

[Steve Wendt]

Is there something still unresolved for this bug?

Comment 22

[Mike Kaply [:mkaply]]

I think we are good here.