Differential D8577

Fix: Missing dependencies for ktexeditor autotests
ClosedPublic
Actions

Authored by hgoebel on Oct 31 2017, 4:04 PM.

Details

Reviewers

dfaure

Group Reviewers

KTextEditor

Commits

R39:d6fc0b782046: Fix: Missing dependencies for ktexeditor autotests

Summary

When building ktexeditor autotests for guix[1], building the following autotests fail:

autotests/src/scripting_test.cpp
autotests/src/vimode/base.cpp

The reasons are missing dependencies in autotests/CMakeLists.txt resp. autotests/src/vimode/CMakeLists.txt – and related missing dependencies in CMakeLists.txt.

See https://phabricator.kde.org/D8112 (already merged) for the same kind of problem for kwin.

[1] https://www.gnu.org/software/guix/

Detailed description and Analysis
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

guix (like nix) used a separate prefix for each package. Thus the header files for qtdeclarative are in /gnu/store/…-qtdeclarative-5.9.1/include/qt5/ while the ones for qtbase are in /gnu/store/…-base-5.9.1/include/qt5/. This means that *each* dependency's include directory must be specified. This is in contrast to a "normal" Unix-system, where all includes end up in /usr/include/qt5 and missing to define some include-dependencies does often not raise an error.

Diff Detail

Repository

R39 KTextEditor

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

hgoebel created this revision.Oct 31 2017, 4:04 PM

hgoebel created this object with visibility "No One".

Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptOct 31 2017, 4:04 PM

Restricted Application added a subscriber: Frameworks. · View Herald Transcript

hgoebel updated this revision to Diff 21656.Oct 31 2017, 4:26 PM

hgoebel changed the visibility from "No One" to "Public (No Login Required)".

hgoebel added a reviewer: KTextEditor.

Please note that I do not have commit rights, so please somebody else commit.

dfaure accepted this revision.Nov 5 2017, 9:01 PM

This revision is now accepted and ready to land.Nov 5 2017, 9:01 PM

Closed by commit R39:d6fc0b782046: Fix: Missing dependencies for ktexeditor autotests (authored by hgoebel, committed by dfaure). · Explain WhyNov 5 2017, 9:01 PM

This revision was automatically updated to reflect the committed changes.

I am a bit surprised by this commit: we just removed the dependency on QtScript in favor of QtQML and now it's back. I don't think this hurts much, but the goal is to not depend on it... ?!

QtScript is only required for the tests. Maybe just a left-over #include?

Hmm, I would rather like to have this fixed than having that dependency just for that.
I thought during the port it compiled completely without QtScript.

Well, the currently the dependency is still needed, see https://cgit.kde.org/ktexteditor.git/tree/autotests/src/scripting_test.cpp#n51:

#include <QtScript/QScriptEngine>

If the code is not compiled in an isolated and controlled environment, and QtScript is installed on the system, the compiler might silently get the includes installed elsewhere. The same is true, if some QT package requires QtScript and QtScript is installed alongside that package.

Could you just remove the problematic includes and try to compile without the dependency, IMHO I see no use of QtScript stuff.

My aim is to bring Plasma into GuixSD, not to develop ktexteditor. So please understand that I'm not going to work on ktexteditor.

In D8577#164815, @cullmann wrote:

Could you just remove the problematic includes and try to compile without the dependency, IMHO I see no use of QtScript stuff.

Done with: 631b1447c97f1d81de2e81d2556474f0bc3109dd

Revision Contents
Changeset List

			Path	Packages
M			CMakeLists.txt (3 lines)
M			autotests/CMakeLists.txt (1 line)
M			autotests/src/vimode/CMakeLists.txt (1 line)
M			ktexteditor-5.39.0/CMakeLists.txt (3 lines)
M			ktexteditor-5.39.0/autotests/CMakeLists.txt (1 line)
M			ktexteditor-5.39.0/autotests/src/vimode/CMakeLists.txt (1 line)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	21652			Oct 31 2017, 3:50 PM	★	★
Diff 2	21656			Oct 31 2017, 4:25 PM	★	★
Diff 3	21931	a77148e	R39:d6fc0b782046b6f2086b1c3fcc9d8d1dc1569147	Nov 5 2017, 9:01 PM	★	★