Build failure after bug 579517

RESOLVED FIXED in Thunderbird 18.0

Status

MailNews Core
Build Config
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gaston, Assigned: gaston)

Tracking

unspecified
Thunderbird 18.0
x86
OpenBSD
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird16 unaffected, thunderbird17 fixed, thunderbird18 fixed, seamonkey2.14 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

5 years ago
Similar to but 785738, c-c now fails on openbsd/clang due to prtime being different from int64_t, and probably others. 

/var/buildslave-mozilla/comm-central-amd64/build/mailnews/base/src/nsMsgPurgeService.cpp:199:17: error: no matching function for call to 'PR_ParseTimeString'
                PR_ParseTimeString(curFolderLastPurgeTimeString.get(), false, &theTime);
                ^~~~~~~~~~~~~~~~~~
../../../mozilla/dist/include/prtime.h:244:20: note: candidate function not viable: no known conversion from 'int64_t *' (aka 'long long *') to 'PRTime *' (aka 'long *') for 3rd argument; 
NSPR_API(PRStatus) PR_ParseTimeString (
                   ^
/var/buildslave-mozilla/comm-central-amd64/build/mailnews/base/src/nsMsgPurgeService.cpp:290:11: error: no matching function for call to 'PR_ParseTimeString'
          PR_ParseTimeString(curJunkFolderLastPurgeTimeString.get(), false, &theTime);
          ^~~~~~~~~~~~~~~~~~
../../../mozilla/dist/include/prtime.h:244:20: note: candidate function not viable: no known conversion from 'int64_t *' (aka 'long long *') to 'PRTime *' (aka 'long *') for 3rd argument; 
NSPR_API(PRStatus) PR_ParseTimeString (
                   ^
2 errors generated.

Working on a patch...
(Assignee)

Updated

5 years ago
Assignee: nobody → landry
Blocks: 579517
Depends on: 634793
(Assignee)

Comment 1

5 years ago
Created attachment 658384 [details] [diff] [review]
Fix build on OpenBSD after bug 579517

Dunno if i should ask a c-c peer .. this diff fixes the build for me on openbsd/clang
Attachment #658384 - Flags: review?(ehsan)
(Assignee)

Comment 2

5 years ago
/home/landry/src/comm-central/mailnews/imap/src/nsAutoSyncManager.cpp:108:20: error: cannot initialize a parameter of type 'PRTime *' (aka 'long *') with an rvalue of type 'int64_t *' (aka 'long long *')

/home/landry/src/comm-central/mailnews/local/src/nsMsgBrkMBoxStore.cpp:177:38: error: cannot initialize a parameter of type 'PRTime *' (aka 'long *') with an rvalue of type 'int64_t *' (aka 'long long *')
/home/landry/src/comm-central/mailnews/local/src/nsMsgMaildirStore.cpp:613:11: error: call to member function 'AppendInt' is ambiguous
newName.AppendInt(PR_Now());

Were the corresponding failure messages.
(Assignee)

Comment 3

5 years ago
comm-aurora with gcc 4.2 is affected too, the same fix will probably do the trick :

/usr/ports/pobj/comm-aurora/mailnews/base/src/nsMsgPurgeService.cpp: In member function 'nsresult nsMsgPurgeService::PerformPurge()':
/usr/ports/pobj/comm-aurora/mailnews/base/src/nsMsgPurgeService.cpp:199: error: invalid conversion from 'int64_t*' to 'PRTime*'
/usr/ports/pobj/comm-aurora/mailnews/base/src/nsMsgPurgeService.cpp:290: error: invalid conversion from 'int64_t*' to 'PRTime*'
status-thunderbird16: --- → unaffected
status-thunderbird17: --- → affected
status-thunderbird18: --- → affected
(Assignee)

Comment 4

5 years ago
Created attachment 658463 [details] [diff] [review]
Fix build on OpenBSD after bug 579517 (c-a patch)

[Approval Request Comment]
Regression caused by (bug #): 579517
User impact if declined: c-a failure on OpenBSD/gcc 4.2.1
Testing completed (on c-c, etc.): in progress
Risk to taking this patch (and alternatives if risky): none, wont affect tier1 platforms for which those types are the same.
Attachment #658463 - Flags: review?(ehsan)
Attachment #658463 - Flags: approval-comm-aurora?
Comment on attachment 658384 [details] [diff] [review]
Fix build on OpenBSD after bug 579517

Review of attachment 658384 [details] [diff] [review]:
-----------------------------------------------------------------

I'm technically not a peer of anything in c-c, but I know some of them.  ;-)
Attachment #658384 - Flags: review?(ehsan) → review+
Comment on attachment 658463 [details] [diff] [review]
Fix build on OpenBSD after bug 579517 (c-a patch)

Review of attachment 658463 [details] [diff] [review]:
-----------------------------------------------------------------

(You don't need to ask for a separate review on branch patches if they're practically the same as trunk patches...)
Attachment #658463 - Flags: review?(ehsan) → review+
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/comm-central/rev/66edec66017c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 18.0
(Assignee)

Comment 8

5 years ago
it seems i've missed one, dunno how it wasnt catched.. or the PRInt64 was brought by new code ?

/var/buildslave-mozilla/comm-central-amd64/build/mailnews/imap/src/nsImapMailFolder.cpp:8261:26: error: cannot initialize a parameter of type 'int64_t *' (aka 'long long *') with an rvalue of type 'PRInt64 *' (aka 'long *')
          seekable->Tell(&curOfflineStorePos);
                         ^~~~~~~~~~~~~~~~~~~
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 658384 [details] [diff] [review]
Fix build on OpenBSD after bug 579517

Review of attachment 658384 [details] [diff] [review]:
-----------------------------------------------------------------

So I know this already landed, but I'm not sure why it is ONLY failing for you, when it works fine on all the official mozilla machines.

I don't have the best comprehension of this C++ magic, so I want Neil [or another real peer of this code] to give it a proper look before *anyone* approves it on the branch

::: mailnews/local/src/nsMsgMaildirStore.cpp
@@ +609,5 @@
>    }
>  
>    // generate new file name
>    nsAutoCString newName;
> +  newName.AppendInt(static_cast<int64_t>(PR_Now()));

In addition to the above this worries me, PR_Now returns PRTime, which is typedefed to PRInt64 which is *not* always an int64_t compat type, where it can be long long, among other things (see: http://mxr.mozilla.org/comm-central/source/mozilla/nsprpub/pr/include/prtypes.h#340 )

So this static_cast feels dangerous if its needed, since it circumvents the normal checks and bounds here
Attachment #658384 - Flags: review?(neil)
Attachment #658384 - Flags: feedback-
(Assignee)

Comment 10

5 years ago
(In reply to Justin Wood (:Callek) from comment #9)
> Comment on attachment 658384 [details] [diff] [review]
> Fix build on OpenBSD after bug 579517
> 
> Review of attachment 658384 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So I know this already landed, but I'm not sure why it is ONLY failing for
> you, when it works fine on all the official mozilla machines.

Because on OpenBSD PRInt64 != int64_t, and on all official mozilla machines the types are equivalent and thus don't produce those failures. See bug 785738 and bug 634793 for the root cause of all my pain.

> In addition to the above this worries me, PR_Now returns PRTime, which is
> typedefed to PRInt64 which is *not* always an int64_t compat type, where it
> can be long long, among other things (see:
> http://mxr.mozilla.org/comm-central/source/mozilla/nsprpub/pr/include/
> prtypes.h#340 )
> 
> So this static_cast feels dangerous if its needed, since it circumvents the
> normal checks and bounds here

The same 'fixes' were applied on m-c.
(Assignee)

Comment 11

5 years ago
Created attachment 659464 [details] [diff] [review]
fix another print64/int64_t mixup

That one fixes the aforementioned error. c-c builds again for me on openbsd. Pending discussion on the other issue....
Attachment #659464 - Flags: review?(bugspam.Callek)
Comment on attachment 659464 [details] [diff] [review]
fix another print64/int64_t mixup

I am not an expert so I still want Neil to look at this bug but this command anyway until he does
Attachment #659464 - Flags: review?(bugspam.Callek) → review+
(In reply to Justin Wood (:Callek) from comment #12)
> Comment on attachment 659464 [details] [diff] [review]
> fix another print64/int64_t mixup
> 
> I am not an expert so I still want Neil to look at this bug but this command
> anyway until he does

Bah - THIS is why I should never try to review from mobile. Let me repeat what I said, only clearer and correctly:

I'm no expert with Cpp unfortunately, so I'd still like to keep the previous review request on Neil open to help check my sanity in my above comments. However given the other patch landed and this unblocks you, lets land this self-contained change anyway.
(Assignee)

Comment 14

5 years ago
 https://hg.mozilla.org/comm-central/rev/e3c08e7b8ebc

i of course need to recheck aurora to see if this chunk needs to be added to the patch waiting approval...
Comment on attachment 658384 [details] [diff] [review]
Fix build on OpenBSD after bug 579517

>-  newName.AppendInt(PR_Now());
>+  newName.AppendInt(static_cast<int64_t>(PR_Now()));
Callek: AppendInt takes an int32_t, uint32_t, int64_t or uint64_t. If PR_Now() doesn't return one of those, you'll get an ambiguous cast, so you have to cast it manually.
Attachment #658384 - Flags: review?(neil) → review+
(Assignee)

Comment 16

5 years ago
(In reply to Landry Breuil (:gaston) from comment #14)
>  https://hg.mozilla.org/comm-central/rev/e3c08e7b8ebc
> 
> i of course need to recheck aurora to see if this chunk needs to be added to
> the patch waiting approval...

That pruint64 is not present in comm-aurora, so my approval-comm-aurora request stands for attachment 658463 [details] [diff] [review].
(Assignee)

Comment 17

5 years ago
Callek, ping comm-aurora a? would be a pity to miss next uplift.
(In reply to Landry Breuil (:gaston) from comment #17)
> Callek, ping comm-aurora a? would be a pity to miss next uplift.

I can't approve mailnews/ patches -- standard8: ping
So is this bug fixed now? Also, exactly which patches need approval?
(Assignee)

Comment 20

5 years ago
att 658463 needs approval, the bug is fixed in c-c but pending in c-a.
(Assignee)

Updated

5 years ago
status-thunderbird18: affected → fixed
Ok, thanks, if its fixed in comm-central, then we normally mark the bug as fixed - branches are tracked by the status field.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Attachment #658463 - Flags: approval-comm-aurora? → approval-comm-aurora+
https://hg.mozilla.org/releases/comm-aurora/rev/c49b8f67479f
status-thunderbird17: affected → fixed
(Assignee)

Comment 23

5 years ago
Comment on attachment 659464 [details] [diff] [review]
fix another print64/int64_t mixup

After a recheck, it seems this chunk is needed too on comm-aurora. In the hope i can slip it in before the next uplift...

[Approval Request Comment]
Regression caused by (bug #): 579517
User impact if declined: c-a failure on OpenBSD/gcc 4.2.1
Testing completed (on c-c, etc.): done, fixes build
Risk to taking this patch (and alternatives if risky): none, wont affect tier1 platforms for which those types are the same.
Attachment #659464 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
status-thunderbird17: fixed → affected

Updated

5 years ago
Attachment #659464 - Flags: approval-mozilla-aurora? → approval-comm-aurora?
Comment on attachment 659464 [details] [diff] [review]
fix another print64/int64_t mixup

Review of attachment 659464 [details] [diff] [review]:
-----------------------------------------------------------------

I can't officially set this [yet - filed Bug 798996 for that], but I'm giving it a+=me
http://hg.mozilla.org/releases/comm-aurora/rev/e0369f027737
status-seamonkey2.14: --- → fixed
status-thunderbird17: affected → fixed
Attachment #659464 - Flags: approval-comm-aurora? → approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.