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

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[ojab]

Attachment: Build error log

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.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Specify where to find StandardInteger.h for srtp

Please try this patch and report if it solves the problem with gcc 4.7

Comment 2

[ojab]

Still the same error with the patch applied.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Can you repost the log snippet with this patch applied?  Are you sure the same file is failing?

Comment 4

[ojab]

Attachment: MediaPipeline.cpp build failure

Ouch, my fault. Error is during MediaPipeline.cpp build this time.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Specify where to find StandardInteger.h for srtp

Please try again with this - that's all the includes of srtp.h in the tree

Comment 6

[Eric Rescorla (:ekr)]

Comment on attachment 668909 [details] [diff] [review]
Specify where to find StandardInteger.h for srtp

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

lgtm

Comment 7

[ojab]

Attachment: MediaPipeline.cpp build failure with patch (Attachment #668909) applied

MediaPipeline.cpp build isn't fixed by the second patch.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

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?

Comment 9

[ojab]

Nothing has changed, the same error.

Comment 10

[ojab]

Attachment: Patch

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?

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 12

[(no longer active)]

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.

Comment 13

[ojab]

Attachment: 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?

Comment 14

[(no longer active)]

(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 15

[Mike Hommey [:glandium]]

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.

Comment 16

[Mike Hommey [:glandium]]

Attachment: Define INTEGER_TYPES_H and others for srtp.h

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.

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

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)

Comment 18

[Mike Hommey [:glandium]]

Attachment: Define INTEGER_TYPES_H and others for srtp.h

Updated patch

Comment 19

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbaae9a05b62

Comment 20

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/cbaae9a05b62

Comment 21

[Mike Hommey [:glandium]]

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

Comment 22

[Alex Keybl [:akeybl]]

(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?

Comment 23

[Mike Hommey [:glandium]]

(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.

Comment 24

[bhavana bajaj [:bajaj]]

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

Comment 25

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d4bd5dcfbd16