Bug 184257 – [GTK] Add CMake package search for vpx and libevent libraries

• Component: WebRTC
• Status: RESOLVED FIXED

[Alejandro G. Castro]

For libwebrtc compilation we are just searching the libraries and the errors when they are not found are not very informative without this.

[Alejandro G. Castro]

Created attachment 337066 [details]
Patch

[Alejandro G. Castro]

Created attachment 337069 [details]
Patch

[Alejandro G. Castro]

Created attachment 337072 [details]
Patch

[Michael Catanzaro]

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.

[Alejandro G. Castro]

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

[Carlos Garcia Campos]

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.

[Alejandro G. Castro]

Created attachment 337152 [details]
Patch

[Alejandro G. Castro]

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!

[Michael Catanzaro]

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.

[Alejandro G. Castro]

Created attachment 337257 [details]
Patch for landing

[Alejandro G. Castro]

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

[WebKit Commit Bot]

Comment on attachment 337257 [details]
Patch for landing

Clearing flags on attachment: 337257

Committed r230297: <https://trac.webkit.org/changeset/230297>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39201847>