D20007: Add GetProcessList for retrieving the list of currently active processes

dfaure added a comment. The CI isn't happy with this code. FreeBSD fails: https://build.kde.org/job/Frameworks/job/kcoreaddons/job/kf5-qt5%20FreeBSDQt5.13/lastCompletedBuild/testReport/projectroot/autotests/kprocesslisttest/ Windows fails: https://build.kde.org/job/Frameworks/job/kcore

D20007: Add GetProcessList for retrieving the list of currently active processes

hallas added a comment. Thanks for the review! Landing it now. REPOSITORY R244 KCoreAddons BRANCH adds_kprocesslist (branched from master) REVISION DETAIL https://phabricator.kde.org/D20007 To: hallas, davidedmundson, broulik Cc: vonreth, adridg, elvisangelaccio, kde-frameworks-devel,

D20007: Add GetProcessList for retrieving the list of currently active processes

hallas closed this revision. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D20007 To: hallas, davidedmundson, broulik Cc: vonreth, adridg, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, bruns

D20007: Add GetProcessList for retrieving the list of currently active processes

davidedmundson accepted this revision. This revision is now accepted and ready to land. REPOSITORY R244 KCoreAddons BRANCH adds_kprocesslist (branched from master) REVISION DETAIL https://phabricator.kde.org/D20007 To: hallas, davidedmundson, broulik Cc: vonreth, adridg, elvisangelaccio,

D20007: Add GetProcessList for retrieving the list of currently active processes

hallas added a comment. @davidedmundson - ping ? REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D20007 To: hallas, davidedmundson, broulik Cc: vonreth, adridg, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, bruns

D20007: Add GetProcessList for retrieving the list of currently active processes

hallas added a comment. @davidedmundson - Ping ;) REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D20007 To: hallas, davidedmundson, broulik Cc: vonreth, adridg, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, bruns

D20007: Add GetProcessList for retrieving the list of currently active processes

hallas added inline comments. INLINE COMMENTS > davidedmundson wrote in kprocesslist.cpp:69 > With the QSharedDataPointer you can delete this method. > > The point of the shareddatapointer is that when you copy KProcessInfo you > don't copy all the members, we just increase the internal ref-cou

D20007: Add GetProcessList for retrieving the list of currently active processes

hallas updated this revision to Diff 56237. hallas added a comment. Review comments REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20007?vs=54949&id=56237 BRANCH adds_kprocesslist (branched from master) REVISION DETAIL https://phabricator.kde.org/

D20007: Add GetProcessList for retrieving the list of currently active processes

davidedmundson added a comment. Looks good to me. \o/ Hopefully a windows person can test soon. Failing that, it's near the start of the month and we have a good unit test. We can ship it and see what CI says. INLINE COMMENTS > kprocesslist.cpp:69 > +{ > +d_ptr->valid = other.d_pt

D20007: Add GetProcessList for retrieving the list of currently active processes

hallas added a comment. @davidedmundson - could you please take a look at this again? I have implemented all the stuff you suggested ;) REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D20007 To: hallas, davidedmundson, broulik Cc: vonreth, adridg, elvisangelaccio

D20007: Add GetProcessList for retrieving the list of currently active processes

hallas added a comment. BTW - I tried locally chaning the D19989 patch to use the new `KProcessList::processInfoList` function and that enables us to achieve the same thing but without the KSysGuard dependency, so that's great :) Another thing, shoul

D20007: Add GetProcessList for retrieving the list of currently active processes

hallas added a comment. In D20007#438640 , @adridg wrote: > On FreeBSD, `/proc` is not necessarily mounted (it might be a Linuxism). So while I do **have** `/proc`, it's empty because procfs isn't mounted there. If I **do** mount it, then there'

D20007: Add GetProcessList for retrieving the list of currently active processes

hallas added inline comments. INLINE COMMENTS > davidedmundson wrote in kprocesslisttest.cpp:38 > KUser::KUser() > > (fortunately it's in kcoreaddons!) Nice :) I didn't know that one > davidedmundson wrote in kprocesslisttest.cpp:52 > qint64 QCoreApplication::applicationPid() Got it - I have

D20007: Add GetProcessList for retrieving the list of currently active processes

hallas updated this revision to Diff 54949. hallas marked 7 inline comments as done. hallas added a comment. Implemented review comments REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20007?vs=54827&id=54949 BRANCH adds_kprocesslist (branched from ma

D20007: Add GetProcessList for retrieving the list of currently active processes

davidedmundson added a comment. These two should make the unit tests windows compatible (untested) INLINE COMMENTS > kprocesslisttest.cpp:38 > +{ > +struct passwd *pwdEntry = getpwuid(uid); > +if (pwdEntry == nullptr) { KUser::KUser() (fortunately it's in kcoreaddons!) > kprocessli

D20007: Add GetProcessList for retrieving the list of currently active processes

elvisangelaccio added a comment. Don't forget to update the commit message (it still mentions "Add GetProcessList") ;) REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D20007 To: hallas, davidedmundson, broulik Cc: vonreth, adridg, elvisangelaccio, kde-frameworks-

D20007: Add GetProcessList for retrieving the list of currently active processes

adridg added a comment. On FreeBSD, `/proc` is not necessarily mounted (it might be a Linuxism). So while I do **have** `/proc`, it's empty because procfs isn't mounted there. If I **do** mount it, then there's the expected list of processes and a curproc symlink. But `/proc/` doesn't contai

D20007: Add GetProcessList for retrieving the list of currently active processes

adridg added a comment. Config: Using QtTest library 5.12.1, Qt 5.12.1 (x86_64-little_endian-lp64 shared (dynamic) release build; by Clang 6.0.1 (tags/RELEASE_601/final 335540)) PASS : KProcessListTest::initTestCase() PASS : KProcessListTest::testKProcessInfoConstructionAssignment

D20007: Add GetProcessList for retrieving the list of currently active processes

davidedmundson added subscribers: adridg, vonreth. davidedmundson added a comment. Awesome progress. Thanks ever so much. @vonreth could you test the windows side for us and tell us how to do the unit test? @adridg could you run this on BSD. You should just need to run ./bin/kproce

D20007: Add GetProcessList for retrieving the list of currently active processes

hallas added a comment. Ok, I have implemented all the review feedback you gave, I have also done some unit tests. Some of the unit tests or Unix specific though, I still don't know what to do about Windows since I don't have a development environment. Please give it a good look :D REPOSITO

D20007: Add GetProcessList for retrieving the list of currently active processes

hallas marked 9 inline comments as done. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D20007 To: hallas, davidedmundson, broulik Cc: elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, bruns

D20007: Add GetProcessList for retrieving the list of currently active processes

hallas updated this revision to Diff 54827. hallas added a comment. Updated with review comments REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20007?vs=54638&id=54827 BRANCH adds_kprocesslist (branched from master) REVISION DETAIL https://phabric

D20007: Add GetProcessList for retrieving the list of currently active processes

hallas added a comment. Thanks a lot for the feedback guys, I will spend some time and implement it and push an updated patch. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D20007 To: hallas, davidedmundson, broulik Cc: elvisangelaccio, kde-frameworks-devel, mi

D20007: Add GetProcessList for retrieving the list of currently active processes

elvisangelaccio added inline comments. INLINE COMMENTS > kprocesslist.h:56 > + */ > +KProcessList GetProcessList(); > + This method should follow the Qt naming style, i.e. `processList()`. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D20007 To: hallas, davidedmu

D20007: Add GetProcessList for retrieving the list of currently active processes

davidedmundson added a comment. To give a context for people who haven't seen the prior conversation. Right now kdevelop (optionally) pulls in libksysguard from plasma to show a simple process list, which isn't ideal. We're not technically API stable and we'll go to Qt6 at a different

D20007: Add GetProcessList for retrieving the list of currently active processes

hallas added inline comments. INLINE COMMENTS > CMakeLists.txt:246 >util/kformat.h >util/kuser.h >util/kshell.h Should util/kprocesslist.h also be added here? > kprocesslist.h:1 > +/** > +** Is this

D20007: Add GetProcessList for retrieving the list of currently active processes

hallas created this revision. hallas added reviewers: davidedmundson, broulik. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. hallas requested review of this revision. REVISION SUMMARY Add GetProcessList for retrieving the list of currently active processe