D4929: DrKonqi : lldb support (and cross-platform adaptation)

[René J.V. Bertin]

rjvbb created this revision.
rjvbb added a project: Plasma: Workspaces.
Restricted Application edited projects, added Plasma; removed Plasma: Workspaces.

REVISION SUMMARY
As touched upon previously, DrKonqi is a utility that can be very useful outside of Plasma desktop environments too, and as such it should at the very least have basic support for the lldb debugger.

This patch introduces that support, providing useful backtraces in bug tickets created through DrKonqi. Attaching lldb to the crashed executable is currently done through a wrapper script that invokes Apple's Terminal.app but that approach is open for discussion.

I've included a few minor adaptations for cross-platform/non Plasma use of DrKonqi (preserve existing app/window icons when QIcon::fromTheme() fails and forcing DrKonqi to the foreground on Mac).

TEST PLAN
This patch has been tested on Mac for over a year now; I have submitted numerous bug reports with it.

The backtrace parser could probably be improved.

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D4929

AFFECTED FILES
drkonqi/CMakeLists.txt
drkonqi/aboutbugreportingdialog.cpp
drkonqi/backtracegenerator.cpp
drkonqi/backtracegenerator.h
drkonqi/backtracewidget.cpp
drkonqi/bugzillaintegration/reportassistantdialog.cpp
drkonqi/data/AppleTerminal
drkonqi/data/CMakeLists.txt
drkonqi/data/debuggers/external/lldbrc
drkonqi/data/debuggers/internal/lldbrc
drkonqi/debugger.cpp
drkonqi/debugger.h
drkonqi/drkonqibackends.cpp
drkonqi/drkonqidialog.cpp
drkonqi/main.cpp
drkonqi/parser/CMakeLists.txt
drkonqi/parser/backtraceparser.cpp
drkonqi/parser/backtraceparserlldb.cpp
drkonqi/parser/backtraceparserlldb.h

To: rjvbb, #plasma_workspaces
Cc: kde-mac, plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Kai Uwe Broulik]

broulik added a comment.


Do apps even ship DrKonqi? It's part of plasma-workspace, which isn't meant for non-X11/Wayland, after all.

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces
Cc: broulik, kde-mac, plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[René J.V. Bertin]

rjvbb added a comment.


Not currently, but DrKonqi ought *really* be elsewhere. There's nothing Plasma-specific to it. On the contrary, it should be easily available on every platform for which KDE applications are available so that every KDE user can contribute to KDE improvement by creating useful bug reports. This has been discussed before.

If memory serves me well DrKonqi is launched by KCrash and IMHO it would make sense to incorporate it there.

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces
Cc: broulik, kde-mac, plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[René J.V. Bertin]

rjvbb retitled this revision from "DrKonqi : lldb support (and cross-platform adaptation)" to "DrKonqi : lldb support".
rjvbb edited the summary of this revision.

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces
Cc: broulik, kde-mac, plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Marco Martin]

mart added a comment.


hmm, frameworksintegration? (but still in frameworks side)

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces
Cc: mart, broulik, kde-mac, plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[René J.V. Bertin]

rjvbb added a comment.

Post by Marco Martin
hmm, frameworksintegration? (but still in frameworks side)

Bundling with, you mean? That could work too.

It's not really the topic of this review request, but I discussed the idea of bundling with someone who bundles a well-known KDE app for Mac and MS Windows. I'll leave it to him to come forward (or not) but it looks we both think that KCrash would be the most logical framework.

Many applications already depend on it (not the case with frameworksintegration) so done right no extra action would be required to build and include DrKonqi in binary distribution forms of those applications. More useful bug reports concerning crashers (because a backtrace is attached) for little effort, who would not appreciate that?

KCrash would probably have to become a Tier3 framework - no idea if that's an issue in itself or not.

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces
Cc: mart, broulik, kde-mac, plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Kevin Funk]

kfunk requested changes to this revision.
kfunk added a comment.
This revision now requires changes to proceed.


So, plasma-workspace is usually way out of my comfort zone, I'm commenting anyway since RJVB urged me to do it.

See my notes.

INLINE COMMENTS

backtracegenerator.cpp:62
+ // than waiting a potentially very long time for it to heed the kill() request.
+ m_proc->deleteLater();
+ } else {

Please do this only if `lldb` is used then. No need to change code which apparently worked fine for years. Also, did you try `QProcess::terminate` instead?

PS: `QProcess` gets unhappy when being deleted while still running (=> runtime warnings).

PPS: Please read my other advice about quitting LLDB below, too

backtracegenerator.cpp:144
emit newLine(line);
+ line = line.simplified();
+ if (line.startsWith(QLatin1String("Process ")) && line.endsWith(QLatin1String(" detached"))) {

This whole logic looks really cumbersome.

Is there really no way to exit LLDB cleanly after detach?

http://stackoverflow.com/questions/26267289/how-can-i-exit-lldb-after-running-commands-with-o suggests there is:

lldb /bin/ls -o "run" -o "script import os; os._exit(1)"

I take it not everyone's got Python on they system, but this works for me as well:

lldb -p $(pidof kate) -o detach -o quit

Just put that into the `-o quit` in the lldbrc?

backtracegenerator.h:87
QString m_parsedBacktrace;
+ bool m_lldbDetached;

Looks pretty unclean to have a backend-specific variable around here.

AppleTerminal:1
+#!/bin/sh
+

Please explain why this file is needed(?)

lldbrc:9
+ExecInputFile=%tempfile
+BatchCommands=set set term-width 200\nthread info\nbt all\ndetach

`set set`? Really?

drkonqibackends.cpp:165
KConfigGroup config(KSharedConfig::openConfig(), "DrKonqi");
-#ifndef Q_OS_WIN
+#if defined(__MAC_OS_X_VERSION_MAX_ALLOWED) && __MAC_OS_X_VERSION_MAX_ALLOWED > 1070
+ QString defaultDebuggerName = config.readEntry("Debugger", QString("lldb"));

Why these special conditions? Needs comments.

Whether to use or not to use lldb on a particular OS X version should be runtime decision, too.

backtraceparserlldb.cpp:26
+ BacktraceLineLldb(const QString & line);
+};

Style: Use `const QString &line`

More of these issues in other lines

backtraceparserlldb.h:28
+ explicit BacktraceParserLldb(QObject *parent = 0);
+

`nullptr`

backtraceparserlldb.h:31
+ virtual void newLine(const QString & lineStr);
+

Use `override`, strip `virtual`

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces, kfunk
Cc: kfunk, mart, broulik, kde-mac, plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[René J.V. Bertin]

rjvbb marked 4 inline comments as done.
rjvbb added inline comments.

INLINE COMMENTS

kfunk wrote in backtracegenerator.cpp:62
Please do this only if `lldb` is used then. No need to change code which apparently worked fine for years. Also, did you try `QProcess::terminate` instead?
PS: `QProcess` gets unhappy when being deleted while still running (=> runtime warnings).
PPS: Please read my other advice about quitting LLDB below, too

It's been a while, QProcess::terminate() was an approach I remember preferring to avoid possibly because it also led to runtime warnings. I also don't think there's much of a difference in what ::kill() and ::terminate() do.
With deleteLater() I rarely if ever see warning messages, probably because it's an implicit way of waiting long enough (in the background) for lldb to exit.

kfunk wrote in backtracegenerator.cpp:144
This whole logic looks really cumbersome.
Is there really no way to exit LLDB cleanly after detach?
lldb /bin/ls -o "run" -o "script import os; os._exit(1)"
lldb -p $(pidof kate) -o detach -o quit
Just put that into the `-o quit` in the lldbrc?

`-o quit` indeed works with newer lldb versions, but not yet with the system version on OS X 10.9 .

I suppose I could do a combination of both. The logical way would be `\nquit\nscript import os; os._exit(0)` but processing stops after the quit command so it'd have to be the opposite. I'd have to test this for a bit.

Not having a python interpreter may not be an issue. I don't know how its complete absence is treated but errors in the python expression (loading an inexisting module for instance) don't stop processing of subsequent commands.

Can one put comments in the lldbrc file explaining the reason of the surprising BatchCommands set?

kfunk wrote in AppleTerminal:1
Please explain why this file is needed(?)

It is required to attach lldb in a terminal window on Mac using the native terminal application. I don't want to rely on konsole being installed there, in part because that application has issues with certain Control keystrokes under Qt5 (including Ctrl-C and family).

kfunk wrote in drkonqibackends.cpp:165
Why these special conditions? Needs comments.
Whether to use or not to use lldb on a particular OS X version should be runtime decision, too.

Actually, there can be a runtime decision to use lldb on earlier (10.7-) versions but not on later versions. Gdb doesn't work properly from 10.8 onwards. And to be honest lldb was in such an early stage in 10.7 and earlier that you wouldn't want to use the system version there.
Given that gdb still works there it seems unnecessarily complicated to make this a runtime decision. Esp. since using gdb means you get access to the standard backtrace parser.

I'll put a concise version of this explanation as a comment.

kfunk wrote in backtraceparserlldb.cpp:26
Style: Use `const QString &line`
More of these issues in other lines

I think I preserved the style in the backtracerparser implementation I cloned...

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces, kfunk
Cc: kfunk, mart, broulik, kde-mac, plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[René J.V. Bertin]

rjvbb marked 8 inline comments as done.
rjvbb added inline comments.

INLINE COMMENTS

kfunk wrote in backtracegenerator.cpp:62
Please do this only if `lldb` is used then. No need to change code which apparently worked fine for years. Also, did you try `QProcess::terminate` instead?
PS: `QProcess` gets unhappy when being deleted while still running (=> runtime warnings).
PPS: Please read my other advice about quitting LLDB below, too

Re (sic:) KProcess::terminate(): see the hunk around line 145!

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces, kfunk
Cc: kfunk, mart, broulik, kde-mac, plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[René J.V. Bertin]

rjvbb updated this revision to Diff 12360.
rjvbb added a comment.


This new version incorporates most of the changes Kevin requested.

Quitting lldb through

1. a python "lambda"
2. the quit command

seems to work reliably enough on Mac.

It still uses AppleTerminal.sh unconditionally, I'll need to see how I can install and invoke that script only on Mac.

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D4929?vs=12148&id=12360

REVISION DETAIL
https://phabricator.kde.org/D4929

AFFECTED FILES
drkonqi/CMakeLists.txt
drkonqi/aboutbugreportingdialog.cpp
drkonqi/backtracegenerator.cpp
drkonqi/backtracegenerator.h
drkonqi/backtracewidget.cpp
drkonqi/bugzillaintegration/reportassistantdialog.cpp
drkonqi/data/AppleTerminal
drkonqi/data/CMakeLists.txt
drkonqi/data/debuggers/external/lldbrc
drkonqi/data/debuggers/internal/lldbrc
drkonqi/debugger.cpp
drkonqi/debugger.h
drkonqi/drkonqibackends.cpp
drkonqi/drkonqidialog.cpp
drkonqi/main.cpp
drkonqi/parser/CMakeLists.txt
drkonqi/parser/backtraceparser.cpp
drkonqi/parser/backtraceparserlldb.cpp
drkonqi/parser/backtraceparserlldb.h

To: rjvbb, #plasma_workspaces, kfunk
Cc: kfunk, mart, broulik, kde-mac, plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[René J.V. Bertin]

rjvbb added inline comments.

INLINE COMMENTS

kfunk wrote in backtracegenerator.h:87
Looks pretty unclean to have a backend-specific variable around here.

I missed this one. I don't think it's possible to detect detaching (from the debugger output!) elsewhere but in the bt. generator. Not without adding a line parser to the backend.

That *would* be the proper way to proceed if other backends required detecting a detached debugger too, but apparently that isn't the case.

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces, kfunk
Cc: kfunk, mart, broulik, kde-mac, plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[René J.V. Bertin]

rjvbb added inline comments.

INLINE COMMENTS

kfunk wrote in backtracegenerator.h:87
Looks pretty unclean to have a backend-specific variable around here.

I suppose I could replace m_lldbDetached with something like

bool Debugger::isDetached(const QString &outputLine=QString())
{
if (!outputLine.isEmpty() && codeName() == "lldb") {
QString line = outputLine.simplified();
if (/*line contains "Process detached"*/) {
m_isDetached = true;
}
}
return m_isDetached;
}

and call that function instead of reading/setting m_lldbDetached.

Whether that's really less cumbersome I don't know?

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces, kfunk
Cc: kfunk, mart, broulik, kde-mac, plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[René J.V. Bertin]

rjvbb updated this revision to Diff 12368.
rjvbb added a comment.


platform-specific data/debuggers/external *rc files and installation of the AppleTerminal script.

gdb and lldb are now both started via that script on Mac and via konsole elsewhere.

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D4929?vs=12360&id=12368

REVISION DETAIL
https://phabricator.kde.org/D4929

AFFECTED FILES
drkonqi/CMakeLists.txt
drkonqi/aboutbugreportingdialog.cpp
drkonqi/backtracegenerator.cpp
drkonqi/backtracegenerator.h
drkonqi/backtracewidget.cpp
drkonqi/bugzillaintegration/reportassistantdialog.cpp
drkonqi/data/AppleTerminal
drkonqi/data/CMakeLists.txt
drkonqi/data/debuggers/external.mac/gdbrc
drkonqi/data/debuggers/external.mac/kdbgrc
drkonqi/data/debuggers/external.mac/lldbrc
drkonqi/data/debuggers/external/lldbrc
drkonqi/data/debuggers/internal/lldbrc
drkonqi/debugger.cpp
drkonqi/debugger.h
drkonqi/drkonqibackends.cpp
drkonqi/drkonqidialog.cpp
drkonqi/main.cpp
drkonqi/parser/CMakeLists.txt
drkonqi/parser/backtraceparser.cpp
drkonqi/parser/backtraceparserlldb.cpp
drkonqi/parser/backtraceparserlldb.h

To: rjvbb, #plasma_workspaces, kfunk
Cc: kfunk, mart, broulik, kde-mac, plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[patrick j pereira]

patrickelectric added a comment.
This revision now requires changes to proceed.


What is the actual state of this patch ?

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces, kfunk
Cc: patrickelectric, kfunk, mart, broulik, kde-mac, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[René J.V. Bertin]

rjvbb marked an inline comment as done.
rjvbb added a comment.


Sorry, I had updating this on my list but it drifted to the bottom...

I think I've addressed all feedback apart from the `m_lldbDetached` variable Kevin objected to. I'm open to suggestions how to make that aspect less eyebrow-raising. Give it a more generic name and add a comment that it's only set/used for lldb (or else set it in every backend)?

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces, kfunk
Cc: patrickelectric, kfunk, mart, broulik, kde-mac, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[René J.V. Bertin]

rjvbb updated this revision to Diff 46025.
rjvbb added a comment.


Refactored for the standalone DrKonqi repo and disabled the integration testing on Mac.

Making DrKonqi standalone is a good step, I'd strongly suggest to move it to KF5 Applications at the first possible occasion. The utility doesn't even depend on a single Plasma library and provides a service that has nothing Plasma-desktop specific.

Instead, ask yourself if automatic crash reports are welcome only from Plasma desktop users or if as many users as possible should be able to submit crash reports (i.e. from any platform where DrKonqi is functional). Better, don't ask yourself, ask the entire family of KDE developers.

On a related note: DrKonqi's dependencies have been bumped along with the other Plasma dependencies. That's overkill: it has no business requiring Qt 5.11, 5.9LTS provides all required APIs. Similarly, it builds just fine against KF5 Frameworks 5.47.0, possibly even earlier versions.

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D4929?vs=12368&id=46025

REVISION DETAIL
https://phabricator.kde.org/D4929

AFFECTED FILES
src/CMakeLists.txt
src/aboutbugreportingdialog.cpp
src/backtracegenerator.cpp
src/backtracegenerator.h
src/backtracewidget.cpp
src/bugzillaintegration/reportassistantdialog.cpp
src/data/AppleTerminal
src/data/CMakeLists.txt
src/data/debuggers/external.mac/gdbrc
src/data/debuggers/external.mac/kdbgrc
src/data/debuggers/external.mac/lldbrc
src/data/debuggers/external/lldbrc
src/data/debuggers/internal/lldbrc
src/debugger.cpp
src/debugger.h
src/drkonqibackends.cpp
src/drkonqidialog.cpp
src/main.cpp
src/parser/CMakeLists.txt
src/parser/backtraceparser.cpp
src/parser/backtraceparserlldb.cpp
src/parser/backtraceparserlldb.h
src/tests/CMakeLists.txt

To: rjvbb, #plasma_workspaces, kfunk
Cc: patrickelectric, kfunk, mart, broulik, kde-mac, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[René J.V. Bertin]

rjvbb retitled this revision from "DrKonqi : lldb support" to "DrKonqi : lldb and Mac support".
rjvbb edited the summary of this revision.
rjvbb set the repository for this revision to R871 DrKonqi.

REPOSITORY
R871 DrKonqi

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces, kfunk
Cc: patrickelectric, kfunk, mart, broulik, kde-mac, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[René J.V. Bertin]

rjvbb edited subscribers, added: KDE Applications; removed: plasma-devel.

REPOSITORY
R871 DrKonqi

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces, kfunk
Cc: #kde_applications, patrickelectric, kfunk, mart, broulik, kde-mac, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, plasma-devel

[patrick j pereira]

patrickelectric added a comment.


I agree, with your points.
About the mac compatibility, kcrash and drkonqi (It was merged yesterday) are already in KDE-mac brew repository, looking forward for this to be accepted !
I'll try to do some tests with your patch here today.

REPOSITORY
R871 DrKonqi

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces, kfunk
Cc: #kde_applications, patrickelectric, kfunk, mart, broulik, kde-mac, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[René J.V. Bertin]

rjvbb added a comment.


FWIW, anyone can request changes to a review, but also accept it. And while I would probably not take that as a green light to commit unless it comes from a known KDE dev it *does* go to show demand/need for a change. And hopefully, confirmation that it doesn't only work for the author.

REPOSITORY
R871 DrKonqi

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces, kfunk
Cc: #kde_applications, patrickelectric, kfunk, mart, broulik, kde-mac, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Luigi Toscano]

ltoscano added a subscriber: plasma-devel.

REPOSITORY
R871 DrKonqi

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces, kfunk
Cc: plasma-devel, #kde_applications, patrickelectric, kfunk, mart, broulik, kde-mac, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[patrick j pereira]

patrickelectric added a comment.


Can you rebase it over the last master branch ?

REPOSITORY
R871 DrKonqi

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces, kfunk
Cc: plasma-devel, #kde_applications, patrickelectric, kfunk, mart, broulik, kde-mac, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[René J.V. Bertin]

rjvbb added a comment.

Post by patrick j pereira
Can you rebase it over the last master branch ?

I did that hours ago, or at least I thought I did?! Is there a commit later than 62c33ba3a885106f31706cbfecc75190ca00c70c <https://phabricator.kde.org/R871:62c33ba3a885106f31706cbfecc75190ca00c70c> which I somehow do not see yet?

REPOSITORY
R871 DrKonqi

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces, kfunk
Cc: plasma-devel, #kde_applications, patrickelectric, kfunk, mart, broulik, kde-mac, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[patrick j pereira]

patrickelectric added a comment.


After taking a second look on it, I was able to merge the patch from the `src`folder and not from the repository root folder, weird..
Will do the tests here and now !

REPOSITORY
R871 DrKonqi

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces, kfunk
Cc: plasma-devel, #kde_applications, patrickelectric, kfunk, mart, broulik, kde-mac, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[patrick j pereira]

patrickelectric added a comment.


a find_packge for `KF5::WindowSystem` is missing in the root CMakeLists file.

CMake Error at src/CMakeLists.txt:84 (add_executable):
Target "drkonqi" links to target "KF5::WindowSystem" but the target was not
found. Perhaps a find_package() call is missing for an IMPORTED target, or
an ALIAS target is missing?

After adding the missing find_package, I was able to compile it but unable to make it to work with a test here, will do some tests to figure out the problem.

REPOSITORY
R871 DrKonqi

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces, kfunk
Cc: plasma-devel, #kde_applications, patrickelectric, kfunk, mart, broulik, kde-mac, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[patrick j pereira]

patrickelectric added a reviewer: davidedmundson.

REPOSITORY
R871 DrKonqi

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces, kfunk, davidedmundson
Cc: plasma-devel, #kde_applications, patrickelectric, kfunk, mart, broulik, kde-mac, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[René J.V. Bertin]

rjvbb added a comment.

Post by patrick j pereira
a find_packge for `KF5::WindowSystem` is missing in the root CMakeLists file.

Ah, thanks. I have that in my own cmake file of course, but also an unrelated change I thought I could safely keep out by just taking a diff of the `src` directory.

I test DrKonqi by starting an application like kate and then doing a `killall -SEGV kate`.
Note that I do use a patched Qt5 on Mac where QStandardPaths can be made to behave like it does on any other Unix variant. Without that DrKonqi might well be unable to find the lldbrc (or equivalent) file it needs.

The easiest way to make this platform independent would be to put those files in the Qt resource. That's a separate change though.

REPOSITORY
R871 DrKonqi

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces, kfunk, davidedmundson
Cc: plasma-devel, #kde_applications, patrickelectric, kfunk, mart, broulik, kde-mac, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[René J.V. Bertin]

rjvbb updated this revision to Diff 46044.
rjvbb added a comment.


includes the root cmake file changes.

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D4929?vs=46025&id=46044

REVISION DETAIL
https://phabricator.kde.org/D4929

AFFECTED FILES
CMakeLists.txt
src/CMakeLists.txt
src/aboutbugreportingdialog.cpp
src/backtracegenerator.cpp
src/backtracegenerator.h
src/backtracewidget.cpp
src/bugzillaintegration/reportassistantdialog.cpp
src/data/debuggers/external.mac/lldbrc
src/data/debuggers/external/lldbrc
src/data/debuggers/internal/lldbrc
src/debugger.cpp
src/debugger.h
src/drkonqibackends.cpp
src/drkonqidialog.cpp
src/main.cpp
src/parser/CMakeLists.txt
src/parser/backtraceparser.cpp
src/parser/backtraceparserlldb.cpp
src/parser/backtraceparserlldb.h
src/tests/CMakeLists.txt

To: rjvbb, #plasma_workspaces, kfunk, davidedmundson
Cc: plasma-devel, #kde_applications, patrickelectric, kfunk, mart, broulik, kde-mac, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[René J.V. Bertin]

rjvbb set the repository for this revision to R871 DrKonqi.

REPOSITORY
R871 DrKonqi

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces, kfunk, davidedmundson
Cc: plasma-devel, #kde_applications, patrickelectric, kfunk, mart, broulik, kde-mac, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[David Edmundson]

davidedmundson added a comment.

Post by René J.V. Bertin
Making DrKonqi standalone is a good step, I'd strongly suggest to move it to KF5 Applications at the first possible occasion.

That'll probably be when we move to Qt6.

Ultimately it'd still be the same tarball just in a different folder on an FTP site, so I doubt it really would make anyone's lives easier.

-----

Generally this patchset is a +1 from me, except for my second comment. It's a shame we pollute the main codebase in a few places for the one backend, but if there's no other option, then we have no choice.

Can you explain why you detach? Work to just call quit in the script to solve the hanging issue?

INLINE COMMENTS

Post by René J.V. Bertin
aboutbugreportingdialog.cpp:39
- setWindowIcon(QIcon::fromTheme(QStringLiteral("help-hint")));
+ setWindowIcon(QIcon::fromTheme(QStringLiteral("help-hint"), windowIcon()));

icon = QIcon::fromTheme
if (icon.isValid()) {
setWindowIcon(...)
}

is more standard..

Post by René J.V. Bertin
Note: On macOS, the window title bar icon is meant for windows representing documents, and will only show up if a file path is also set.

We don't set this, is this an issue?

Post by René J.V. Bertin
backtracegenerator.cpp:140
int pos;
- while ((pos = m_output.indexOf('\n')) != -1) {
+ while ((pos = m_output.indexOf('\n')) != -1 && m_proc->state() == QProcess::Running) {
QString line = QString::fromLocal8Bit(m_output.constData(), pos + 1);

this seems dangerous for the other clients.

It's not unfeasible for a process to have a load of data still in the buffer when it quits.

I don't know lldb, but it seems you can probably move this to ~149

REPOSITORY
R871 DrKonqi

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces, kfunk, davidedmundson
Cc: plasma-devel, #kde_applications, patrickelectric, kfunk, mart, broulik, kde-mac, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[René J.V. Bertin]

rjvbb added a comment.

Post by David Edmundson
That'll probably be when we move to Qt6.

Which is something I hope will be as far in the future as possible :)

but

Post by David Edmundson
I doubt it really would make anyone's lives easier.

Moving something outside of what I secretly call the Plasma jealousy universe should make life a bit easier for those who now have to argue why they'd want to use it. I can (kind of) understand why certain Plasma code would want to use the latest Qt5 version and almost why that version would be standardised across all Plasma members even though not necessary at all. So there's that too.

Post by David Edmundson
Can you explain why you detach? Work to just call quit in the script to solve the hanging issue?

In a nutshell, detaching before quitting makes the chance of hanging a lot smaller and speeds up the quitting too in my experience.

As I said, I can make the variable backend-agnostic if you think that's preferable. But maybe it could be a nice junior job or GSoC project to redesign the backend so that DrKonqi doesn't have to wait for the debugger to quit. With both gdb and lldb it should be possible to obtain a reloaded backtrace from the same debugger instance, and that refreh should be a lot faster if you don't have to wait for a new debugger instance to start and churn through all loaded libraries - and I'm guessing that there will be no hanging issues when you quit lldb at DrKonqi's exit.
And if so, keeping the backend-specific variable where it is could serve as a nice little reminder.

INLINE COMMENTS

Post by David Edmundson
davidedmundson wrote in aboutbugreportingdialog.cpp:39
icon = QIcon::fromTheme
if (icon.isValid()) {
setWindowIcon(...)
}
is more standard..

Post by René J.V. Bertin
Note: On macOS, the window title bar icon is meant for windows representing documents, and will only show up if a file path is also set.

We don't set this, is this an issue?

Hmmm, this may have changed after I developed the brunt of this patch (in KDE4 days). To answer your question, no, it's not a problem.
I'll have to look at this again, to see if this and similar changes still make sense.

Post by David Edmundson
davidedmundson wrote in backtracegenerator.cpp:140
this seems dangerous for the other clients.
It's not unfeasible for a process to have a load of data still in the buffer when it quits.
I don't know lldb, but it seems you can probably move this to ~149

You mean move the check on QProcess::Running into the `if (line.startsWith(QLatin1String("Process ")) && line.endsWith(QLatin1String(" detached")))` ?

REPOSITORY
R871 DrKonqi

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces, kfunk, davidedmundson
Cc: plasma-devel, #kde_applications, patrickelectric, kfunk, mart, broulik, kde-mac, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

rjvbb wrote in backtracegenerator.cpp:140
You mean move the check on QProcess::Running into the `if (line.startsWith(QLatin1String("Process ")) && line.endsWith(QLatin1String(" detached")))` ?

Yes

Also is it worth adding a "break;" there?

lldbrc:7
+[KCrash]
+Exec=AppleTerminal lldb -p %pid
+Terminal=true

Is this AppleTerminal line meant to be here?

REPOSITORY
R871 DrKonqi

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces, kfunk, davidedmundson
Cc: plasma-devel, #kde_applications, patrickelectric, kfunk, mart, broulik, kde-mac, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[René J.V. Bertin]

rjvbb added inline comments.

INLINE COMMENTS

Post by David Edmundson
davidedmundson wrote in backtracegenerator.cpp:140
Yes
Also is it worth adding a "break;" there?

A break instead of or in addition to the return? I think I'd prefer the return, unless you think that maybe someday there will be some extra steps to be taken after the while loop?

Post by David Edmundson
davidedmundson wrote in lldbrc:7
Is this AppleTerminal line meant to be here?

Yes, that file is used when the user asks to attach a debugger to the crashed process. We shouldn't presume that s/he has Konsole installed, nor that it is installed with a wrapper script on the path. So we install a script that starts Apple's standard terminal emulator with the proper arguments, and call that.

REPOSITORY
R871 DrKonqi

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces, kfunk, davidedmundson
Cc: plasma-devel, #kde_applications, patrickelectric, kfunk, mart, broulik, kde-mac, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[patrick j pereira]

patrickelectric added a comment.


@rjvbb I got it to work here ! Took some time to figure out the problems with relative paths and KConfig, I am working to bundle kcrash in a .dmg :)

REPOSITORY
R871 DrKonqi

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces, kfunk, davidedmundson
Cc: plasma-devel, #kde_applications, patrickelectric, kfunk, mart, broulik, kde-mac, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

rjvbb wrote in lldbrc:7
Yes, that file is used when the user asks to attach a debugger to the crashed process. We shouldn't presume that s/he has Konsole installed, nor that it is installed with a wrapper script on the path. So we install a script that starts Apple's standard terminal emulator with the proper arguments, and call that.

Linux has lldb too

I assumed that's why you also had a "src/data/debuggers/external.mac/lldbrc" as well as this file.

REPOSITORY
R871 DrKonqi

REVISION DETAIL
https://phabricator.kde.org/D4929

To: rjvbb, #plasma_workspaces, kfunk, davidedmundson
Cc: plasma-devel, #kde_applications, patrickelectric, kfunk, mart, broulik, kde-mac, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol