Build error during Windows cross-compile of Thunderbird on Linux (type mismatch in nsMsgImapSearch.cpp)

RESOLVED FIXED in Thunderbird 60.0

Status

defect
RESOLVED FIXED
Last year
Last year

People

(Reporter: mozbugs, Assigned: mozbugs)

Tracking

Thunderbird 60.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Assignee

Description

Last year
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20180219150722

Steps to reproduce:

On cross-compiling comm-esr52 (THUNDERBIRD_52_6_0_RELEASE tag) for Windows on Linux using mingw-w64, the . Please see the attached log for details but a brief summary is:

{{{
20:38.34 /var/tmp/build/thunderbird-48f960ae7db5/mailnews/base/search/src/nsMsgImapSearch.cpp: In static member function 'static nsresult nsMsgSearchOnlineMail::Encode(nsCString&, nsISupportsArray*, const char16_t*)':
20:38.34 /var/tmp/build/thunderbird-48f960ae7db5/mailnews/base/search/src/nsMsgImapSearch.cpp:122:43: error: call of overloaded 'NS_IsAscii(char16ptr_t)' is ambiguous
20:38.34          asciiOnly = NS_IsAscii(pchar.get());
20:38.34                                            ^
20:38.34 In file included from /var/tmp/build/thunderbird-48f960ae7db5/obj-mingw/dist/include/msgCore.h:15:0,
20:38.35                  from /var/tmp/build/thunderbird-48f960ae7db5/mailnews/base/search/src/nsMsgImapSearch.cpp:5:
20:38.35 /var/tmp/build/thunderbird-48f960ae7db5/obj-mingw/dist/include/nsCRTGlue.h:101:6: note: candidate: bool NS_IsAscii(char16_t)
20:38.35  bool NS_IsAscii(char16_t aChar);
20:38.35       ^
20:38.35 /var/tmp/build/thunderbird-48f960ae7db5/obj-mingw/dist/include/nsCRTGlue.h:102:6: note: candidate: bool NS_IsAscii(const char16_t*)
20:38.35  bool NS_IsAscii(const char16_t* aString);
20:38.35       ^
20:38.35 /var/tmp/build/thunderbird-48f960ae7db5/mailnews/base/search/src/nsMsgImapSearch.cpp:131:48: error: operands to ?: have different types 'char16ptr_t' and 'const char16_t*'
20:38.35    char *csname = GetImapCharsetParam(asciiOnly ? usAsciiCharSet.get() : destCharset);
}}}

I have attached a patch (based on changes from #928351) which fixes this issue.
Assignee

Comment 2

Last year
Posted file typemistach
error log
Assignee

Comment 3

Last year
(In reply to Sukhbir Singh from comment #0)
> User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101
> Firefox/58.0
> Build ID: 20180219150722
> 
> Steps to reproduce:
> 
> On cross-compiling comm-esr52 (THUNDERBIRD_52_6_0_RELEASE tag) for Windows
> on Linux using mingw-w64, the . Please see the attached log for details but
> a brief summary is:

Seems like I missed a sentence here. 

> On cross-compiling comm-esr52 (THUNDERBIRD_52_6_0_RELEASE tag) for Windows on Linux using mingw-w64, the build fails due to a type mismatch  in nsMsgImapSearch.cpp.
Component: Build Config → Search
Product: Thunderbird → MailNews Core
Version: 52 Branch → 52
Comment on attachment 8955525 [details] [diff] [review]
0001-Bug-1442648-Fix-mismatched-types-in-mingw-w64-builds.patch

Jorg, I'm not sure who the best person to review this is. Can you redirect as appropriate?
Attachment #8955525 - Flags: review?(jorgk)

Comment 5

Last year
Comment on attachment 8955525 [details] [diff] [review]
0001-Bug-1442648-Fix-mismatched-types-in-mingw-w64-builds.patch

I can take this review (being a mailnews peer).

Usually contributors provide patches that apply to trunk. Looking at the patch, there are many trailing spaces which I have removed from the code base in the last few months, so I'm pretty sure this patch won't apply to trunk.

The first hunk is OK, for the second hunk I have this comment:
Line 121:   nsAutoString usAsciiCharSet(NS_LITERAL_STRING("us-ascii"));
Line 123:   char *csname = GetImapCharsetParam(asciiOnly ? usAsciiCharSet.get() : destCharset);
Line 128:     asciiOnly ?  usAsciiCharSet.get(): destCharset,
Line 129:     asciiOnly ?  usAsciiCharSet.get(): destCharset, false);

Looking at the declaration and use of usAsciiCharSet, it is an nsAutoString for no good reason at all. So can we do something like:
char16_t* usAsciiCharSet = u"us-ascii" and be done with this (of course removing all the .get()s)?
Attachment #8955525 - Flags: review?(jorgk) → review-
Assignee

Comment 6

Last year
(In reply to Jorg K (GMT+1) from comment #5)
> Comment on attachment 8955525 [details] [diff] [review]
> 0001-Bug-1442648-Fix-mismatched-types-in-mingw-w64-builds.patch
> 
> I can take this review (being a mailnews peer).

Thanks for the review, Jorg.

> Usually contributors provide patches that apply to trunk. Looking at the
> patch, there are many trailing spaces which I have removed from the code
> base in the last few months, so I'm pretty sure this patch won't apply to
> trunk.

Ah I see. I thought I should do it with the tag I am working; I have fixed it so that it applies to trunk.

> The first hunk is OK, for the second hunk I have this comment:
> Line 121:   nsAutoString usAsciiCharSet(NS_LITERAL_STRING("us-ascii"));
> Line 123:   char *csname = GetImapCharsetParam(asciiOnly ?
> usAsciiCharSet.get() : destCharset);
> Line 128:     asciiOnly ?  usAsciiCharSet.get(): destCharset,
> Line 129:     asciiOnly ?  usAsciiCharSet.get(): destCharset, false);
> 
> Looking at the declaration and use of usAsciiCharSet, it is an nsAutoString
> for no good reason at all. So can we do something like:
> char16_t* usAsciiCharSet = u"us-ascii" and be done with this (of course
> removing all the .get()s)?

You are correct, it's better if this is a const char16_t* instead. I have updated the patch.
Assignee

Comment 7

Last year
Attachment #8955525 - Attachment is obsolete: true

Comment 8

Last year
Hmm, as I said, trunk does not have the trailing spaces any more that are in your patch. I've rebased it, this patch now applies.
Attachment #8955568 - Attachment is obsolete: true
Attachment #8955612 - Flags: review+

Updated

Last year
Assignee: nobody → mozbugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: checkin-needed

Comment 9

Last year
Fixed more white-space issues.
Attachment #8955612 - Attachment is obsolete: true
Attachment #8955614 - Flags: review+

Comment 10

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d0eae7f808b9
Fix mismatched types in mingw-w64 builds (const char16_t*/char16ptr_t). r=jorgk
Status: ASSIGNED → RESOLVED
Closed: Last year
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

Last year
Target Milestone: --- → Thunderbird 60.0
Assignee

Comment 11

Last year
Thanks for the merge Jorg. (Sorry about the whitespace issues, it was my editor on a remote machine missing the .vimrc :)

Comment 12

Last year
Interesting the patch could drop some .get()s. Was the code wrong in the first place, but worked on other compilers due to some automatic casting?
Well, there is some magic going on with char16_t*/char16ptr_t:
https://searchfox.org/mozilla-central/rev/908e3662388a96673a0fc00b84c47f832a5bea7c/mfbt/Char16.h#35

There are other static_cast<const char16_t*> casts in the code base, for example here, so I think we did the right thing:
https://searchfox.org/mozilla-central/rev/908e3662388a96673a0fc00b84c47f832a5bea7c/xpcom/string/nsTSubstring.h#219

I'm just surprised that this should be the only compiler error in mingw-w64 builds.
Assignee

Comment 14

Last year
(In reply to Jorg K (GMT+1) from comment #13)
> Well, there is some magic going on with char16_t*/char16ptr_t:
> https://searchfox.org/mozilla-central/rev/
> 908e3662388a96673a0fc00b84c47f832a5bea7c/mfbt/Char16.h#35

Yes, this is from bug 928351.

> There are other static_cast<const char16_t*> casts in the code base, for
> example here, so I think we did the right thing:
> https://searchfox.org/mozilla-central/rev/
> 908e3662388a96673a0fc00b84c47f832a5bea7c/xpcom/string/nsTSubstring.h#219
> 
> I'm just surprised that this should be the only compiler error in mingw-w64
> builds.

This is the only type mismatch error that I came across as of now but the build is still not complete due to missing headers in mingw-w64. I opened bug 1442034 but since cross-compiling Thunderbird is not supported that was closed. So now I will try adding those headers (for example, SearchAPI.h as in bug 1442034) to mingw-w64 instead.
You need to log in before you can comment on or make changes to this bug.