Closed Bug 798926 Opened 7 years ago Closed 7 years ago

Firefox build with --with-system-cairo is broken after Bug 792188 (error: conflicting declaration ‘typedef double uint64_t’)

Categories

(Core :: WebRTC: Signaling, defect)

Other Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: ojab, Assigned: glandium)

References

Details

(Whiteboard: [WebRTC] [blocking-webrtc-] [qa-])

Attachments

(4 files, 5 obsolete files)

Attached file Build error log (obsolete) —
I cannot properly bisect this error because preceding revisions is broken with another errors, so I'm not sure that it is broken during Bug 792188 commits, but looks like so. 
mozilla-inbound tree:
r109552:f1b524d51e6b Builds fine

109553-1095560 fails with
In file included from /sources/mozilla-inbound/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp:49:0:
/sources/mozilla-inbound/media/webrtc/signaling//./src/mediapipeline/MediaPipeline.h:10:21: fatal error: sigslot.h: No such file or directory

109561-109563 fails with
In file included from /sources/mozilla-inbound/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp:52:0:
/sources/mozilla-inbound/media/webrtc/signaling//./src/peerconnection/PeerConnectionImpl.h:15:29: fatal error: IPeerConnection.h: No such file or directory
compilation terminated.

r109564:750fa0811f41 and up fails during /sources/mozilla-inbound/media/webrtc/signaling/src/mediapipeline/SrtpFlow.cpp compilation with:
/sources/mozilla-inbound/media/webrtc/signaling//../../../netwerk/srtp/src/crypto/include/integers.h:111:16: error: conflicting declaration ‘typedef double uint64_t’
/usr/include/stdint.h:56:27: error: ‘uint64_t’ has a previous declaration as ‘typedef long unsigned int uint64_t’

Full error log can be found in the attached file, because it's rather long.
Blocks: 792188
Please try this patch and report if it solves the problem with gcc 4.7
Assignee: nobody → rjesup
Status: UNCONFIRMED → NEW
Ever confirmed: true
Still the same error with the patch applied.
Can you repost the log snippet with this patch applied?  Are you sure the same file is failing?
Attached file MediaPipeline.cpp build failure (obsolete) —
Ouch, my fault. Error is during MediaPipeline.cpp build this time.
Attachment #668900 - Attachment is obsolete: true
Please try again with this - that's all the includes of srtp.h in the tree
Attachment #668901 - Attachment is obsolete: true
Attachment #668901 - Flags: review?(ekr)
Comment on attachment 668909 [details] [diff] [review]
Specify where to find StandardInteger.h for srtp

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

lgtm
Attachment #668909 - Flags: review?(ekr) → review+
MediaPipeline.cpp build isn't fixed by the second patch.
Attachment #668906 - Attachment is obsolete: true
Comment on attachment 668910 [details]
MediaPipeline.cpp build failure with patch (Attachment #668909 [details] [diff]) applied

That's very odd.  Could you try changing the #define INTEGER_TYPES_H line to #include "mozilla/StandardInteger.h" in MediaPipeline.cpp?
Nothing has changed, the same error.
Whiteboard: [blocking-webrtc-]
Whiteboard: [blocking-webrtc-] → [WebRTC] [blocking-webrtc-]
Attached patch Patch (obsolete) — Splinter Review
In mozilla tree HAVE_UINT64_T is defined in configure.in, but only in case of mozilla was built with bundled cairo, so it is unrelated to GCC version. I'm building --with-system-cairo, which leads to build failure.

I can build firefox without errors using attached patch. Is it a proper solution?
Attachment #670780 - Flags: feedback?
Comment on attachment 670780 [details] [diff] [review]
Patch

I'd be wary of blanket #defining HAVE_UINT64_T/HAVE_UINT32_T/etc - StandardInteger.h only helps if you include it, and imported code won't generally. Imported code may key off HAVE_BLAH however and netwerk/srtp/src for example was broken by the HAVE_UINT64_T define for cairo.

Adding bsmedberg and ehsan for feedback
Attachment #670780 - Flags: feedback?(ehsan)
Attachment #670780 - Flags: feedback?(benjamin)
Summary: Firefox build with gcc-4.7.2 is broken after Bug 792188 (error: conflicting declaration ‘typedef double uint64_t’) → Firefox build with --with-system-cairo is broken after Bug 792188 (error: conflicting declaration ‘typedef double uint64_t’)
Depends on: 722975
Comment on attachment 670780 [details] [diff] [review]
Patch

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

Yes, this is very bad.  In fact, what we should really do here is to take out HAVE_UINT64_T there and just fix whatever is depending on this brokenness without knowing.

FWIW, I have digged through the history to see where this code came from and IIRC it came from the first time that we added building with system cairo, presumably as a bad attempt to fix a problem.
Attachment #670780 - Flags: review-
Attachment #670780 - Flags: feedback?(ehsan)
Attachment #670780 - Flags: feedback?(benjamin)
Attachment #670780 - Flags: feedback?
Attached patch Patch try2Splinter Review
…and I don't really understand what is the correct solution ._.
If there is uint64_t definition in the standard headers — can we suppose that imported code will found it and define HAVE_UINT64_T (see the attached patch)? If so — what should be done if uint64_t is not defined? Should we also check for uint32_t, uint16_t and uint8_t?
Attachment #670780 - Attachment is obsolete: true
(In reply to comment #13)
> Created attachment 671121 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=671121&action=edit
> Patch try2
> 
> …and I don't really understand what is the correct solution ._.
> If there is uint64_t definition in the standard headers — can we suppose that
> imported code will found it and define HAVE_UINT64_T (see the attached patch)?
> If so — what should be done if uint64_t is not defined? Should we also check
> for uint32_t, uint16_t and uint8_t?

We can only safely get those types if mozilla/StandardInteger.h is #included.  Having HAVE_UINT64_TT and the like just make imported code to assume wrongly that this type is always available.
Comment on attachment 671121 [details] [diff] [review]
Patch try2

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

::: configure.in
@@ +2697,5 @@
> +AC_MSG_CHECKING(for uint64_t)
> +AC_CACHE_VAL(ac_cv_uint64_t,
> + [AC_TRY_COMPILE([#include <stdint.h>
> +                  #include <inttypes.h>
> +                  #include <sys/int_types.h>],

I doubt there is any system where including these three actually works.

@@ -7955,5 @@
>  AC_SUBST(MOZ_PIXMAN_CFLAGS)
>  AC_SUBST(MOZ_PIXMAN_LIBS)
>  
> -# Check for headers defining standard int types.
> -MOZ_CHECK_HEADERS(stdint.h inttypes.h sys/int_types.h)

Please don't remove that, we have code relying on HAVE_STDINT_H, which results from this test.

@@ -7963,5 @@
>      AC_DEFINE(MOZ_TREE_CAIRO)
>  
> -    # For now we assume that we will have a uint64_t available through
> -    # one of the above headers or mozstdint.h.
> -    AC_DEFINE(HAVE_UINT64_T)

I'm afraid removing this basically means breaking windows builds.
Attachment #671121 - Flags: review-
This does the same thing as http://dxr.mozilla.org/mozilla-central/netwerk/srtp/src/Makefile.in#l68

I do agree we need to get rid of the unconditional AC_DEFINE(HAVE_UINT64_T) in configure.in for in-tree cairo builds, but that's kind of orthogonal to the issue here.
Attachment #672228 - Flags: review?(rjesup)
Assignee: rjesup → mh+mozilla
Comment on attachment 672228 [details] [diff] [review]
Define INTEGER_TYPES_H and others for srtp.h

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

Remove HAVE_STRING_H (it should go from srtp/src/Makefile.in too)
Attachment #672228 - Flags: review?(rjesup) → review+
Attachment #672228 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/cbaae9a05b62
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment on attachment 672255 [details] [diff] [review]
Define INTEGER_TYPES_H and others for srtp.h

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Regression from bug 792188
User impact if declined: Breaks build in some setups
Testing completed (on m-c, etc.): Landed on m-c
Risk to taking this patch (and alternatives if risky): Fairly low risk. The netwert/srtp part is a noop because HAVE_STRING_H is never used, and the signaling part "only" tricks srtp.h, as included from signaling code, into including mozilla/StandardInteger.h and using stdint types, which is what is done in netwerk/srtp for the same header.
String or UUID changes made by this patch: None
Attachment #672255 - Flags: approval-mozilla-aurora?
(In reply to Mike Hommey [:glandium] from comment #21)
> Risk to taking this patch (and alternatives if risky): Fairly low risk. The
> netwert/srtp part is a noop because HAVE_STRING_H is never used, and the
> signaling part "only" tricks srtp.h, as included from signaling code, into
> including mozilla/StandardInteger.h and using stdint types, which is what is
> done in netwerk/srtp for the same header.

Would any regressions show up as a build issue, or could they manifest as a user experience regression?
(In reply to Alex Keybl [:akeybl] from comment #22)
> Would any regressions show up as a build issue, or could they manifest as a
> user experience regression?

That would show up as build issue if at all.
Whiteboard: [WebRTC] [blocking-webrtc-] → [WebRTC] [blocking-webrtc-] [qa-]
Comment on attachment 672255 [details] [diff] [review]
Define INTEGER_TYPES_H and others for srtp.h

Approving the low risk patch based on comment 23
Attachment #672255 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.