Re: Notice of intention to remove tests from KCrash and KNotifications

[Ben Cooksley]

On Tue, Nov 5, 2019 at 11:50 PM Harald Sitter  wrote:
>
> I get where you are coming from, but honestly, none of this makes
> pushing unreviewed commits that disable the entire test collection of
> an entire framework any more correct. If it had only affected the one
> test I wouldn't mind so much, but since it hasn't it only goes to
> proof that we have reviews for a reason. In particular for repos that
> are part of frameworks where we rely on tests to tell us if the
> frameworks are good for the monthly release.

Unfortunately reviews require responsive developers.

In this instance, both Frameworks have been found to have developers
which are not responsive (because my previous requests have been
ignored), so sending a patch for review would have been pointless -
because there would have been nobody to review it.

Therefore bypassing review and taking any necessary action against the
offending code (even if that does happen to be all of the tests) to
correct the situation is the only viable option forward.

>
> There are many shades of grey between sending a "someone please fix"
> mail to a mailing list and the nuclear option you implemented. The
> most notable one being that you can ask someone who worked on the
> repo, or tests recently "Hey, X, can you please disable the test on
> windows because its impairing CI?" to which the answer is probably yes
> because you'd not only address a very specific person but also the
> task is doable in less than 5 minutes.
> I understand that there's an element of cat herding in this, but
> quality must matter for our products, and the quality of frameworks is
> very tightly linked to autotesting and reviews because of how we
> release them.

This would require spending more time on the issue, which is something
i'd very much rather avoid.

It is expensive enough having to login to the CI builders to clear out
these jobs when they get stuck (looking at anything that uses Akonadi
especially here, along with these two Frameworks) and then asking the
project lists to please fix their faulty code (which is then ignored,
indicating that the concept of community maintained does not really
work).

Having to then lookup someone and then forward it on to them requires
yet more time, with no guarantee that it will work - especially
because some contributors are still hostile to any platform that is
not FOSS and outright refuse to do anything to help those platforms.

In the event that it does not work - then what would I do? Pick on the
next person in the list (requiring yet more time)?

Sorry, but that is simply too expensive, and if that is the policy
you'd like to run with, then i'll stick to stripping projects of their
test execution privileges outright across all platforms in the event
they cause issues like this (which if ECM is anything to go by, is
something people won't even notice even if the commit that puts that
in effect is CC'ed to the project mailing list, which means that
nobody will notice when the tests break because the CI system won't be
running them)

Regards,
Ben

>
> On Tue, Nov 5, 2019 at 11:24 AM Ben Cooksley  wrote:
> >
> > On Tue, Nov 5, 2019 at 11:11 PM Harald Sitter  wrote:
> > >
> > > Perhaps you need to find a minion to do these changes for you then or
> > > read up on cmake and/or put these changes through review, because for
> > > KCrash you also disabled and unrelated test :|
> >
> > It would be nice if people would take action to either disable the
> > offending tests or correct the breakage within the tests when I first
> > mention it.
> >
> > Unfortunately as people have not been doing so and because it is
> > causing me issues at the whole-of-KDE level (and therefore inflicting
> > harm on not just this Framework, but on all of KDE by delaying the CI
> > system from completing builds for other projects) i'm forced to take
> > rather heavy handed approaches to resolving the issue, which sometimes
> > has the effect of creating collateral damage (which I consider
> > acceptable in this instance, as the only one damaged is the project
> > that failed to respond).
> >
> > While i'd rather not do this, the cost of not doing so is much
> > greater, so taking a heavy handed approach to projects that fail to
> > take corrective action when corrected will continue to be necessary.
> >
> > The other option of course is to simply terminate providing CI
> > coverage of any form to projects that fail to take corrective action
> > (for all platforms).
> >
> > Regards,
> > Ben
> >
> > >
> > > On Tue, Nov 5, 2019 at 10:24 AM Ben Cooksley  wrote:
> > > >
> > > > On Tue, Nov 5, 2019 at 1:20 AM Harald Sitter  wrote:
> > > > >
> > > > > Wouldn't the more appropriate workaround then be to disable the test 
> > > > > on windows?
> > > >
> > > > If one had the appropriate knowledge of CMake to do so, quite possibly.
> > > >
> > > > Given that I don't however, and others haven't made the necessary
> > > > changes (and nobody has taken action when I have 

D25149: Add a new Template for KCM's.

[Yuri Chornoivan]

yurchor added inline comments.

INLINE COMMENTS

> main.qml:37
> +QQC2.Label {
> +text: i18n("Exemple KCM that does nothing")
> +}

Typo: Exemple -> Example

REPOSITORY
  R242 Plasma Framework (Library)

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

To: tcanabrava
Cc: yurchor, davidedmundson, ognarb, ervin, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, ngraham, bruns

D25149: Add a new Template for KCM's.

[David Edmundson]

davidedmundson added a comment.


  A very sensible idea. ++
  
  > templates/kcm-qml/qml-plasmoid.png
  
  What's this about?

INLINE COMMENTS

> main.qml:26
> +import org.kde.kcm 1.1 as KCM
> +import Qt.labs.platform 1.1
> +

lets not encourage this import

REPOSITORY
  R242 Plasma Framework (Library)

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

To: tcanabrava
Cc: davidedmundson, ognarb, ervin, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D25149: Add a new Template for KCM's.

[Carl Schwan]

ognarb added a comment.


  Another related todo would be to update/rewrite 
https://techbase.kde.org/Development/Tutorials/KCM_HowTo after this template is 
merged and include information about it ;)

REPOSITORY
  R242 Plasma Framework (Library)

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

To: tcanabrava
Cc: ognarb, ervin, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D25067: Fix the header layouts for EntryDetails and Page components

[Dan Leinir Turthra Jensen]

This revision was automatically updated to reflect the committed changes.
Closed by commit R304:4d56727cc7d7: Fix the header layouts for EntryDetails and 
Page components (authored by leinir).

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25067?vs=69319=69330

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

AFFECTED FILES
  src/qtquick/qml/EntryDetails.qml
  src/qtquick/qml/Page.qml

To: leinir, ngraham, #knewstuff, #frameworks, #plasma
Cc: ahiemstra, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

Re: Notice of intention to remove tests from KCrash and KNotifications

[Ben Cooksley]

On Wed, 6 Nov 2019, 01:02 Harald Sitter,  wrote:

> I think on a CI level we can only disable test executation as a whole,
> so that's an even bigger sledgehammer ;)
>

That is correct.

That was the hammer that extra CMake modules was hit with back in 2017,
when a similar issue occurred with a broken test there, and once again
nobody responded when the issue was reported.

In the time since then, numerous tests broke with nobody noticing until I
got a request just recently to reinstate running of tests for ECM as the
offending test had been removed.

So I think that is a hammer that we should definitely avoid using.

Cheers,
Ben


> For KCrash at least we've now implemented the aforementioned
> workaround where the broken test is disabled for windows. For
> knotifications I trust Kai will come up with a solution once he's back
> from QtWS.
>
> On Tue, Nov 5, 2019 at 12:10 PM Adrian Chaves  wrote:
> >
> > Would it make sense to re-enable those tests in the code repositories
> > but disable CI for the corresponding repositories until someone
> > addressed the issues affecting overall CI?
> >
> > On 2019-11-05 11:50, Harald Sitter wrote:
> >
> > > I get where you are coming from, but honestly, none of this makes
> > > pushing unreviewed commits that disable the entire test collection of
> > > an entire framework any more correct. If it had only affected the one
> > > test I wouldn't mind so much, but since it hasn't it only goes to
> > > proof that we have reviews for a reason. In particular for repos that
> > > are part of frameworks where we rely on tests to tell us if the
> > > frameworks are good for the monthly release.
> > >
> > > There are many shades of grey between sending a "someone please fix"
> > > mail to a mailing list and the nuclear option you implemented. The
> > > most notable one being that you can ask someone who worked on the
> > > repo, or tests recently "Hey, X, can you please disable the test on
> > > windows because its impairing CI?" to which the answer is probably yes
> > > because you'd not only address a very specific person but also the
> > > task is doable in less than 5 minutes.
> > > I understand that there's an element of cat herding in this, but
> > > quality must matter for our products, and the quality of frameworks is
> > > very tightly linked to autotesting and reviews because of how we
> > > release them.
> > >
> > > On Tue, Nov 5, 2019 at 11:24 AM Ben Cooksley 
> wrote:
> > > On Tue, Nov 5, 2019 at 11:11 PM Harald Sitter  wrote:
> > > Perhaps you need to find a minion to do these changes for you then or
> > > read up on cmake and/or put these changes through review, because for
> > > KCrash you also disabled and unrelated test :|
> > > It would be nice if people would take action to either disable the
> > > offending tests or correct the breakage within the tests when I first
> > > mention it.
> > >
> > > Unfortunately as people have not been doing so and because it is
> > > causing me issues at the whole-of-KDE level (and therefore inflicting
> > > harm on not just this Framework, but on all of KDE by delaying the CI
> > > system from completing builds for other projects) i'm forced to take
> > > rather heavy handed approaches to resolving the issue, which sometimes
> > > has the effect of creating collateral damage (which I consider
> > > acceptable in this instance, as the only one damaged is the project
> > > that failed to respond).
> > >
> > > While i'd rather not do this, the cost of not doing so is much
> > > greater, so taking a heavy handed approach to projects that fail to
> > > take corrective action when corrected will continue to be necessary.
> > >
> > > The other option of course is to simply terminate providing CI
> > > coverage of any form to projects that fail to take corrective action
> > > (for all platforms).
> > >
> > > Regards,
> > > Ben
> > >
> > > On Tue, Nov 5, 2019 at 10:24 AM Ben Cooksley 
> wrote:
> > > On Tue, Nov 5, 2019 at 1:20 AM Harald Sitter  wrote:
> > > Wouldn't the more appropriate workaround then be to disable the test on
> > > windows?
> > > If one had the appropriate knowledge of CMake to do so, quite possibly.
> > >
> > > Given that I don't however, and others haven't made the necessary
> > > changes (and nobody has taken action when I have mentioned these tests
> > > as causing issues) disabling the tests everywhere is the simplest path
> > > forward and allows the CI system to operate correctly for everyone
> > > rather than be disrupted by these two offending tests.
> > >
> > > Cheers,
> > > Ben
> > >
> > > On Mon, Nov 4, 2019 at 10:57 AM Ben Cooksley 
> wrote:
> > > On Mon, Nov 4, 2019 at 10:53 PM David Edmundson
> > >  wrote:
> > > Given kcrashtest passes locally, can you please confirm that by
> > > "remove" you mean disable and not remove.
> > > I mean remove.
> > >
> > > This test is highly dangerous and enters into a fork loop on Windows,
> > > necessitating use of an administrator level console prompt to recover
> 

D25159: Fix linking to libssh 0.9.1

[Antonio Rojas]

arojas abandoned this revision.

REPOSITORY
  R320 KIO Extras

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

To: arojas, apol, sitter, fvogt
Cc: fvogt, sitter, asn, apol, asturmlechner, kde-frameworks-devel, kfm-devel, 
pberestov, iasensio, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D25159: Fix linking to libssh 0.9.1

[Fabian Vogt]

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


  The libssh maintainer is likely reverting the change, so this should not be 
necessary.

REPOSITORY
  R320 KIO Extras

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

To: arojas, apol, sitter, fvogt
Cc: fvogt, sitter, asn, apol, asturmlechner, kde-frameworks-devel, kfm-devel, 
pberestov, iasensio, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D25159: Fix linking to libssh 0.9.1

[Andreas Schneider]

asn added a comment.


  The plan is to have ssh as the target name and nothing else and support 
BUILD_SHARED_LIBS.

REPOSITORY
  R320 KIO Extras

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

To: arojas, apol, sitter
Cc: sitter, asn, apol, asturmlechner, kde-frameworks-devel, kfm-devel, 
pberestov, iasensio, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D25159: Fix linking to libssh 0.9.1

[Harald Sitter]

sitter added a comment.


  In D25159#559016 , @asn wrote:
  
  > We moved from a manually generated libssh-config.cmake to install(EXPORTS 
libssh-config) and it does things completely different.
  >
  > I'm currently trying to fix it. However better don't apply this as 
ssh_shared will vanish as a target.
  >
  > I wonder how I can define LIBSSH_LIBRARIES again with EXPORTS.
  
  
  I am not super certain, but I don't think you can as that is pretty much 
exactly where configure_package_config_file would be used. We certainly do use 
it that way all over KDE frameworks.
  
  Btw about the ssh_shared target. It may make sense to settle on a target name 
and use that moving forward and advertise it as the recommended way of using 
libssh. IMPORTED targets are vastly preferred over the _LIBRARES/_INCLUDE_DIRS 
variables from a cmake POV because the targets can inject include dirs, flags 
and the likes without the library user having to worry about anything. So they 
are nicer to use in cmake. Just something to think about perhaps.

REPOSITORY
  R320 KIO Extras

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

To: arojas, apol, sitter
Cc: sitter, asn, apol, asturmlechner, kde-frameworks-devel, kfm-devel, 
pberestov, iasensio, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D25159: Fix linking to libssh 0.9.1

[Andreas Schneider]

asn added a comment.


  We moved from a manually generated libssh-config.cmake to install(EXPORTS 
libssh-config) and it does things completely different.
  
  I'm currently trying to fix it. However better don't apply this as ssh_shared 
will vanish as a target.
  
  I wonder how I can define LIBSSH_LIBRARIES again with EXPORTS.

REPOSITORY
  R320 KIO Extras

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

To: arojas, apol, sitter
Cc: sitter, asn, apol, asturmlechner, kde-frameworks-devel, kfm-devel, 
pberestov, iasensio, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D25159: Fix linking to libssh 0.9.1

[Harald Sitter]

sitter added subscribers: asn, sitter.
sitter accepted this revision.
sitter added a comment.


  I think there's a smarter way of dealing with this somewhere in our finder, 
not blocking though. I'll take a look when I find a minute.
  
  @asn it seems to me the cmake config broke compat between 0.9.0 and 0.9.1 :|

REPOSITORY
  R320 KIO Extras

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

To: arojas, apol, sitter
Cc: sitter, asn, apol, asturmlechner, kde-frameworks-devel, kfm-devel, 
pberestov, iasensio, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

KDE CI: Frameworks » knotifications » kf5-qt5 WindowsMSVCQt5.13 - Build # 31 - Fixed!

[CI System]

BUILD SUCCESS
 Build URL
https://build.kde.org/job/Frameworks/job/knotifications/job/kf5-qt5%20WindowsMSVCQt5.13/31/
 Project:
kf5-qt5 WindowsMSVCQt5.13
 Date of build:
Tue, 05 Nov 2019 09:26:14 +
 Build duration:
6 hr 41 min and counting

KDE CI: Frameworks » kcrash » kf5-qt5 WindowsMSVCQt5.13 - Build # 12 - Fixed!

[CI System]

BUILD SUCCESS
 Build URL
https://build.kde.org/job/Frameworks/job/kcrash/job/kf5-qt5%20WindowsMSVCQt5.13/12/
 Project:
kf5-qt5 WindowsMSVCQt5.13
 Date of build:
Tue, 05 Nov 2019 09:06:58 +
 Build duration:
6 hr 58 min and counting
   JUnit Tests
  Name: projectroot Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)

D25159: Fix linking to libssh 0.9.1

[Aleix Pol Gonzalez]

apol accepted this revision.
apol added a comment.
This revision is now accepted and ready to land.


  Fixes the build for me too, patch looks good.
  
  Thanks!

REPOSITORY
  R320 KIO Extras

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

To: arojas, apol
Cc: apol, asturmlechner, kde-frameworks-devel, kfm-devel, pberestov, iasensio, 
fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D25159: Fix linking to libssh 0.9.1

[Antonio Rojas]

arojas retitled this revision from "Fix linking to libssh 0.9" to "Fix linking 
to libssh 0.9.1".
arojas edited the summary of this revision.
arojas edited the test plan for this revision.

REPOSITORY
  R320 KIO Extras

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

To: arojas
Cc: asturmlechner, kde-frameworks-devel, kfm-devel, pberestov, iasensio, 
fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D25159: Fix linking to libssh 0.9

[Antonio Rojas]

arojas created this revision.
Herald added projects: Dolphin, Frameworks.
Herald added subscribers: kfm-devel, kde-frameworks-devel.
arojas requested review of this revision.

REVISION SUMMARY
  libssh 0.9 no longer defines LIBSSH_LIBRARIES in its cmake config, and 
exports a ssh_shared target instead.

TEST PLAN
  Builds against libssh 0.8 and 0.9

REPOSITORY
  R320 KIO Extras

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

AFFECTED FILES
  sftp/CMakeLists.txt

To: arojas
Cc: kde-frameworks-devel, kfm-devel, pberestov, iasensio, fprice, LeGast00n, 
MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, 
spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, 
mikesomov

D25067: Fix the header layouts for EntryDetails and Page components

[Nathaniel Graham]

ngraham accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R304 KNewStuff

BRANCH
  fix-page-and-entrydetails-headers (branched from master)

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

To: leinir, ngraham, #knewstuff, #frameworks, #plasma
Cc: ahiemstra, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24825: Add hideOnWindowDeactivate to PlasmaComponents.Dialog

[Konrad Materka]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:96b7fc21f5b5: Add hideOnWindowDeactivate to 
PlasmaComponents.Dialog (authored by kmaterka).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24825?vs=68424=69323

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

AFFECTED FILES
  src/declarativeimports/plasmacomponents/qml/Dialog.qml

To: kmaterka, #plasma, #frameworks, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

KDE CI: Frameworks » kcmutils » kf5-qt5 WindowsMSVCQt5.13 - Build # 28 - Fixed!

[CI System]

BUILD SUCCESS
 Build URL
https://build.kde.org/job/Frameworks/job/kcmutils/job/kf5-qt5%20WindowsMSVCQt5.13/28/
 Project:
kf5-qt5 WindowsMSVCQt5.13
 Date of build:
Mon, 04 Nov 2019 14:43:52 +
 Build duration:
22 hr and counting

D24825: Add hideOnWindowDeactivate to PlasmaComponents.Dialog

[David Edmundson]

davidedmundson added a comment.


  > maybe there is no point in fixing this?
  
  PlasmaComponents are dead in favour of Plasma Components 3 (which have to 
match QtQuickControls2 API)
  
  Maybe not, from what I can see the original code could use PlasmaCore.Dialog 
directly. Or the inbuilt way plasma does popups.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

To: kmaterka, #plasma, #frameworks, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24825: Add hideOnWindowDeactivate to PlasmaComponents.Dialog

[David Edmundson]

davidedmundson accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

To: kmaterka, #plasma, #frameworks, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24825: Add hideOnWindowDeactivate to PlasmaComponents.Dialog

[Konrad Materka]

kmaterka added a comment.


  Should I proceed? This component has serious issues anyway, maybe there is no 
point in fixing this?
  For example:
  There is Loader that load always the same QML because... "if (true || )" 
- line 241. Even if this is obsolete, cleanup would be good idea.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25067: Fix the header layouts for EntryDetails and Page components

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 69319.
leinir marked 6 inline comments as done.
leinir added a comment.


  - Remove the custom title delegate on EntryDetails

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25067?vs=69281=69319

BRANCH
  fix-page-and-entrydetails-headers (branched from master)

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

AFFECTED FILES
  src/qtquick/qml/EntryDetails.qml
  src/qtquick/qml/Page.qml

To: leinir, ngraham, #knewstuff, #frameworks, #plasma
Cc: ahiemstra, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25067: Fix the header layouts for EntryDetails and Page components

[Dan Leinir Turthra Jensen]

leinir added inline comments.

INLINE COMMENTS

> ahiemstra wrote in EntryDetails.qml:94
> Hmm, with the new toolbar code, there's actually little reason to use a 
> custom delegate here, since the toolbar header already uses a title + tool 
> buttons style. You could convert the three toolbuttons to actions and drop 
> the rest of the delegate.

Ooh... you know, that's a very good idea :)

It'd be even better if we could work out how to do that with the bits on the 
front page as well... this /is/ intended to work for convergence and whatnot - 
but let's poke at that after we've got this bit sorted and such :)

REPOSITORY
  R304 KNewStuff

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

To: leinir, ngraham, #knewstuff, #frameworks, #plasma
Cc: ahiemstra, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25143: Basic cross-compiling support for parsetrigrams

[Christoph Cullmann]

This revision was automatically updated to reflect the committed changes.
Closed by commit R246:1ada4bec82ea: Basic cross-compiling support for 
parsetrigrams (authored by vkrause, committed by cullmann).

REPOSITORY
  R246 Sonnet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25143?vs=69285=69317

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

AFFECTED FILES
  CMakeLists.txt
  data/CMakeLists.txt
  src/core/CMakeLists.txt

To: vkrause, cullmann, apol
Cc: apol, cullmann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

Re: Notice of intention to remove tests from KCrash and KNotifications

[Harald Sitter]

I think on a CI level we can only disable test executation as a whole,
so that's an even bigger sledgehammer ;)

For KCrash at least we've now implemented the aforementioned
workaround where the broken test is disabled for windows. For
knotifications I trust Kai will come up with a solution once he's back
from QtWS.

On Tue, Nov 5, 2019 at 12:10 PM Adrian Chaves  wrote:
>
> Would it make sense to re-enable those tests in the code repositories
> but disable CI for the corresponding repositories until someone
> addressed the issues affecting overall CI?
>
> On 2019-11-05 11:50, Harald Sitter wrote:
>
> > I get where you are coming from, but honestly, none of this makes
> > pushing unreviewed commits that disable the entire test collection of
> > an entire framework any more correct. If it had only affected the one
> > test I wouldn't mind so much, but since it hasn't it only goes to
> > proof that we have reviews for a reason. In particular for repos that
> > are part of frameworks where we rely on tests to tell us if the
> > frameworks are good for the monthly release.
> >
> > There are many shades of grey between sending a "someone please fix"
> > mail to a mailing list and the nuclear option you implemented. The
> > most notable one being that you can ask someone who worked on the
> > repo, or tests recently "Hey, X, can you please disable the test on
> > windows because its impairing CI?" to which the answer is probably yes
> > because you'd not only address a very specific person but also the
> > task is doable in less than 5 minutes.
> > I understand that there's an element of cat herding in this, but
> > quality must matter for our products, and the quality of frameworks is
> > very tightly linked to autotesting and reviews because of how we
> > release them.
> >
> > On Tue, Nov 5, 2019 at 11:24 AM Ben Cooksley  wrote:
> > On Tue, Nov 5, 2019 at 11:11 PM Harald Sitter  wrote:
> > Perhaps you need to find a minion to do these changes for you then or
> > read up on cmake and/or put these changes through review, because for
> > KCrash you also disabled and unrelated test :|
> > It would be nice if people would take action to either disable the
> > offending tests or correct the breakage within the tests when I first
> > mention it.
> >
> > Unfortunately as people have not been doing so and because it is
> > causing me issues at the whole-of-KDE level (and therefore inflicting
> > harm on not just this Framework, but on all of KDE by delaying the CI
> > system from completing builds for other projects) i'm forced to take
> > rather heavy handed approaches to resolving the issue, which sometimes
> > has the effect of creating collateral damage (which I consider
> > acceptable in this instance, as the only one damaged is the project
> > that failed to respond).
> >
> > While i'd rather not do this, the cost of not doing so is much
> > greater, so taking a heavy handed approach to projects that fail to
> > take corrective action when corrected will continue to be necessary.
> >
> > The other option of course is to simply terminate providing CI
> > coverage of any form to projects that fail to take corrective action
> > (for all platforms).
> >
> > Regards,
> > Ben
> >
> > On Tue, Nov 5, 2019 at 10:24 AM Ben Cooksley  wrote:
> > On Tue, Nov 5, 2019 at 1:20 AM Harald Sitter  wrote:
> > Wouldn't the more appropriate workaround then be to disable the test on
> > windows?
> > If one had the appropriate knowledge of CMake to do so, quite possibly.
> >
> > Given that I don't however, and others haven't made the necessary
> > changes (and nobody has taken action when I have mentioned these tests
> > as causing issues) disabling the tests everywhere is the simplest path
> > forward and allows the CI system to operate correctly for everyone
> > rather than be disrupted by these two offending tests.
> >
> > Cheers,
> > Ben
> >
> > On Mon, Nov 4, 2019 at 10:57 AM Ben Cooksley  wrote:
> > On Mon, Nov 4, 2019 at 10:53 PM David Edmundson
> >  wrote:
> > Given kcrashtest passes locally, can you please confirm that by
> > "remove" you mean disable and not remove.
> > I mean remove.
> >
> > This test is highly dangerous and enters into a fork loop on Windows,
> > necessitating use of an administrator level console prompt to recover
> > the system.
> > Fortunately the grand-parent process terminates after it's child has
> > successfully forked, which is the only thing stopping this test from
> > being a fork bomb and totally taking down the system.
> >
> > David
> > Regards,
> > Ben

D24843: [KDEPlatformSystemTrayIcon] Recreate deleted menu

[Konrad Materka]

This revision was automatically updated to reflect the committed changes.
Closed by commit R135:bc1c85144adb: [KDEPlatformSystemTrayIcon] Recreate 
deleted menu (authored by kmaterka).

REPOSITORY
  R135 Integration for Qt applications in Plasma

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24843?vs=68482=69313

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kdeplatformsystemtrayicon_unittest.cpp
  src/platformtheme/kdeplatformsystemtrayicon.cpp
  src/platformtheme/kdeplatformsystemtrayicon.h

To: kmaterka, apol, davidedmundson, #plasma, #frameworks, broulik, nicolasfella
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25153: disable kcrashtest and its helper executable test_crasher on windows

[Harald Sitter]

This revision was automatically updated to reflect the committed changes.
Closed by commit R285:d3b83d931176: disable kcrashtest and its helper 
executable test_crasher on windows (authored by sitter).

REPOSITORY
  R285 KCrash

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25153?vs=69304=69311

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

AFFECTED FILES
  autotests/CMakeLists.txt

To: sitter, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25154: actually set a dependency between kcrashtest and test_crasher

[Harald Sitter]

This revision was automatically updated to reflect the committed changes.
Closed by commit R285:cc0d3c8b7977: actually set a dependency between 
kcrashtest and test_crasher (authored by sitter).

REPOSITORY
  R285 KCrash

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25154?vs=69305=69312

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

AFFECTED FILES
  autotests/CMakeLists.txt

To: sitter, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25143: Basic cross-compiling support for parsetrigrams

[Aleix Pol Gonzalez]

apol accepted this revision.
apol added a comment.


  LGTM, thanks for looking into this!

REPOSITORY
  R246 Sonnet

BRANCH
  master

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

To: vkrause, cullmann, apol
Cc: apol, cullmann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D24843: [KDEPlatformSystemTrayIcon] Recreate deleted menu

[Aleix Pol Gonzalez]

apol accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R135 Integration for Qt applications in Plasma

BRANCH
  master

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

To: kmaterka, apol, davidedmundson, #plasma, #frameworks, broulik, nicolasfella
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25154: actually set a dependency between kcrashtest and test_crasher

[Aleix Pol Gonzalez]

apol accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R285 KCrash

BRANCH
  test-dep

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

To: sitter, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25153: disable kcrashtest and its helper executable test_crasher on windows

[Aleix Pol Gonzalez]

apol accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R285 KCrash

BRANCH
  master

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

To: sitter, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25067: Fix the header layouts for EntryDetails and Page components

[Arjen Hiemstra]

ahiemstra added inline comments.

INLINE COMMENTS

> EntryDetails.qml:94
>  title: i18nc("Combined title for the entry details page made of the name 
> of the entry, and the author's name", "%1 by 
> %2").arg(component.name).arg(entryAuthor.name)
>  titleDelegate: QtLayouts.RowLayout {
> +QtLayouts.Layout.fillWidth: true

Hmm, with the new toolbar code, there's actually little reason to use a custom 
delegate here, since the toolbar header already uses a title + tool buttons 
style. You could convert the three toolbuttons to actions and drop the rest of 
the delegate.

REPOSITORY
  R304 KNewStuff

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

To: leinir, ngraham, #knewstuff, #frameworks, #plasma
Cc: ahiemstra, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

Re: Notice of intention to remove tests from KCrash and KNotifications

[Adrian Chaves]

Would it make sense to re-enable those tests in the code repositories
but disable CI for the corresponding repositories until someone
addressed the issues affecting overall CI?

On 2019-11-05 11:50, Harald Sitter wrote:


I get where you are coming from, but honestly, none of this makes
pushing unreviewed commits that disable the entire test collection of
an entire framework any more correct. If it had only affected the one
test I wouldn't mind so much, but since it hasn't it only goes to
proof that we have reviews for a reason. In particular for repos that
are part of frameworks where we rely on tests to tell us if the
frameworks are good for the monthly release.

There are many shades of grey between sending a "someone please fix"
mail to a mailing list and the nuclear option you implemented. The
most notable one being that you can ask someone who worked on the
repo, or tests recently "Hey, X, can you please disable the test on
windows because its impairing CI?" to which the answer is probably yes
because you'd not only address a very specific person but also the
task is doable in less than 5 minutes.
I understand that there's an element of cat herding in this, but
quality must matter for our products, and the quality of frameworks is
very tightly linked to autotesting and reviews because of how we
release them.

On Tue, Nov 5, 2019 at 11:24 AM Ben Cooksley  wrote:
On Tue, Nov 5, 2019 at 11:11 PM Harald Sitter  wrote:
Perhaps you need to find a minion to do these changes for you then or
read up on cmake and/or put these changes through review, because for
KCrash you also disabled and unrelated test :|
It would be nice if people would take action to either disable the
offending tests or correct the breakage within the tests when I first
mention it.

Unfortunately as people have not been doing so and because it is
causing me issues at the whole-of-KDE level (and therefore inflicting
harm on not just this Framework, but on all of KDE by delaying the CI
system from completing builds for other projects) i'm forced to take
rather heavy handed approaches to resolving the issue, which sometimes
has the effect of creating collateral damage (which I consider
acceptable in this instance, as the only one damaged is the project
that failed to respond).

While i'd rather not do this, the cost of not doing so is much
greater, so taking a heavy handed approach to projects that fail to
take corrective action when corrected will continue to be necessary.

The other option of course is to simply terminate providing CI
coverage of any form to projects that fail to take corrective action
(for all platforms).

Regards,
Ben

On Tue, Nov 5, 2019 at 10:24 AM Ben Cooksley  wrote:
On Tue, Nov 5, 2019 at 1:20 AM Harald Sitter  wrote:
Wouldn't the more appropriate workaround then be to disable the test on 
windows?

If one had the appropriate knowledge of CMake to do so, quite possibly.

Given that I don't however, and others haven't made the necessary
changes (and nobody has taken action when I have mentioned these tests
as causing issues) disabling the tests everywhere is the simplest path
forward and allows the CI system to operate correctly for everyone
rather than be disrupted by these two offending tests.

Cheers,
Ben

On Mon, Nov 4, 2019 at 10:57 AM Ben Cooksley  wrote:
On Mon, Nov 4, 2019 at 10:53 PM David Edmundson
 wrote:
Given kcrashtest passes locally, can you please confirm that by
"remove" you mean disable and not remove.
I mean remove.

This test is highly dangerous and enters into a fork loop on Windows,
necessitating use of an administrator level console prompt to recover
the system.
Fortunately the grand-parent process terminates after it's child has
successfully forked, which is the only thing stopping this test from
being a fork bomb and totally taking down the system.

David
Regards,
Ben

D25152: Reenable the tests and only disable krcrashtest on windows

[Ahmad Samir]

ahmadsamir added a comment.


  In D25152#558886 , @sitter wrote:
  
  > Heh, sorry 
  
  
  Don't be sorry, yours is actually a fix, mine isn't/wasn't :D
  
  > D25153  for reference.

REPOSITORY
  R285 KCrash

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

To: ahmadsamir, #frameworks, sitter
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25152: Reenable the tests and only disable krcrashtest on windows

[Harald Sitter]

sitter added a comment.


  Heh, sorry 
  
  D25153  for reference.

REPOSITORY
  R285 KCrash

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

To: ahmadsamir, #frameworks, sitter
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

Re: Notice of intention to remove tests from KCrash and KNotifications

[Harald Sitter]

I get where you are coming from, but honestly, none of this makes
pushing unreviewed commits that disable the entire test collection of
an entire framework any more correct. If it had only affected the one
test I wouldn't mind so much, but since it hasn't it only goes to
proof that we have reviews for a reason. In particular for repos that
are part of frameworks where we rely on tests to tell us if the
frameworks are good for the monthly release.

There are many shades of grey between sending a "someone please fix"
mail to a mailing list and the nuclear option you implemented. The
most notable one being that you can ask someone who worked on the
repo, or tests recently "Hey, X, can you please disable the test on
windows because its impairing CI?" to which the answer is probably yes
because you'd not only address a very specific person but also the
task is doable in less than 5 minutes.
I understand that there's an element of cat herding in this, but
quality must matter for our products, and the quality of frameworks is
very tightly linked to autotesting and reviews because of how we
release them.

On Tue, Nov 5, 2019 at 11:24 AM Ben Cooksley  wrote:
>
> On Tue, Nov 5, 2019 at 11:11 PM Harald Sitter  wrote:
> >
> > Perhaps you need to find a minion to do these changes for you then or
> > read up on cmake and/or put these changes through review, because for
> > KCrash you also disabled and unrelated test :|
>
> It would be nice if people would take action to either disable the
> offending tests or correct the breakage within the tests when I first
> mention it.
>
> Unfortunately as people have not been doing so and because it is
> causing me issues at the whole-of-KDE level (and therefore inflicting
> harm on not just this Framework, but on all of KDE by delaying the CI
> system from completing builds for other projects) i'm forced to take
> rather heavy handed approaches to resolving the issue, which sometimes
> has the effect of creating collateral damage (which I consider
> acceptable in this instance, as the only one damaged is the project
> that failed to respond).
>
> While i'd rather not do this, the cost of not doing so is much
> greater, so taking a heavy handed approach to projects that fail to
> take corrective action when corrected will continue to be necessary.
>
> The other option of course is to simply terminate providing CI
> coverage of any form to projects that fail to take corrective action
> (for all platforms).
>
> Regards,
> Ben
>
> >
> > On Tue, Nov 5, 2019 at 10:24 AM Ben Cooksley  wrote:
> > >
> > > On Tue, Nov 5, 2019 at 1:20 AM Harald Sitter  wrote:
> > > >
> > > > Wouldn't the more appropriate workaround then be to disable the test on 
> > > > windows?
> > >
> > > If one had the appropriate knowledge of CMake to do so, quite possibly.
> > >
> > > Given that I don't however, and others haven't made the necessary
> > > changes (and nobody has taken action when I have mentioned these tests
> > > as causing issues) disabling the tests everywhere is the simplest path
> > > forward and allows the CI system to operate correctly for everyone
> > > rather than be disrupted by these two offending tests.
> > >
> > > Cheers,
> > > Ben
> > >
> > > >
> > > > On Mon, Nov 4, 2019 at 10:57 AM Ben Cooksley  wrote:
> > > > >
> > > > > On Mon, Nov 4, 2019 at 10:53 PM David Edmundson
> > > > >  wrote:
> > > > > >
> > > > > > Given kcrashtest passes locally, can you please confirm that by
> > > > > > "remove" you mean disable and not remove.
> > > > >
> > > > > I mean remove.
> > > > >
> > > > > This test is highly dangerous and enters into a fork loop on Windows,
> > > > > necessitating use of an administrator level console prompt to recover
> > > > > the system.
> > > > > Fortunately the grand-parent process terminates after it's child has
> > > > > successfully forked, which is the only thing stopping this test from
> > > > > being a fork bomb and totally taking down the system.
> > > > >
> > > > > >
> > > > > > David
> > > > >
> > > > > Regards,
> > > > > Ben

D25067: Fix the header layouts for EntryDetails and Page components

[Dan Leinir Turthra Jensen]

leinir added a comment.


  In D25067#558557 , @ngraham wrote:
  
  > With that Kirigami patch, this *almost* works. There's still an empty area 
on the right:
  >
  > F7735292: Screenshot_20191104_092108.png 

  >
  > F7735295: Screenshot_20191104_092215.png 

  
  
  The other patch updated and whatnot, and it now looks like so:
  
  F7740484: image.png 
  
  F7740480: image.png 
  
  In other words, as it should! :) (and with much simplified code as well, 
which is always a bonus ;) )

REPOSITORY
  R304 KNewStuff

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

To: leinir, ngraham, #knewstuff, #frameworks, #plasma
Cc: ahiemstra, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24979: KRunner: port away from deprecated KF5 API

[Friedrich W. H. Kossebau]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R308:7bbd29591094: KRunner: port away from deprecated KF5 API 
(authored by dfaure, committed by kossebau).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D24979?vs=69242=69307#toc

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24979?vs=69242=69307

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

AFFECTED FILES
  CMakeLists.txt
  src/abstractrunner.h

To: kossebau, mart, apol, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25154: actually set a dependency between kcrashtest and test_crasher

[Harald Sitter]

sitter created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
sitter requested review of this revision.

REVISION SUMMARY
  the former uses the latter to actualy conduct its test. without the
  declared dependency `make kcrashtest` would not produce a working
  result because it'd be missing the test_crasher helper

TEST PLAN
  `make kcrashtest` now builds both targets

REPOSITORY
  R285 KCrash

BRANCH
  test-dep

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

AFFECTED FILES
  autotests/CMakeLists.txt

To: sitter
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25152: Reenable the tests and only disable krcrashtest on windows

[Ahmad Samir]

ahmadsamir abandoned this revision.
ahmadsamir added a comment.


  @sitter: I see you have a fix already :)

REPOSITORY
  R285 KCrash

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

To: ahmadsamir, #frameworks, sitter
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

Re: Notice of intention to remove tests from KCrash and KNotifications

[Ben Cooksley]

On Tue, Nov 5, 2019 at 11:11 PM Harald Sitter  wrote:
>
> Perhaps you need to find a minion to do these changes for you then or
> read up on cmake and/or put these changes through review, because for
> KCrash you also disabled and unrelated test :|

It would be nice if people would take action to either disable the
offending tests or correct the breakage within the tests when I first
mention it.

Unfortunately as people have not been doing so and because it is
causing me issues at the whole-of-KDE level (and therefore inflicting
harm on not just this Framework, but on all of KDE by delaying the CI
system from completing builds for other projects) i'm forced to take
rather heavy handed approaches to resolving the issue, which sometimes
has the effect of creating collateral damage (which I consider
acceptable in this instance, as the only one damaged is the project
that failed to respond).

While i'd rather not do this, the cost of not doing so is much
greater, so taking a heavy handed approach to projects that fail to
take corrective action when corrected will continue to be necessary.

The other option of course is to simply terminate providing CI
coverage of any form to projects that fail to take corrective action
(for all platforms).

Regards,
Ben

>
> On Tue, Nov 5, 2019 at 10:24 AM Ben Cooksley  wrote:
> >
> > On Tue, Nov 5, 2019 at 1:20 AM Harald Sitter  wrote:
> > >
> > > Wouldn't the more appropriate workaround then be to disable the test on 
> > > windows?
> >
> > If one had the appropriate knowledge of CMake to do so, quite possibly.
> >
> > Given that I don't however, and others haven't made the necessary
> > changes (and nobody has taken action when I have mentioned these tests
> > as causing issues) disabling the tests everywhere is the simplest path
> > forward and allows the CI system to operate correctly for everyone
> > rather than be disrupted by these two offending tests.
> >
> > Cheers,
> > Ben
> >
> > >
> > > On Mon, Nov 4, 2019 at 10:57 AM Ben Cooksley  wrote:
> > > >
> > > > On Mon, Nov 4, 2019 at 10:53 PM David Edmundson
> > > >  wrote:
> > > > >
> > > > > Given kcrashtest passes locally, can you please confirm that by
> > > > > "remove" you mean disable and not remove.
> > > >
> > > > I mean remove.
> > > >
> > > > This test is highly dangerous and enters into a fork loop on Windows,
> > > > necessitating use of an administrator level console prompt to recover
> > > > the system.
> > > > Fortunately the grand-parent process terminates after it's child has
> > > > successfully forked, which is the only thing stopping this test from
> > > > being a fork bomb and totally taking down the system.
> > > >
> > > > >
> > > > > David
> > > >
> > > > Regards,
> > > > Ben

D25153: disable kcrashtest and its helper executable test_crasher on windows

[Harald Sitter]

sitter created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
sitter requested review of this revision.

REVISION SUMMARY
  test_crasher gets stuck in a loop breaking the CI system. let's disable
  the entire test until someone finds the interest to investigate a fix

TEST PLAN
  still builds and runs on linux

REPOSITORY
  R285 KCrash

BRANCH
  master

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

AFFECTED FILES
  autotests/CMakeLists.txt

To: sitter
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24979: KRunner: port away from deprecated KF5 API

[Friedrich W. H. Kossebau]

kossebau commandeered this revision.
kossebau edited reviewers, added: dfaure; removed: kossebau.
kossebau added a comment.
This revision now requires review to proceed.


  @dfaure I allow myself to take over here given your are off the next days and 
I would like to get this off the table :)

REPOSITORY
  R308 KRunner

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

To: kossebau, mart, apol, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25152: Reenable the tests and only disable krcrashtest on windows

[Ahmad Samir]

ahmadsamir created this revision.
ahmadsamir added reviewers: Frameworks, sitter.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

TEST PLAN
  Code builds and tests pass on Linux

REPOSITORY
  R285 KCrash

BRANCH
  l-win-test (branched from master)

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kcrashtest.cpp

To: ahmadsamir, #frameworks, sitter
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

Re: Notice of intention to remove tests from KCrash and KNotifications

[Harald Sitter]

Perhaps you need to find a minion to do these changes for you then or
read up on cmake and/or put these changes through review, because for
KCrash you also disabled and unrelated test :|

On Tue, Nov 5, 2019 at 10:24 AM Ben Cooksley  wrote:
>
> On Tue, Nov 5, 2019 at 1:20 AM Harald Sitter  wrote:
> >
> > Wouldn't the more appropriate workaround then be to disable the test on 
> > windows?
>
> If one had the appropriate knowledge of CMake to do so, quite possibly.
>
> Given that I don't however, and others haven't made the necessary
> changes (and nobody has taken action when I have mentioned these tests
> as causing issues) disabling the tests everywhere is the simplest path
> forward and allows the CI system to operate correctly for everyone
> rather than be disrupted by these two offending tests.
>
> Cheers,
> Ben
>
> >
> > On Mon, Nov 4, 2019 at 10:57 AM Ben Cooksley  wrote:
> > >
> > > On Mon, Nov 4, 2019 at 10:53 PM David Edmundson
> > >  wrote:
> > > >
> > > > Given kcrashtest passes locally, can you please confirm that by
> > > > "remove" you mean disable and not remove.
> > >
> > > I mean remove.
> > >
> > > This test is highly dangerous and enters into a fork loop on Windows,
> > > necessitating use of an administrator level console prompt to recover
> > > the system.
> > > Fortunately the grand-parent process terminates after it's child has
> > > successfully forked, which is the only thing stopping this test from
> > > being a fork bomb and totally taking down the system.
> > >
> > > >
> > > > David
> > >
> > > Regards,
> > > Ben

Re: Notice of intention to remove tests from KCrash and KNotifications

[Ben Cooksley]

On Tue, Nov 5, 2019 at 1:20 AM Harald Sitter  wrote:
>
> Wouldn't the more appropriate workaround then be to disable the test on 
> windows?

If one had the appropriate knowledge of CMake to do so, quite possibly.

Given that I don't however, and others haven't made the necessary
changes (and nobody has taken action when I have mentioned these tests
as causing issues) disabling the tests everywhere is the simplest path
forward and allows the CI system to operate correctly for everyone
rather than be disrupted by these two offending tests.

Cheers,
Ben

>
> On Mon, Nov 4, 2019 at 10:57 AM Ben Cooksley  wrote:
> >
> > On Mon, Nov 4, 2019 at 10:53 PM David Edmundson
> >  wrote:
> > >
> > > Given kcrashtest passes locally, can you please confirm that by
> > > "remove" you mean disable and not remove.
> >
> > I mean remove.
> >
> > This test is highly dangerous and enters into a fork loop on Windows,
> > necessitating use of an administrator level console prompt to recover
> > the system.
> > Fortunately the grand-parent process terminates after it's child has
> > successfully forked, which is the only thing stopping this test from
> > being a fork bomb and totally taking down the system.
> >
> > >
> > > David
> >
> > Regards,
> > Ben

KDE CI: Frameworks » kcrash » kf5-qt5 WindowsMSVCQt5.13 - Build # 11 - Still unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kcrash/job/kf5-qt5%20WindowsMSVCQt5.13/11/
 Project:
kf5-qt5 WindowsMSVCQt5.13
 Date of build:
Mon, 04 Nov 2019 09:01:16 +
 Build duration:
1 day 0 hr and counting
   JUnit Tests
  Name: projectroot Failed: 1 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 2 test(s)Failed: projectroot.autotests.kcrashtest