Differential D7950

Document how to determine which optional dependencies are used (e.g. QtWebKit instead of QtWebEngine
ClosedPublic
Actions

Authored by rjvbb on Sep 23 2017, 9:30 AM.

Details

Reviewers
apol
kfunk
Group Reviewers
KDevelop
Commits
R32:69548771c676: Document how to determine which optional dependencies are used (e.g. QtWebKit…
Summary

As long as QtWebKit remains a supported option for documentation rendering users might want an option to build KDevelop against that library even if QWE is present on their system.

KDevelop uses enough resources of its own, so an option to avoid using the additional resource-hungry QWE is very welcome (after all, many will probably already have a chrome-based browser running).

This patch is the simplest possible implementation, and doesn't advertise itself by creating an option variable in the toplevel CMake file. I'd be happy to add that possibility though.

Test Plan

It would be nice if CMake's summary printed a list of optional dependencies that were disabled explicitly.

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
rjvbb created this revision.Sep 23 2017, 9:30 AM
rjvbb added a reviewer: KDevelop.
rjvbb set the repository for this revision to R32 KDevelop.
rjvbb added a project: KDevelop.
rjvbb added a subscriber: kdevelop-devel.
apol requested changes to this revision.Sep 23 2017, 9:19 PM
apol added a subscriber: apol.

This is not necessary: see here.

You can use -DCMAKE_DISABLE_FIND_PACKAGE_Qt5WebEngine.

This revision now requires changes to proceed.Sep 23 2017, 9:19 PM
rjvbb added a comment.Sep 24 2017, 6:44 AM

Ah, nice. I don't have time to test right now, but I do note that cmake's own docs say

Every non-REQUIRED find_package call can be disabled by setting the CMAKE_DISABLE_FIND_PACKAGE_<PackageName> variable to TRUE.

And that makes sense if you want to protect users from themselves.

rjvbb added a comment.Sep 24 2017, 4:06 PM

Make that -DCMAKE_DISABLE_FIND_PACKAGE_Qt5WebEngineWidgets=ON but then it works. I see now that the REQUIRED nature is not declared in the find_package command but in set_package_properties, I guess that's why it works?

kfunk requested changes to this revision.Sep 24 2017, 8:06 PM
kfunk added a subscriber: kfunk.
In D7950#148564, @rjvbb wrote:

Make that -DCMAKE_DISABLE_FIND_PACKAGE_Qt5WebEngineWidgets=ON but then it works. I see now that the REQUIRED nature is not declared in the find_package command but in set_package_properties, I guess that's why it works?

Yes. If it would be REQUIRED in find_package, it would fail. Took me 3 seconds to verify:

$ cmake -DCMAKE_DISABLE_FIND_PACKAGE_Qt5Core=ON .

CMake Error at /usr/lib/x86_64-linux-gnu/cmake/Qt5/Qt5Config.cmake:26 (find_package):
  find_package for module Qt5Core called with REQUIRED, but
  CMAKE_DISABLE_FIND_PACKAGE_Qt5Core is enabled.  A REQUIRED package cannot
  be disabled.
kfunk added a comment.Sep 24 2017, 8:06 PM
In D7950#148347, @apol wrote:

This is not necessary: see here.

+1

rjvbb added a comment.Sep 25 2017, 7:43 AM

So, should I just drop this DR or turn it into a proposition how to document the cmake feature?

I'd look into introducing making the dependency on *a* qtweb framework optional but really don't have time for that right now.

kfunk added a comment.Sep 25 2017, 8:10 AM

Feel free to document this -- probably best in the top-level ReadMe.md under a new subsection in the *Compile* section.

rjvbb updated this revision to Diff 19886.Sep 25 2017, 9:44 AM
rjvbb edited the test plan for this revision. (Show Details)

Patch updated to add a subsection to the README.md instead of the initial proposal to add a cmake option.
It felt a bit strange adding just this detail about compiling to a README that simply referred to a wiki page.

rjvbb retitled this revision from Add (hidden) option to avoid using QtWebEngine to Document how to determine which optional dependencies are used (e.g. QtWebKit instead of QtWebEngine.Sep 25 2017, 9:47 AM
rjvbb edited the test plan for this revision. (Show Details)
rjvbb set the repository for this revision to R32 KDevelop.
kfunk accepted this revision.Sep 25 2017, 9:47 AM

Thanks, looks good to me!

kfunk added a comment.Sep 25 2017, 9:48 AM

Let me push that.

Closed by commit R32:69548771c676: Document how to determine which optional dependencies are used (e.g. QtWebKit… (authored by rjvbb, committed by kfunk). · Explain WhySep 25 2017, 9:50 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
MREADME.md (16 lines)

Diff 19887

View Options

README.md

Loading...