1336329 - media/webrtc/ fails to build with --enable-warnings-as-errors on Tier3 platforms

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Jan Beich]

$ echo ac_add_options --enable-warnings-as-errors >>.mozconfig
$ ./mach build
[...]
In file included from media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:31:
In file included from media/webrtc/trunk/webrtc/voice_engine/voice_engine_impl.h:29:
media/webrtc/trunk/webrtc/voice_engine/voe_external_media_impl.h:29:15: error:
      'SetExternalRecordingStatus' overrides a member function but is not marked 'override'
      [-Werror,-Winconsistent-missing-override]
  virtual int SetExternalRecordingStatus(bool enable);
              ^
media/webrtc/trunk/webrtc/voice_engine/include/voe_external_media.h:90:15: note:
      overridden virtual function is here
  virtual int SetExternalRecordingStatus(bool enable) = 0;
              ^
In file included from media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:31:
In file included from media/webrtc/trunk/webrtc/voice_engine/voice_engine_impl.h:29:
media/webrtc/trunk/webrtc/voice_engine/voe_external_media_impl.h:31:15: error:
      'SetExternalPlayoutStatus' overrides a member function but is not marked 'override'
      [-Werror,-Winconsistent-missing-override]
  virtual int SetExternalPlayoutStatus(bool enable);
              ^
media/webrtc/trunk/webrtc/voice_engine/include/voe_external_media.h:93:15: note:
      overridden virtual function is here
  virtual int SetExternalPlayoutStatus(bool enable) = 0;
              ^
In file included from media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:31:
In file included from media/webrtc/trunk/webrtc/voice_engine/voice_engine_impl.h:29:
media/webrtc/trunk/webrtc/voice_engine/voe_external_media_impl.h:33:15: error:
      'ExternalRecordingInsertData' overrides a member function but is not marked 'override'
      [-Werror,-Winconsistent-missing-override]
  virtual int ExternalRecordingInsertData(
              ^
media/webrtc/trunk/webrtc/voice_engine/include/voe_external_media.h:98:15: note:
      overridden virtual function is here
  virtual int ExternalRecordingInsertData(
              ^
In file included from media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:31:
In file included from media/webrtc/trunk/webrtc/voice_engine/voice_engine_impl.h:29:
media/webrtc/trunk/webrtc/voice_engine/voe_external_media_impl.h:40:15: error:
      'ExternalPlayoutData' overrides a member function but is not marked 'override'
      [-Werror,-Winconsistent-missing-override]
  virtual int ExternalPlayoutData(
              ^
media/webrtc/trunk/webrtc/voice_engine/include/voe_external_media.h:107:15: note:
      overridden virtual function is here
  virtual int ExternalPlayoutData(
              ^
In file included from media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:31:
In file included from media/webrtc/trunk/webrtc/voice_engine/voice_engine_impl.h:29:
media/webrtc/trunk/webrtc/voice_engine/voe_external_media_impl.h:47:15: error:
      'ExternalPlayoutGetData' overrides a member function but is not marked 'override'
      [-Werror,-Winconsistent-missing-override]
  virtual int ExternalPlayoutGetData(int16_t speechData10ms[],
              ^
media/webrtc/trunk/webrtc/voice_engine/include/voe_external_media.h:116:15: note:
      overridden virtual function is here
  virtual int ExternalPlayoutGetData(
              ^
5 errors generated.

Comment 1

[Jan Beich]

$ c++ -v
FreeBSD clang version 4.0.0 (branches/release_40 293807) (based on LLVM 4.0.0)
Target: x86_64-unknown-freebsd12.0
Thread model: posix
InstalledDir: /usr/bin
Found CUDA installation: /usr/local/cuda, version 7.5

Comment 2

[Jan Beich]

Attachment: Bug 1336329 - Pass clang value from mozbuild to gyp.

Review commit: https://reviewboard.mozilla.org/r/109406/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/109406/

Comment 3

[Chris Peterson [:cpeterson]]

Comment on attachment 8833178 [details]
Bug 1336329 - Pass clang value from mozbuild to gyp.

Thanks for catching this problem on FreeBSD, Jan. Sorry!

Your fix LGTM but I can't r+ changes in these modules.

Comment 4

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

Comment on attachment 8833178 [details]
Bug 1336329 - Pass clang value from mozbuild to gyp.

https://reviewboard.mozilla.org/r/109406/#review110550

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c1030c5c50b
Pass clang value from mozbuild to gyp. r=jesup

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8c1030c5c50b

Comment 7

[Jan Beich]

Comment on attachment 8833178 [details]
Bug 1336329 - Pass clang value from mozbuild to gyp.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f109f346f539fa3aff43a1b6a224dca7c1573353

FF53 lacks bug 1333686, so media/webrtc/moz.build change should be skipped. "hg transplant" and "git cherry-pick" properly detect already applied chunks but manually using patch(1) may produce rejects which need to be ignored.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1330240 feature
[User impact if declined]: --enable-warnings-as-errors fails to build on FreeBSD as Clang check in GYP files is ignored.
[Is this code covered by automated tests?]: Yes, MOZ_AUTOMATION implies --enable-warnings-as-errors. Clang is covered by Linux ASan, OS X jobs.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Can only break build due to unrecognized -W* flags.
[String changes made/needed]: None

Comment 8

[Gerry Chang [:gchang]]

Comment on attachment 8833178 [details]
Bug 1336329 - Pass clang value from mozbuild to gyp.

The [Feature/Bug causing the regression] in the uplift request is incorrect because bug 1330240 is not in FF53 yet. This bug is still waiting for approval on Beta53. 
We can take it this time in order to make your life easier. 
But, please fill the correct info in the uplift request next time for us. Beta53+

Comment 9

[Jan Beich]

(In reply to Gerry Chang [:gchang] from comment #8)
Without either bug 1330240 or bug 1333686 the patch here is mostly a nop i.e., cannot fix --enable-warnings-as-errors. I just wasn't sure how to express such a dependency unless the following is preferred:

[Feature/Bug causing the regression]: None

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/264bb4d0c084