Bug 184257 - [GTK] Add CMake package search for vpx and libevent libraries
Summary: [GTK] Add CMake package search for vpx and libevent libraries
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alejandro G. Castro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-04-03 02:53 PDT by Alejandro G. Castro
Modified: 2018-04-05 01:01 PDT (History)
6 users (show)

See Also:


Attachments
Patch (7.01 KB, patch)
2018-04-03 03:00 PDT, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (7.01 KB, patch)
2018-04-03 04:40 PDT, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (7.01 KB, patch)
2018-04-03 05:13 PDT, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (8.35 KB, patch)
2018-04-04 02:02 PDT, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch for landing (8.12 KB, patch)
2018-04-05 00:24 PDT, Alejandro G. Castro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alejandro G. Castro 2018-04-03 02:53:37 PDT
For libwebrtc compilation we are just searching the libraries and the errors when they are not found are not very informative without this.
Comment 1 Alejandro G. Castro 2018-04-03 03:00:18 PDT
Created attachment 337066 [details]
Patch
Comment 2 Alejandro G. Castro 2018-04-03 04:40:28 PDT
Created attachment 337069 [details]
Patch
Comment 3 Alejandro G. Castro 2018-04-03 05:13:06 PDT
Created attachment 337072 [details]
Patch
Comment 4 Michael Catanzaro 2018-04-03 10:03:55 PDT
Comment on attachment 337072 [details]
Patch

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

But they're clearly only used by libwebrtc, so these don't belong outside Source/ThirdParty/libwebrtc.

> Source/cmake/OptionsGTK.cmake:240
> +    find_package(Vpx 1.7.0)
> +    if (NOT LIBVPX_FOUND)
> +       message(FATAL_ERROR "vpx is needed for USE_LIBWEBRTC.")
> +    endif ()
> +    find_package(LibEvent)
> +    if (NOT LIBEVENT_FOUND)
> +       message(FATAL_ERROR "libevent is needed for USE_LIBWEBRTC.")
> +    endif ()

I think you want to do something like this, but in Source/ThirdParty/libwebrtc/CMakeLists.txt instead.
Comment 5 Alejandro G. Castro 2018-04-04 01:26:25 PDT
(In reply to Michael Catanzaro from comment #4)
> Comment on attachment 337072 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=337072&action=review
> 
> But they're clearly only used by libwebrtc, so these don't belong outside
> Source/ThirdParty/libwebrtc.
> 
> > Source/cmake/OptionsGTK.cmake:240
> > +    find_package(Vpx 1.7.0)
> > +    if (NOT LIBVPX_FOUND)
> > +       message(FATAL_ERROR "vpx is needed for USE_LIBWEBRTC.")
> > +    endif ()
> > +    find_package(LibEvent)
> > +    if (NOT LIBEVENT_FOUND)
> > +       message(FATAL_ERROR "libevent is needed for USE_LIBWEBRTC.")
> > +    endif ()
> 
> I think you want to do something like this, but in
> Source/ThirdParty/libwebrtc/CMakeLists.txt instead.

That is an option, but I saw libraries in thirdparty directory were doing it here and carlos recommended to use optionsgtk, so not sure about it :-). Maybe Carlos can comment about his preferences.
Comment 6 Carlos Garcia Campos 2018-04-04 01:32:59 PDT
I don't care where the checks are as along as the error message is understandable, with the current one it's not obvious what is missing and why that is required.
Comment 7 Alejandro G. Castro 2018-04-04 02:02:41 PDT
Created attachment 337152 [details]
Patch
Comment 8 Alejandro G. Castro 2018-04-04 02:14:57 PDT
I moved the detection to the libwebrtc directory, even if it seems others are doing it in OptionsGTK I agree with Michael it is better to add them in the libwebrtc/CMakeList.txt.

Thanks for the comments!
Comment 9 Michael Catanzaro 2018-04-04 09:59:48 PDT
Comment on attachment 337152 [details]
Patch

Can't you move the find modules, too?

What other ThirdParty modules have checks for their dependencies in OptionsGTK.cmake? That shouldn't be. I don't see any.
Comment 10 Alejandro G. Castro 2018-04-05 00:24:19 PDT
Created attachment 337257 [details]
Patch for landing
Comment 11 Alejandro G. Castro 2018-04-05 00:26:04 PDT
(In reply to Michael Catanzaro from comment #9)
> Comment on attachment 337152 [details]
> Patch
> 
> Can't you move the find modules, too?
> 

I did, they are in a cmake directory inside libwebrtc directory.

> What other ThirdParty modules have checks for their dependencies in
> OptionsGTK.cmake? That shouldn't be. I don't see any.

Your are right, just checked more carefully and what I thought they were thirdparties now they are system dependencies, sorry about that.

Thanks for the review!
Comment 12 WebKit Commit Bot 2018-04-05 01:00:54 PDT
Comment on attachment 337257 [details]
Patch for landing

Clearing flags on attachment: 337257

Committed r230297: <https://trac.webkit.org/changeset/230297>
Comment 13 WebKit Commit Bot 2018-04-05 01:00:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2018-04-05 01:01:57 PDT
<rdar://problem/39201847>