Bug 145837 - [EFL] Fix the openwebrtc and gst-plugins-openwebrtc build with clang
Summary: [EFL] Fix the openwebrtc and gst-plugins-openwebrtc build with clang
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Csaba Osztrogonác
URL:
Keywords:
Depends on:
Blocks: 145121
  Show dependency treegraph
 
Reported: 2015-06-10 03:22 PDT by Csaba Osztrogonác
Modified: 2015-10-01 08:39 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.53 KB, patch)
2015-06-10 03:48 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2015-06-10 03:22:12 PDT
Building openwebrtc jhbuild module with clang fails:
-----------------------------------------------------

/home/webkit/WebKit/WebKitBuild/DependenciesEFL/Root/include/gstreamer-1.0/gst/gstcompat.h:54:38: warning: equality comparison with extraneous parentheses [-Wparentheses-equality]
  if (((((GstPad*)(pad))->direction) == GST_PAD_SRC))
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~
/home/webkit/WebKit/WebKitBuild/DependenciesEFL/Root/include/gstreamer-1.0/gst/gstcompat.h:54:38: note: remove extraneous parentheses around the comparison to silence this warning
  if (((((GstPad*)(pad))->direction) == GST_PAD_SRC))
      ~                              ^             ~
/home/webkit/WebKit/WebKitBuild/DependenciesEFL/Root/include/gstreamer-1.0/gst/gstcompat.h:54:38: note: use '=' to turn this equality comparison into an assignment
  if (((((GstPad*)(pad))->direction) == GST_PAD_SRC))
                                     ^~
                                     =
1 warning generated.

-----

gstcompat.h:54 -->  if (GST_PAD_IS_SRC (pad))

Removing the extra parentheses would be very ugly here,
we should simply disable parentheses-equality warning.
Comment 1 Csaba Osztrogonác 2015-06-10 03:23:10 PDT
We have same error in gst-plugins-openwebrtc module.
Comment 2 Csaba Osztrogonác 2015-06-10 03:28:38 PDT
It can be fixed easily on EFL, because we download tarballs, which
can be patched by jhbuild. But unfortunately GTK download git repositories,
which can't be patched by jhbuild.
Comment 3 Csaba Osztrogonác 2015-06-10 03:48:52 PDT
Created attachment 254643 [details]
Patch
Comment 4 Csaba Osztrogonác 2015-06-10 03:51:47 PDT
Comment on attachment 254643 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254643&action=review

> Tools/efl/patches/openwebrtc-gst-plugins-clang-warning-fix.patch:10
> +-libgstvideorepair_la_CFLAGS = $(GST_CFLAGS) $(GST_PLUGINS_BASE_CFLAGS) -Wall -Wextra -Werror
> ++libgstvideorepair_la_CFLAGS = $(GST_CFLAGS) $(GST_PLUGINS_BASE_CFLAGS) -Wall -Wextra

Adding -Wno-parentheses-equality here doesn't work, because 
the parent Makefile adds -Wall to CFLAGS which comes after 
libgstvideorepair_la_CFLAGS and overrides the -Wno option.
Comment 5 Rohit 2015-06-10 06:04:02 PDT
I have been facing this issue on GTK port for some time now. Is there any workaround to build WebKit for GTK port?
Comment 6 Csaba Osztrogonác 2015-06-10 06:16:10 PDT
(In reply to comment #5)
> I have been facing this issue on GTK port for some time now. Is there any
> workaround to build WebKit for GTK port?

You can build the dependencies with GCC without any problem.
And then you can build WebKit with GCC or Clang too.
Comment 7 Csaba Osztrogonác 2015-06-12 03:37:46 PDT
cc-ing Philip, maybe you are interested in fixing it for GTK
and/or fixing it in upstream openwebrtc / gst-plugins-openwebrtc.
Comment 8 Philippe Normand 2015-06-12 06:08:40 PDT
Would you like to submit the patch upstream? It's yours after all :)
Comment 9 Csaba Osztrogonác 2015-06-26 07:41:29 PDT
(In reply to comment #8)
> Would you like to submit the patch upstream? It's yours after all :)

upstream issues:
https://github.com/EricssonResearch/openwebrtc-gst-plugins/issues/44
https://github.com/EricssonResearch/openwebrtc/issues/420
Comment 10 Csaba Osztrogonác 2015-07-30 11:45:44 PDT
I reported the bug upstream. Can we push this patch
to WebKit to be able to build dependencies with clang?
Comment 11 Gyuyoung Kim 2015-07-30 21:12:29 PDT
Comment on attachment 254643 [details]
Patch

rs=me.
Comment 12 Philippe Normand 2015-07-31 02:01:52 PDT
Comment on attachment 254643 [details]
Patch

Please bump the version now that your patch was merged upstream :)
Comment 13 Csaba Osztrogonác 2015-09-24 03:46:55 PDT
(In reply to comment #12)
> Comment on attachment 254643 [details]
> Patch
> 
> Please bump the version now that your patch was merged upstream :)

Only the openwebrtc patch was merged, but bumping isn't as trivial as you think. Not at all. The openwebrtc-gst-plugin fix was rejected.

Bumping to trunk openwebrtc is impossible, because https://github.com/EricssonResearch/openwebrtc/commit/49635a9eba2d3e081cab7ba631bc32a5521975dd
needs newer gst-plugins-base - https://github.com/gstreamer-mirror/gst-plugins-base/commit/bfc13c8e51862662804dcc9d14bf3a7f9f0027bd . 

Or bumping to the exact openwebrtc revision which merged my fix 
isn't as easy, because we would need many unrelated fixes too:
- https://github.com/EricssonResearch/openwebrtc/commit
605fbcdc734444a936dde73ddc19b300f59c7503 commit in openwebrtc
uses a feature added to libnice by https://github.com/libnice/libnice/commit/4e4af4be3890f12e300fe71104564171091b7a17 , so we will need to bump libnice too.
This commit is already included in 0.1.11.
- Additionally we would have to fix FindOpenWebRTC.cmake too, because the
package version was bumped by https://github.com/EricssonResearch/openwebrtc/commit/437679ffb777265f6bcfdea2be0d0aec8bc6a474
- We would need to add -Wno-parentheses-equality to the brand
new tests/Makefile.am

----

I don't think if we should fix the whole world and bump absouletely
unrelated modules to unstable revisions (with possible new bugs, etc.)
just to suppress some compiler warnings.
Comment 14 Csaba Osztrogonác 2015-09-24 03:55:17 PDT
Comment on attachment 254643 [details]
Patch

r? again, I still think it is the easieast fix for this issue.
Comment 15 Philippe Normand 2015-09-24 04:13:35 PDT
Bumping openwebrtc* only makes sense after we migrated to GStreamer 1.6.x.

OpenWebRTC barely works anyway with older versions of GStreamer.
Comment 16 Csaba Osztrogonác 2015-09-28 04:28:09 PDT
So can we land this patch as is?
Comment 17 Csaba Osztrogonác 2015-10-01 03:00:05 PDT
ping?
Comment 18 WebKit Commit Bot 2015-10-01 08:39:29 PDT
Comment on attachment 254643 [details]
Patch

Clearing flags on attachment: 254643

Committed r190409: <http://trac.webkit.org/changeset/190409>
Comment 19 WebKit Commit Bot 2015-10-01 08:39:34 PDT
All reviewed patches have been landed.  Closing bug.