Closed Bug 814693 Opened 7 years ago Closed 7 years ago

Build failure on Debian powerpc

Categories

(Core :: WebRTC, defect, P3)

17 Branch
Other
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: michel, Assigned: glandium)

References

Details

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

Attachments

(9 files, 1 obsolete file)

Fix
3.51 KB, patch
jesup
: review-
Details | Diff | Splinter Review
658 bytes, patch
Details | Diff | Splinter Review
2.13 KB, patch
Details | Diff | Splinter Review
7.91 KB, patch
jesup
: review+
Details | Diff | Splinter Review
1.12 KB, patch
jesup
: review+
Details | Diff | Splinter Review
1.06 KB, patch
Details | Diff | Splinter Review
2.05 KB, patch
Details | Diff | Splinter Review
1.68 KB, patch
Details | Diff | Splinter Review
639 bytes, patch
jesup
: review+
ted
: review+
gaston
: checkin+
Details | Diff | Splinter Review
Attached patch FixSplinter Review
User Agent: Mozilla/5.0 (X11; Linux ppc; rv:17.0) Gecko/17.0 Firefox/17.0 Iceweasel/17.0
Build ID: 20121122175550



Actual results:

Firefox fails to build on Debian powerpc, see https://buildd.debian.org/status/fetch.php?pkg=iceweasel&arch=powerpc&ver=17.0-1&stamp=1353519787


Expected results:

The attached patch allows Firefox to build and run on this platform.
Attachment #684694 - Flags: review?(rjesup)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 684694 [details] [diff] [review]
Fix

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

::: iceweasel-17.0.orig/media/webrtc/trunk/src/modules/audio_coding/codecs/pcm16b/pcm16b.gypi
@@ +12,5 @@
>        'target_name': 'PCM16B',
>        'type': '<(library)',
> +      'dependencies': [
> +        '<(webrtc_root)/common_audio/common_audio.gyp:signal_processing',
> +      ],

What error requires this part?
Comment on attachment 684694 [details] [diff] [review]
Fix

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

SSE2 tests need to be aligned.

::: iceweasel-17.0.orig/media/webrtc/shared_libs.mk
@@ +41,5 @@
>    $(call EXPAND_LIBNAME_PATH,yuv,$(DEPTH)/media/webrtc/trunk/third_party/libyuv/libyuv_libyuv) \
>    $(call EXPAND_LIBNAME_PATH,webrtc_jpeg,$(DEPTH)/media/webrtc/trunk/src/common_video/common_video_webrtc_jpeg) \
>    $(NULL)
> +
> +ifneq (,$(findstring 86,$(CPU_ARCH)))

Is this really the right test for sse2 support?  I wouldn't think so.  (It could be we don't have a good test, in which case this is better than nothing.)

Actually, the test should match the test used to build it in video_processing.gyp (or that test should also be changed):  it looks for a target_arch of ia32 or x64.  Note that target_arch != CPU_ARCH; see configure.in.
Attachment #684694 - Flags: review?(rjesup) → review-
(In reply to Randell Jesup [:jesup] from comment #2)
> 
> Is this really the right test for sse2 support?  I wouldn't think so.  (It
> could be we don't have a good test, in which case this is better than
> nothing.)

I don't know what the right test is in the place(s) that include media/webrtc/shared_libs.mk . I'll gladly leave that to someone who's familiar with this. :) But it seems clear that the build is currently broken for anything that can't handle SSE2.
(In reply to Mike Hommey [:glandium] from comment #1)
> What error requires this part?

make[1]: Entering directory `/home/daenzer/build/iceweasel-17.0/build-xulrunner/media/webrtc/trunk/src/modules/modules_PCM16B'
gcc -o audio_coding/codecs/pcm16b/pcm16b.o -c -fvisibility=hidden -DMOZ_GLUE_IN_PROGRAM -D_FILE_OFFSET_BITS=64 -DCHROMIUM_BUILD -DUSE_NSS=1 -DTOOLKIT_USES_GTK=1 -DGTK_DISABLE_SINGLE_INCLUDES=1 -D_ISOC99_SOURCE=1 -DENABLE_REMOTING=1 -DENABLE_P2P_APIS=1 -DENABLE_CONFIGURATION_POLICY -DENABLE_INPUT_SPEECH -DENABLE_NOTIFICATIONS -DENABLE_GPU=1 -DENABLE_EGLIMAGE=1 -DUSE_SKIA=1 -DENABLE_REGISTER_PROTOCOL_HANDLER=1 -DENABLE_WEB_INTENTS=1 -DENABLE_PLUGIN_INSTALLATION=1 -DWEBRTC_LINUX -DWEBRTC_THREAD_RR -D__STDC_FORMAT_MACROS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -I. -I/home/daenzer/build/iceweasel-17.0/media/webrtc/trunk/src/modules/.. -I/home/daenzer/build/iceweasel-17.0/media/webrtc/trunk/src/modules/../.. -I/home/daenzer/build/iceweasel-17.0/media/webrtc/trunk/src/modules/audio_coding/codecs/pcm16b/include  -fPIC -D_FORTIFY_SOURCE=2 -Wall -Wpointer-arith -Wdeclaration-after-statement -Werror=return-type -Wtype-limits -Wempty-body -Wno-unused -Wno-overlength-strings -Wcast-align -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security -fno-strict-aliasing -ffunction-sections -fdata-sections -pthread -pipe -DNDEBUG -DTRIMMED -g -Os -freorder-blocks  -fomit-frame-pointer    -D_FORTIFY_SOURCE=2 -include ../../../../../../mozilla-config.h -DMOZILLA_CLIENT -MD -MF .deps/pcm16b.o.pp /home/daenzer/build/iceweasel-17.0/media/webrtc/trunk/src/modules/audio_coding/codecs/pcm16b/pcm16b.c
/home/daenzer/build/iceweasel-17.0/media/webrtc/trunk/src/modules/audio_coding/codecs/pcm16b/pcm16b.c:19:39: fatal error: signal_processing_library.h: No such file or directory
compilation terminated.
make[1]: *** [audio_coding/codecs/pcm16b/pcm16b.o] Error 1


Not sure why this doesn't happen on x86...
(In reply to Michel Dänzer from comment #4)
> Not sure why this doesn't happen on x86...

Ah:

#ifdef WEBRTC_BIG_ENDIAN
#include "signal_processing_library.h"
#endif
(In reply to Michel Dänzer from comment #3)
> (In reply to Randell Jesup [:jesup] from comment #2)
> > 
> > Is this really the right test for sse2 support?  I wouldn't think so.  (It
> > could be we don't have a good test, in which case this is better than
> > nothing.)
> 
> I don't know what the right test is in the place(s) that include
> media/webrtc/shared_libs.mk . I'll gladly leave that to someone who's
> familiar with this. :) But it seems clear that the build is currently broken
> for anything that can't handle SSE2.

First cut (and a reasonable one) would be to make it match the code that builds or doesn't build the SSE2 code - see my original review.  Do that and I'll r+ it.
(In reply to Randell Jesup [:jesup] from comment #6)
> First cut (and a reasonable one) would be to make it match the code that
> builds or doesn't build the SSE2 code - see my original review.

The thing is, the SSE2 code build is defined in media/webrtc/trunk/src/modules/video_processing/main/source/video_processing.gypi, but shared_libs.mk is included from layout/media/Makefile(.in). I have no idea if/how variable from the former are available in the latter.
(In reply to Michel Dänzer from comment #7)
> The thing is, the SSE2 code build is defined in
> media/webrtc/trunk/src/modules/video_processing/main/source/video_processing.
> gypi, but shared_libs.mk is included from layout/media/Makefile(.in). I have
> no idea if/how variable from the former are available in the latter.

The normal test we use elsewhere in the tree is
ifneq (,$(INTEL_ARCHITECTURE))
(In reply to Timothy B. Terriberry (:derf) from comment #9)
> The normal test we use elsewhere in the tree is
> ifneq (,$(INTEL_ARCHITECTURE))

Thanks! Hadn't seen that.
Priority: -- → P3
Whiteboard: [WebRTC] [blocking-webrtc-][blocking-gum-]
(In reply to Michel Dänzer from comment #5)
> (In reply to Michel Dänzer from comment #4)
> > Not sure why this doesn't happen on x86...
> 
> Ah:
> 
> #ifdef WEBRTC_BIG_ENDIAN
> #include "signal_processing_library.h"
> #endif

Why does it need that?
Comment on attachment 685102 [details] [diff] [review]
Fix for pcm16b.gypi with test for some big endian architectures.

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

::: iceweasel-17.0.orig/media/webrtc/trunk/src/modules/audio_coding/codecs/pcm16b/pcm16b.gypi
@@ +11,5 @@
>      {
>        'target_name': 'PCM16B',
>        'type': '<(library)',
> +      'conditions': [
> +        ['target_arch=="ppc32" or target_arch=="ppc64" or target_arch=="sparc"',

other big endian architectures: avr32, hppa, m68k, mips (not mipsel), s390 and s390x.
(In reply to Mike Hommey [:glandium] from comment #12)
> iceweasel-17.0.orig/media/webrtc/trunk/src/modules/audio_coding/codecs/
> pcm16b/pcm16b.gypi
> @@ +11,5 @@
> >      {
> >        'target_name': 'PCM16B',
> >        'type': '<(library)',
> > +      'conditions': [
> > +        ['target_arch=="ppc32" or target_arch=="ppc64" or target_arch=="sparc"',
> 
> other big endian architectures: avr32, hppa, m68k, mips (not mipsel), s390
> and s390x.

I'm waiting with bated breath for WebRTC on the s390.  Gotta find whomever ended up with the old campus 390 from college and add some USB ports to it.  ;-)
#define WEBRTC_SPL_MEMCPY_W8(v1, v2, length) \
    memcpy(v1, v2, (length) * sizeof(char))
#define WEBRTC_SPL_MEMCPY_W16(v1, v2, length) \
    memcpy(v1, v2, (length) * sizeof(WebRtc_Word16))

These are the macros pcm16b.c uses from signal_processing_library.h. Pretty ridiculous.
I'm going to test this on Debian.
Assignee: nobody → mh+mozilla
(In reply to Randell Jesup [:jesup] from comment #13)
> I'm waiting with bated breath for WebRTC on the s390.  Gotta find whomever
> ended up with the old campus 390 from college and add some USB ports to it. 
> ;-)

Well, with this patch, it builds. I haven't tested if it actually works.
Attachment #687403 - Flags: review?(rjesup)
Attachment #687362 - Attachment is obsolete: true
Comment on attachment 687403 [details] [diff] [review]
Allow webrtc to build on more architectures

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

::: media/webrtc/trunk/src/modules/audio_coding/codecs/pcm16b/pcm16b.c
@@ +25,5 @@
>                                       WebRtc_Word16 len,
>                                       WebRtc_Word16 *speechOut16b)
>  {
>  #ifdef WEBRTC_BIG_ENDIAN
> +    memcpy(speechOut16b, speechIn16b, len * sizeof(WebRtc_Word16));

I assume there's some reason we can't include signal_processing_library.h here?  Should we fix that instead?

::: media/webrtc/trunk/src/typedefs.h
@@ +119,5 @@
> +#elif defined(__avr32__)
> +#define WEBRTC_ARCH_AVR32 1
> +#define WEBRTC_ARCH_32_BITS 1
> +#define WEBRTC_ARCH_BIG_ENDIAN
> +#define WEBRTC_BIG_ENDIAN

Wow, very comprehensive.
Attachment #687403 - Flags: review?(rjesup) → review+
(In reply to Randell Jesup [:jesup] from comment #17)
> Comment on attachment 687403 [details] [diff] [review]
> Allow webrtc to build on more architectures
> 
> Review of attachment 687403 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/trunk/src/modules/audio_coding/codecs/pcm16b/pcm16b.c
> @@ +25,5 @@
> >                                       WebRtc_Word16 len,
> >                                       WebRtc_Word16 *speechOut16b)
> >  {
> >  #ifdef WEBRTC_BIG_ENDIAN
> > +    memcpy(speechOut16b, speechIn16b, len * sizeof(WebRtc_Word16));
> 
> I assume there's some reason we can't include signal_processing_library.h
> here?  Should we fix that instead?

The alternative is to add a *lot* of conditions in the gyp file (one for each big endian architecture, see comment 12). Using memcpy, which WEBRTC_SPL_MEMCPY_W* is anyways (see comment 14) is way simpler.
Erf, the shared_libs.mk part landed in bug 750869.
https://hg.mozilla.org/mozilla-central/rev/ea3ca2b1bd7f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Thanks for fixing this!
Whiteboard: [WebRTC] [blocking-webrtc-][blocking-gum-] → [WebRTC] [blocking-webrtc-][blocking-gum-][qa-]
I failed to notice there was a __MIPSEL__ block already (I made the original patch for 17, which doesn't have this block). It conflicts with the __mips__ block I added which also handles the big endian variant and the 64-bits variant.
Attachment #689640 - Flags: review?(rjesup)
Attachment #689640 - Flags: review?(rjesup) → review+
Duplicate of this bug: 824757
Duplicate of this bug: 804032
I used some of the above patches but took a different approach and just enabled the signal_processing stuff to build as it appears was intended (which eliminates the issue in comment #18 etc).  With the the attached three patches and some portage eclass support (append-ldflags "-logg -lvorbis") in the ebuild everything seems to build and link fine on ppc, and *should* Just Work (TM) on other big endian platforms.  The library flags could obviously be added to the right place(s) by someone more familiar with the build setup than I am...
That should've been "With the three attached patches" in comment #27.  Sheesh...
When I try to build mozilla-central   (117348:4e18ac9b51e2) that includes this fix on my 32 bit ppc (debian) machine with webrtc I get the following error

make[5]: Leaving directory `/media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/build-output/security/build'
make -C media/webrtc export
make[5]: Entering directory `/media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/build-output/media/webrtc'
make[5]: *** No rule to make target `trunk/Makefile', needed by `export'.  Stop.

Everything builds okay with the attach patch.  Are similar rules needed for the other platforms? I'm not sure why it built for others on ppc but isn't for me.
Attachment #698508 - Flags: review?(rjesup)
Comment on attachment 698508 [details] [diff] [review]
fixes 'no rule to make trunk/Makefile' errors on ppc

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

r+ but you should have a build peer
Attachment #698508 - Flags: review?(ted)
Attachment #698508 - Flags: review?(rjesup)
Attachment #698508 - Flags: review+
Attachment #698508 - Flags: review?(ted) → review+
Flags: in-testsuite-
Comment on attachment 698508 [details] [diff] [review]
fixes 'no rule to make trunk/Makefile' errors on ppc

https://hg.mozilla.org/integration/mozilla-inbound/rev/efd019bee0ff
Attachment #698508 - Flags: checkin? → checkin+
Comment on attachment 698508 [details] [diff] [review]
fixes 'no rule to make trunk/Makefile' errors on ppc

[Approval Request Comment]
User impact if declined: Build failure on Linux/OpenBSD powerpc
Testing completed (on m-c, etc.): It's on aurora, currently, and has been tested on powerpc by various people already.
Risk to taking this patch (and alternatives if risky): NPOTB
String or IDL/UUID changes made by this patch: None
Attachment #698508 - Flags: approval-mozilla-beta?
Comment on attachment 698508 [details] [diff] [review]
fixes 'no rule to make trunk/Makefile' errors on ppc

low risk, well tested patch (NPOTB). Avoid's Build failure on Linux/OpenBSD powerpc, has been verified by a couple of users
Attachment #698508 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This issue (or the part that's in the webrtc.org area - media/webrtc/trunk) needs to be filed as an upstream issue at webrtc.org, and preferably the patch put up for codereview there as well.
No longer blocks: webrtc_upstream_bugs
You need to log in before you can comment on or make changes to this bug.