D19092: Add bison minimum version of 2.4.1 due to %code

[Kurt Hindenburg]

This revision was automatically updated to reflect the committed changes.
Closed by commit R309:6b46411622b0: Add bison minimum version of 2.4.1 due to 
%code (authored by hindenburg).

REPOSITORY
  R309 KService

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19092?vs=51902=55031

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

AFFECTED FILES
  CMakeLists.txt

To: hindenburg, #frameworks, winterz
Cc: pino, ltoscano, winterz, kde-frameworks-devel, michaelh, ngraham, bruns

D19887: KFileItem: call stat() on demand, add SkipMimeTypeDetermination option

[Robert Hoffmann]

hoffmannrobert updated this revision to Diff 55033.
hoffmannrobert marked 3 inline comments as done.
hoffmannrobert added a comment.


  - Add ensureInitialized(), entry() changes

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19887?vs=55025=55033

BRANCH
  add_skipStat

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

AFFECTED FILES
  autotests/kfileitemtest.cpp
  autotests/kfileitemtest.h
  src/core/kfileitem.cpp
  src/core/kfileitem.h

To: hoffmannrobert, dfaure, #frameworks, #dolphin
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D19887: KFileItem: call stat() on demand, add SkipMimeTypeDetermination option

[Robert Hoffmann]

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

INLINE COMMENTS

> dfaure wrote in kfileitem.cpp:362
> There is one thing I've been wondering back and forth about: the alternative 
> way of doing this, which is to call init() unconditionally and testing the 
> bool first thing in there.
> The benefit is that it would reduce the code size overall, and greatly 
> simplify the top of this method (two calls to init, done).
> The downside is that for the reader, a call to init() might in fact do 
> nothing (and it reads like it's doing something, until going there to check). 
> So it's maybeInit(), urgh.
> 
> Ah, in another code base (QMimeType) I wrote ensureLoaded(). This could be 
> ensureInitialized() ?
> 
> Sorry for not suggesting this earlier. Do you agree that it would make the 
> code better?

Yes, I agree, but here you still need the two ifs, and the init() still is 
needed because of refresh().

> dfaure wrote in kfileitem.cpp:609
> No no, this won't work. entry() returns a copy, not a reference.

Ah, sorry, yes.

> dfaure wrote in kfileitem.cpp:663
> here entry() would work, no?

No, it's KFileItemPrivate.

REPOSITORY
  R241 KIO

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

To: hoffmannrobert, dfaure, #frameworks, #dolphin
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

KDE CI: Frameworks » kservice » kf5-qt5 FreeBSDQt5.12 - Build # 17 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kservice/job/kf5-qt5%20FreeBSDQt5.12/17/
 Project:
kf5-qt5 FreeBSDQt5.12
 Date of build:
Fri, 29 Mar 2019 14:02:13 +
 Build duration:
1 min 22 sec and counting
   JUnit Tests
  Name: projectroot Failed: 2 test(s), Passed: 7 test(s), Skipped: 0 test(s), Total: 9 test(s)Failed: projectroot.autotests.kmimeassociationstestFailed: projectroot.autotests.ksycoca_xdgdirstestName: projectroot.tests Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)

D20032: Convert string formatting tests to be data driven

[Aleix Pol Gonzalez]

apol added a subscriber: mgallien.
apol added inline comments.

INLINE COMMENTS

> bruns wrote in propertyinfotest.cpp:71
> Still not convinced?

Not really, but if @mgallien, who is the maintainer, is fine with it, I'm fine 
too.
After all it's still better than what we used to have.

REPOSITORY
  R286 KFileMetaData

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

To: bruns, #baloo, #frameworks, ngraham, astippich
Cc: mgallien, apol, kde-frameworks-devel, gennad, domson, ashaposhnikov, 
michaelh, astippich, spoorun, ngraham, bruns, abrahams

D20032: Convert string formatting tests to be data driven

[Stefan Brüns]

bruns added a comment.


  @apol - do you have a proposal how to avoid the "weird struct" and still keep 
it readable? Preferably, no repetition of the Property, and no extra explicit 
constructors.

REPOSITORY
  R286 KFileMetaData

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

To: bruns, #baloo, #frameworks, ngraham, astippich
Cc: apol, kde-frameworks-devel, gennad, domson, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D19887: KFileItem: call stat() on demand, add SkipMimeTypeDetermination option

[David Faure]

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


  Thanks! Looks nice now.
  
  I don't see you in the list of developer accounts, so I guess you need 
someone to push this for you?

REPOSITORY
  R241 KIO

BRANCH
  add_skipStat

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

To: hoffmannrobert, dfaure, #frameworks, #dolphin
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D19887: KFileItem: call stat() on demand, add SkipMimeTypeDetermination option

[Robert Hoffmann]

hoffmannrobert marked 3 inline comments as done.
hoffmannrobert added a comment.


  Yes, please, and https://phabricator.kde.org/D19784 too.

REPOSITORY
  R241 KIO

BRANCH
  add_skipStat

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

To: hoffmannrobert, dfaure, #frameworks, #dolphin
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D19764: Fix Minimap with QtCurve style

[Kåre Särs]

sars added reviewers: dhaumann, cullmann.

REPOSITORY
  R39 KTextEditor

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

To: sars, brauch, dhaumann, cullmann
Cc: kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, ngraham, 
bruns, demsking, cullmann, sars, dhaumann

Re: CI system maintainability

[Ben Cooksley]

On Fri, Mar 29, 2019 at 10:33 PM Friedrich W. H. Kossebau
 wrote:
>
> Am Donnerstag, 28. März 2019, 23:06:17 CET schrieb laurent Montel:
> > Le jeudi 28 mars 2019, 18:27:42 CET Friedrich W. H. Kossebau a écrit :
> > > Am Donnerstag, 28. März 2019, 16:56:33 CET schrieb laurent Montel:
> > > > Le jeudi 28 mars 2019, 16:11:12 CET Friedrich W. H. Kossebau a écrit :
> > > > > Given the actual purpose of this thread, I would be more curious how
> > > > > you
> > > > > have CI integrated in your workflow?
> > > >
> > > > CI: sometime I look at it, sometime not, sometime some guys informs me
> > > > that
> > > > it's broken (I remember that Luca told me some days ago that a package
> > > > didn't compile, so I fixed it).
> > > > Sometime my code compiles on local so for me it's ok but it's just that
> > > > I
> > > > forgot to trash my builddir.
> > >
> > > So you do not go on yourself to build.kde.org and check yourself? Does
> > > #kde- pim have a bot reporting build failures?
> >
> > Long time ago we had it in #kontact.
> > It's not the case now.
>
> Do you remember a reason why it is no longer the case?
>
> IMHO bringing the build report bot back to the chat channel could help with
> the issue this thread is about.

It was quite possibly lost during one of the various refactors we've
had to do of the CI system to solve maintainability issues.
We've had a couple of generations of the CI system so far (by my
count, we're on generation 4), and things have unfortunately been lost
in between the switch between various generations.

IRC channel notifications are currently governed by the rules in
sysadmin/irc-notifications (which is also where commits and Bugzilla
activity notifications for IRC channels are controlled from). See
https://cgit.kde.org/sysadmin/irc-notifications.git/tree/jenkins/notifications.cfg

>
> People tend to look more often into the chat channel then in their email
> folder, so this bot would improve the visibility of the state on
> build.kde.org, also be in a public place so people can directly chat about
> any reasons.
>
> > > > > more? Given you said you work daily on KDE projects, it seems that
> the
> > > > > brokeness of those projects on the KDE CI has slipped your attention.
> > > > > So
> > > > > how does this happen, and how could this be prevented, other than
> > > > > people
> > > > > having to hunt you down on irc and tell you :)
> > > >
> > > > I think that Luca idea to send an email directly to the people which
> > > > breaks
> > > > code when it breaks from several commit is a good idea.
> > >
> > > Noted. Personally I would also fancy that over the generic mass emailing,
> > > where most of the time it was somebody else breaking stuff, so they
> should
> > > care. Given the time-offset due to build times it is also unclear who
> > > broke
> > > things, given the email is not easy to parse, and one might already be
> off
> > > again to other things in life.
> > >
> > > Question is: when would you read the email, and how quick would you
> react?
> >
> > I read it sometime as it arrives in my kdepim-devel mailbox, but indeed
> > sometime we can have several mail in same time because we increase a
> package
> > dependancy and it breaks all other packages. So indeed I didn"t look at all
> > the time.
> >
> > But when a people signals me a problem I try to fix it quickly.
> >
> > An email send after 1 day that package is broken can be a good idea, so it
> > can't be a dependancy problem because we increase package version just that
> > it's really broken.
>
> Increasing the package version ideally should not result in CI breakages.
> Ideally CI only fails if there is a real issue, otherwise people just get
> used to it failing and do not give the deserved attention.
>
> What would prevent you to turn to a system like used with KDE Frameworks,
> where there is some internal dependency version and a separate actual
> version, with the actual version bumped earlier and the internal dependency
> version only bumped some days later? From what I saw, you increase versions
> quite often in PIM, so related breakages would happen quite often.
>
> Cheers
> Friedrich

Cheers,
Ben

>
> PS: Okay if we start to slim the list of CC:s a bit now? Would leave out
> plasma, kdevelop, frameworks-devel on any next reply at least.
>
>

Re: CI system maintainability

[Ben Cooksley]

On Fri, Mar 29, 2019 at 6:45 AM Johannes Zarl-Zierl
 wrote:
>
> Hi,

Hi,

>
> (Sorry for top-posting)
>
> I fear that a mandatory reviews would add too juch strain on smaller teams. 
> If there's just one person with an intimate knowledge of the code-base, plus 
> two co-developers, then who should do the reviews?
>
> How about a distinction based on importance of a project instead? I.e. 
> mandatory reviews for frameworks and any app that wants to be included in KDE 
> apps releases. Non-mandatory reviews can then also come with a "price" to 
> pay: if CI errors are not dealt with in a timely manner, you should be free 
> to disable the CI for administrative reasons...

While this does seem like a nice solution, unfortunately for many
repositories it isn't a case of disabling CI coverage for it, but also
CI coverage for everything that depends on it. In the case of
KContacts, this would also impact on parts of Extragear and Calligra
(who depending on their exact requirements would either lose a
dependency being available, or lose all of their CI coverage).

This is why i've not pursued this as an option in the past, because
it's not fair on other projects that don't have anything to do with
another project aside from being a user of it's interfaces to lose
their coverage, simply because the project they depend on is broken.

>
>   Johannes

Cheers,
Ben

>
> Am 28. März 2019 10:17:18 MEZ schrieb Tomaz Canabrava :
> >On Thu, Mar 28, 2019 at 10:09 AM Luca Beltrame 
> >wrote:
> >>
> >> In data giovedì 28 marzo 2019 09:50:47 CET, Kevin Ottens ha scritto:
> >> > I'd argue we're loosing more with the current state of PIM than
> >we'd loose
> >> > with mandatory reviews.
> >>
> >> Perhaps, instead of an all-or-nothing approach, why not a minimal set
> >of
> >> "requirements" that would require a review? Yes, it requires more
> >discipline
> >> from those involved, but at least it will help people getting
> >"ingrained" with
> >> the concept without being a wall.
> >>
> >> Examples:
> >>
> >> - No review: typo fixes, compile errors, version bumps (internal)
> >> - Review: build system adjustments (perhaps CC some people
> >knowledgeable in
> >> this case), non-trivial changes like patches
> >> - "Deprecation" removals (as in the casus belli here) - review if
> >touching
> >> more than a handful of files / multiple repos
> >>
> >> (list made by someone who has a passing knowledge of C++, so feel
> >free to rip
> >> me to shreds)
> >>
> >> Pre-commit CI (i.e. once the switch to GitLab occurs) and perhaps
> >direct
> >> mailing to the user (as I suggested earlier) in case of continuous
> >failures
> >> will also help.
> >>
> >> If this thing works, one can gradually ramp up the requirements of
> >things that
> >> go through review when the "muscle memory" is formed.
> >
> >The problem is that a git comit is a git commit, there's no way that a
> >typo will be treated differently then another commit.
> >I strongly advocate for "reviews always", but since I was outvoted a
> >few times on this I would at least say "can we at least have reviews
> >for non project members" ?
> >
> >
> >> --
> >> Luca Beltrame
> >> GPG key ID: A29D259B

D20063: Sublime Merge Icon.

[Noah Davis]

ndavis accepted this revision.
ndavis added a comment.


  @otavva I need your email and real name to land your patch with the correct 
author. If you use the Arcanist tool for patches in the future, you will need 
to make sure that your real name is set as your Git user name.

REPOSITORY
  R266 Breeze Icons

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

To: otavva, #vdg, ndavis
Cc: trickyricky26, filipf, ngraham, #vdg, kde-frameworks-devel, arvidhansson, 
ian, hannahk, Pixel_Lime, jraleigh, squeakypancakes, alexde, IohannesPetros, 
GB_2, mglb, michaelh, crozbo, ndavis, firef, bruns, skadinna, aaronhoneycutt, 
mbohlender

D20092: New class KOSRelease - a parser for os-release files

[Albert Astals Cid]

aacid added a comment.


  Just to make sure, have you checked the log to make sure everyone involved in 
the class agrees to GPL -> LGPL?

REPOSITORY
  R244 KCoreAddons

BRANCH
  master

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

To: sitter, apol
Cc: aacid, mpyne, kde-frameworks-devel, michaelh, ngraham, bruns

D5656: Adds method to force the reloading of a document

[Pedro Arthur P. R. Duarte]

pedroarthurp abandoned this revision.

REPOSITORY
  R39 KTextEditor

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

To: pedroarthurp, dfaure, brauch, dhaumann, cullmann
Cc: kde-frameworks-devel, anthonyfieroni, nalvarez, kwrite-devel, gennad, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

Re: CI system maintainability

[Ben Cooksley]

On Fri, Mar 29, 2019 at 10:35 PM David Faure  wrote:
>
> On jeudi 28 mars 2019 16:56:33 CET laurent Montel wrote:
> > CI: sometime I look at it, sometime not, sometime some guys informs me that
> > it's broken (I remember that Luca told me some days ago that a package
> > didn't compile, so I fixed it).
>
> I think the solution to all this is quite simple. If you don't want the
> community to impose mandatory code reviews on you, you need to make it part of
> your daily workflow to look at the state of CI for KDEPIM.
>
> If you go to build.kde.org, Applications, Everything kf5-qt5, and sort by
> status, you can see what's currently broken (red = compilation broken, yellow
> = unittests failing).
>
> I do this (on a frequency matching my own contributions) for all of
> Frameworks, so I'm often the one pinging others about broken unittests.
> Someone (who is not Ben) needs to do this for PIM, and as the most frequent
> contributor, it would make sense for you to do it -- you'd often catch your
> own breakages that way :)
>
> @Ben : do you think it would be possible to have a PIM view on build.kde.org,
> with only the kde/pim/* repos?

That's theoretically possible, but also non-trivial. I'll have a think
about how to best implement this.

>
> (I also wish there was a linux-only view, given that Windows and FreeBSD have
> their own set of problems. Not that I want to ignore them, I did fix things in
> KF5 for those platforms - but once we get to a completely green state on Linux
> (which typically happens first), it would be extremely useful to be able to
> check in one glance that it stays that way.)

This now exists :)
https://build.kde.org/job/Frameworks/view/Platform%20-%20SUSEQt5.10/

>
> @Laurent : do you also have permissions to log into build.kde.org and trigger
> a build? I find this useful, to fix temporary CI problems, like
> https://build.kde.org/job/Applications/view/Everything%20-%20kf5-qt5/job/
> korganizer/job/stable-kf5-qt5%20SUSEQt5.10/24/console
> which failed with a weird "OSError: [Errno 26] Text file busy: '/home/
> jenkins//install-prefix/bin/kdeinit5'".

That is indeed a very weird error. I'm not sure why that would have
happened (best guess: overlayfs had a glitch, or the system ran out of
disk space which can definitely happen)

> Going one level up, I can click on, well, "Lancer un build" (why is this damn
> thing in French --- well, Frenglish? ;) ), to give it another chance.

Seems like Jenkins respects/follows (or at least tries) your browser
language preferences :)

>
> --
> David Faure, fa...@kde.org, http://www.davidfaure.fr
> Working on KDE Frameworks 5
>
>
>

Cheers,
Ben

Re: CI system maintainability

[Ben Cooksley]

On Fri, Mar 29, 2019 at 9:56 PM Kevin Ottens  wrote:
>
> Hello,
>
> On Thursday, 28 March 2019 20:35:11 CET Dr.-Ing. Christoph Cullmann wrote:
> > I and others tried to get more reviews done in the past, but actually I
> > merged more than once stuff that I reviewed but it did break the CI.
>
> That I hope we'll get fixed at some point. It's a big big advantage when you
> can get a CI run on reviews. I find it best when you get both humans and
> scripts looking at the code in a review. It helps a lot in having better
> quality reviews from the humans since they are relieved from the boring
> redundant nitpicking (catching warnings, basic code style, etc.).

With the shift to Gitlab we should be able to provide this hopefully.

We're still figuring out how to be able to provide CI in an easy to
maintain manner (in terms of controlling platforms builds are done
for, which branches, etc).
This is a non-trivial problem (which is why Jenkins is only able to do
master/trunk and stable builds currently) but it should be solvable.

>
> Regards.
> --
> Kevin Ottens, http://ervin.ipsquad.net

Cheers,
Ben

D20102: [timeline] Fix missing/misplaced SlaveBase::finished() calls

[Nathaniel Graham]

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

REPOSITORY
  R293 Baloo

BRANCH
  timeline

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

To: bruns, #baloo, #frameworks, poboiko, ngraham, astippich
Cc: kde-frameworks-devel, gennad, domson, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D20104: [balooshow] Several extensions basic file information output

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> ngraham wrote in main.cpp:171
> Could we use user-friendly terms here?

balooctl is a lowlevel tool, I see no reason to. Also, ISODate is completely 
self describing and non-anbiguous (Timezone, Year/Day/Month ordering, ...)

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, #frameworks, ngraham, poboiko
Cc: kde-frameworks-devel, gennad, domson, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D19764: Fix Minimap with QtCurve style

[Sven Brauch]

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


  Looks sensible, and I think I've even seen this bug already somewhere.

REPOSITORY
  R39 KTextEditor

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

To: sars, brauch, dhaumann, cullmann
Cc: kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, ngraham, 
bruns, demsking, cullmann, sars, dhaumann

KDE CI: Frameworks » baloo » kf5-qt5 SUSEQt5.12 - Build # 36 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/baloo/job/kf5-qt5%20SUSEQt5.12/36/
 Project:
kf5-qt5 SUSEQt5.12
 Date of build:
Sat, 30 Mar 2019 01:06:21 +
 Build duration:
13 min and counting
   BUILD ARTIFACTS
  abi-compatibility-results.yamlacc/KF5Baloo-5.57.0.xmlcompat_reports/KF5Baloo_compat_report.htmllogs/KF5Baloo/5.57.0/log.txt
   JUnit Tests
  Name: (root) Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)Name: projectroot.autotests Failed: 0 test(s), Passed: 4 test(s), Skipped: 0 test(s), Total: 4 test(s)Name: projectroot.autotests.unit Failed: 1 test(s), Passed: 32 test(s), Skipped: 0 test(s), Total: 33 test(s)Failed: projectroot.autotests.unit.file.kinotifytest
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report43%
(10/23)67%
(112/168)67%
(112/168)59%
(5706/9637)41%
(2111/5208)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests.benchmarks100%
(2/2)100%
(2/2)100%
(58/58)100%
(22/22)autotests.integration100%
(3/3)100%
(3/3)92%
(373/406)76%
(168/220)autotests.unit.codecs100%
(3/3)100%
(3/3)100%
(70/70)64%
(23/36)autotests.unit.engine100%
(17/17)100%
(17/17)100%
(761/761)55%
(213/384)autotests.unit.file100%
(11/11)100%
(11/11)95%
(867/910)50%
(255/506)autotests.unit.lib100%
(6/6)100%
(6/6)97%
(403/414)58%
(93/160)src.codecs100%
(5/5)100%
(5/5)89%
(130/146)76%
(35/46)src.engine95%
(35/37)95%
(35/37)78%
(1713/2209)54%
(678/1255)src.file63%
(24/38)63%
(24/38)51%
(867/1710)39%
(394/1008)src.file.extractor0%
(0/6)0%
(0/6)0%
(0/182)0%
(0/70)src.kioslaves.kded0%
(0/1)0%
(0/1)0%
(0/38)0%
(0/42)src.kioslaves.search0%
(0/1)0%
(0/1)0%
(0/105)0%
(0/32)src.kioslaves.tags0%
(0/1)0%
(0/1)0%
(0/273)0%
(0/237)src.kioslaves.timeline0%
(0/2)0%
(0/2)0%
(0/224)0%
(0/113)src.lib55%
(6/11)55%
(6/11)49%
(464/945)43%
(230/537)src.qml0%
(0/2)0%
(0/2)0%
(0/69)0%
(0/20)src.qml.experimental0%
 

D20104: [balooshow] Several extensions basic file information output

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> main.cpp:171
> +   << 
> QDateTime::fromSecsSinceEpoch(time.mTime).toString(Qt::ISODate)
> +   << QLatin1String("\n\tCtime: ") << time.cTime << QChar(' ')
> +   << 
> QDateTime::fromSecsSinceEpoch(time.cTime).toString(Qt::ISODate)

Could we use user-friendly terms here?

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, #frameworks, ngraham, poboiko
Cc: kde-frameworks-devel, gennad, domson, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D20101: [timeline] Canonicalize Url

[Stefan Brüns]

bruns updated this revision to Diff 55049.
bruns added a comment.


  use explicit type name

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20101?vs=55007=55049

BRANCH
  master

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

AFFECTED FILES
  src/kioslaves/timeline/kio_timeline.cpp
  src/kioslaves/timeline/timelinetools.cpp
  src/kioslaves/timeline/timelinetools.h

To: bruns, #baloo, #frameworks, poboiko, ngraham, astippich
Cc: kde-frameworks-devel, gennad, domson, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

KDE CI: Frameworks » baloo » kf5-qt5 SUSEQt5.12 - Build # 38 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/baloo/job/kf5-qt5%20SUSEQt5.12/38/
 Project:
kf5-qt5 SUSEQt5.12
 Date of build:
Sat, 30 Mar 2019 02:36:17 +
 Build duration:
13 min and counting
   BUILD ARTIFACTS
  abi-compatibility-results.yamlacc/KF5Baloo-5.57.0.xmlcompat_reports/KF5Baloo_compat_report.htmllogs/KF5Baloo/5.57.0/log.txt
   JUnit Tests
  Name: (root) Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)Name: projectroot.autotests Failed: 0 test(s), Passed: 4 test(s), Skipped: 0 test(s), Total: 4 test(s)Name: projectroot.autotests.unit Failed: 1 test(s), Passed: 32 test(s), Skipped: 0 test(s), Total: 33 test(s)Failed: projectroot.autotests.unit.file.kinotifytest
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report43%
(10/23)67%
(113/168)67%
(113/168)59%
(5718/9681)40%
(2117/5228)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests.benchmarks100%
(2/2)100%
(2/2)100%
(58/58)100%
(22/22)autotests.integration100%
(3/3)100%
(3/3)92%
(373/406)76%
(168/220)autotests.unit.codecs100%
(3/3)100%
(3/3)100%
(70/70)64%
(23/36)autotests.unit.engine100%
(17/17)100%
(17/17)100%
(761/761)55%
(213/384)autotests.unit.file100%
(11/11)100%
(11/11)95%
(867/910)50%
(255/506)autotests.unit.lib100%
(6/6)100%
(6/6)97%
(403/414)58%
(93/160)src.codecs100%
(5/5)100%
(5/5)89%
(130/146)76%
(35/46)src.engine95%
(35/37)95%
(35/37)78%
(1713/2209)54%
(678/1255)src.file66%
(25/38)66%
(25/38)51%
(879/1712)40%
(400/1008)src.file.extractor0%
(0/6)0%
(0/6)0%
(0/182)0%
(0/70)src.kioslaves.kded0%
(0/1)0%
(0/1)0%
(0/38)0%
(0/42)src.kioslaves.search0%
(0/1)0%
(0/1)0%
(0/105)0%
(0/32)src.kioslaves.tags0%
(0/1)0%
(0/1)0%
(0/273)0%
(0/237)src.kioslaves.timeline0%
(0/2)0%
(0/2)0%
(0/254)0%
(0/125)src.lib55%
(6/11)55%
(6/11)49%
(464/945)43%
(230/537)src.qml0%
(0/2)0%
(0/2)0%
(0/69)0%
(0/20)src.qml.experimental0%
 

D20104: [balooshow] Several extensions basic file information output

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> ngraham wrote in main.cpp:171
> Still, not everyone who uses it would know what "Ctime" means (for example). 
> It's not a huge deal... but I think it would be nice.

The meaning of ctime is too complex to be grasped in one or two words. Anyone 
who //understands// ctime also knows it by its name.

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, #frameworks, ngraham, poboiko
Cc: kde-frameworks-devel, gennad, domson, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D20102: [timeline] Fix missing/misplaced SlaveBase::finished() calls

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R293:fb3fb089c4bc: [timeline] Fix missing/misplaced 
SlaveBase::finished() calls (authored by bruns).

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20102?vs=55008=55052

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

AFFECTED FILES
  src/kioslaves/timeline/kio_timeline.cpp

To: bruns, #baloo, #frameworks, poboiko, ngraham, astippich
Cc: kde-frameworks-devel, gennad, domson, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

KDE CI: Frameworks » baloo » kf5-qt5 SUSEQt5.12 - Build # 37 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/baloo/job/kf5-qt5%20SUSEQt5.12/37/
 Project:
kf5-qt5 SUSEQt5.12
 Date of build:
Sat, 30 Mar 2019 01:19:52 +
 Build duration:
3 min 51 sec and counting
   BUILD ARTIFACTS
  abi-compatibility-results.yamlacc/KF5Baloo-5.57.0.xmlcompat_reports/KF5Baloo_compat_report.htmllogs/KF5Baloo/5.57.0/log.txt
   JUnit Tests
  Name: (root) Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)Name: projectroot.autotests Failed: 0 test(s), Passed: 4 test(s), Skipped: 0 test(s), Total: 4 test(s)Name: projectroot.autotests.unit Failed: 1 test(s), Passed: 32 test(s), Skipped: 0 test(s), Total: 33 test(s)Failed: projectroot.autotests.unit.file.kinotifytest
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report43%
(10/23)67%
(112/168)67%
(112/168)59%
(5706/9650)40%
(2111/5216)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests.benchmarks100%
(2/2)100%
(2/2)100%
(58/58)100%
(22/22)autotests.integration100%
(3/3)100%
(3/3)92%
(373/406)76%
(168/220)autotests.unit.codecs100%
(3/3)100%
(3/3)100%
(70/70)64%
(23/36)autotests.unit.engine100%
(17/17)100%
(17/17)100%
(761/761)55%
(213/384)autotests.unit.file100%
(11/11)100%
(11/11)95%
(867/910)50%
(255/506)autotests.unit.lib100%
(6/6)100%
(6/6)97%
(403/414)58%
(93/160)src.codecs100%
(5/5)100%
(5/5)89%
(130/146)76%
(35/46)src.engine95%
(35/37)95%
(35/37)78%
(1713/2209)54%
(678/1255)src.file63%
(24/38)63%
(24/38)51%
(867/1710)39%
(394/1008)src.file.extractor0%
(0/6)0%
(0/6)0%
(0/182)0%
(0/70)src.kioslaves.kded0%
(0/1)0%
(0/1)0%
(0/38)0%
(0/42)src.kioslaves.search0%
(0/1)0%
(0/1)0%
(0/105)0%
(0/32)src.kioslaves.tags0%
(0/1)0%
(0/1)0%
(0/273)0%
(0/237)src.kioslaves.timeline0%
(0/2)0%
(0/2)0%
(0/225)0%
(0/113)src.lib55%
(6/11)55%
(6/11)49%
(464/945)43%
(230/537)src.qml0%
(0/2)0%
(0/2)0%
(0/69)0%
(0/20)src.qml.experimental0%
   

D20087: Enable glGetGraphicsResetStatus support by default on Qt >= 5.13

[Nathaniel Graham]

ngraham added a comment.


  Given the testing already done by users, yeah, I think this is safe to land. 
+1

REPOSITORY
  R296 KDeclarative

BRANCH
  master

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

To: davidedmundson, apol
Cc: ngraham, apol, kde-frameworks-devel, michaelh, bruns

D20104: [balooshow] Several extensions basic file information output

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> bruns wrote in main.cpp:171
> balooctl is a lowlevel tool, I see no reason to. Also, ISODate is completely 
> self describing and non-anbiguous (Timezone, Year/Day/Month ordering, ...)

Still, not everyone who uses it would know what "Ctime" means (for example). 
It's not a huge deal... but I think it would be nice.

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, #frameworks, ngraham, poboiko
Cc: kde-frameworks-devel, gennad, domson, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D20100: [timeline] Fix warning, add missing UDS entry for "."

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R293:7cd2b04b8ef1: [timeline] Fix warning, add missing UDS 
entry for . (authored by bruns).

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20100?vs=55006=55050

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

AFFECTED FILES
  src/kioslaves/timeline/kio_timeline.cpp

To: bruns, #baloo, #frameworks, poboiko, ngraham, astippich
Cc: kde-frameworks-devel, gennad, domson, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D20104: [balooshow] Several extensions basic file information output

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R293:919fa25cef0b: [balooshow] Several extensions basic file 
information output (authored by bruns).

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20104?vs=55010=55051

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

AFFECTED FILES
  src/tools/balooshow/main.cpp

To: bruns, #baloo, #frameworks, ngraham, poboiko
Cc: kde-frameworks-devel, gennad, domson, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D20100: [timeline] Fix warning, add missing UDS entry for "."

[Nathaniel Graham]

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

REPOSITORY
  R293 Baloo

BRANCH
  timeline

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

To: bruns, #baloo, #frameworks, poboiko, ngraham, astippich
Cc: kde-frameworks-devel, gennad, domson, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D20101: [timeline] Canonicalize Url

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> kio_timeline.cpp:134
>  {
> +auto canonicalUrl = canonicalizeTimelineUrl(url);
> +if (url != canonicalUrl) {

Any reason not to make the type explicit?

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, #frameworks, poboiko, ngraham, astippich
Cc: kde-frameworks-devel, gennad, domson, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D20104: [balooshow] Several extensions basic file information output

[Nathaniel Graham]

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


  All right.

REPOSITORY
  R293 Baloo

BRANCH
  timeline

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

To: bruns, #baloo, #frameworks, ngraham, poboiko
Cc: kde-frameworks-devel, gennad, domson, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D20101: [timeline] Canonicalize Url

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R293:3f3b6560f963: [timeline] Canonicalize Url (authored by 
bruns).

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20101?vs=55049=55054

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

AFFECTED FILES
  src/kioslaves/timeline/kio_timeline.cpp
  src/kioslaves/timeline/timelinetools.cpp
  src/kioslaves/timeline/timelinetools.h

To: bruns, #baloo, #frameworks, poboiko, ngraham, astippich
Cc: kde-frameworks-devel, gennad, domson, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D20101: [timeline] Canonicalize Url

[Nathaniel Graham]

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

REPOSITORY
  R293 Baloo

BRANCH
  master

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

To: bruns, #baloo, #frameworks, poboiko, ngraham, astippich
Cc: kde-frameworks-devel, gennad, domson, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D19887: KFileItem: call stat() on demand, add SkipMimeTypeDetermination option

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kfileitem.cpp:362
> +
> +if (!item.m_bInitCalled && m_bInitCalled) {
> +item.init();

There is one thing I've been wondering back and forth about: the alternative 
way of doing this, which is to call init() unconditionally and testing the bool 
first thing in there.
The benefit is that it would reduce the code size overall, and greatly simplify 
the top of this method (two calls to init, done).
The downside is that for the reader, a call to init() might in fact do nothing 
(and it reads like it's doing something, until going there to check). So it's 
maybeInit(), urgh.

Ah, in another code base (QMimeType) I wrote ensureLoaded(). This could be 
ensureInitialized() ?

Sorry for not suggesting this earlier. Do you agree that it would make the code 
better?

> kfileitem.cpp:609
>  
> -d->m_entry.replace(KIO::UDSEntry::UDS_LOCAL_PATH, path);
> +entry().replace(KIO::UDSEntry::UDS_LOCAL_PATH, path);
>  }

No no, this won't work. entry() returns a copy, not a reference.

> kfileitem.cpp:663
>  // Extract the local path from the KIO::UDSEntry
>  return m_entry.stringValue(KIO::UDSEntry::UDS_LOCAL_PATH);
>  }

here entry() would work, no?

> kfileitem.cpp:1054
>  
> -QStringList names = 
> d->m_entry.stringValue(KIO::UDSEntry::UDS_ICON_OVERLAY_NAMES).split(QLatin1Char(','),
>  QString::SkipEmptyParts);
> +QStringList names = 
> entry().stringValue(KIO::UDSEntry::UDS_ICON_OVERLAY_NAMES).split(QLatin1Char(','),
>  QString::SkipEmptyParts);
>  

It's more fragile this way here, because the reader of this code won't see that 
d->m_bLink is initialized indirectly via entry() calling init().

This is why I had only suggested to use entry() in a few places, not in those 
other places which need init() for other reasons anyway.

By fragile it means, it works today, but any further work/refactoring of this 
code is likely to break.

Same problem in linkDest().

REPOSITORY
  R241 KIO

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

To: hoffmannrobert, dfaure, #frameworks, #dolphin
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

Re: CI system maintainability

[Kevin Ottens]

Hello,

On Thursday, 28 March 2019 20:35:11 CET Dr.-Ing. Christoph Cullmann wrote:
> I and others tried to get more reviews done in the past, but actually I
> merged more than once stuff that I reviewed but it did break the CI.

That I hope we'll get fixed at some point. It's a big big advantage when you 
can get a CI run on reviews. I find it best when you get both humans and 
scripts looking at the code in a review. It helps a lot in having better 
quality reviews from the humans since they are relieved from the boring 
redundant nitpicking (catching warnings, basic code style, etc.).

Regards.
-- 
Kevin Ottens, http://ervin.ipsquad.net


signature.asc
Description: This is a digitally signed message part.

Re: CI system maintainability

[Ovidiu-Florin Bogdan]

Hello,

A Merge Request in GitLab does not necessarily imply the need for a review by e 
person. It can just run a pipeline to validate that the code isn't broken. If 
the pipeline fails, the merge button is not available.

We use GitLab at work and we have it set up like this:

* Main branches (develop/master/release/etc) are proteted and cannot be 
directly commited/pushed to, and only updated through MR
* Each project defines what it's build/validate pipeline is (Jenkinsfile in 
project repo)
* The pipeline is executed uppon creating the MR
* if the Pipeline passes, the MR can be merged to the mainline branch

This way we ensure that no code gets in that fails the build or with tests 
failing.

P.S. We also store the build artifacts in a binary repository from where other 
pipelines can fetch them to be used in compiling other projects.

P.P.S. This is the "DevOps" process used in most companies. The tools might 
differ, but the process is the same. It's the same for most FOSS projects as 
well.

Regards,
Ovidiu

În ziua de joi, 28 martie 2019, la 10:29:22 EET, Kevin Ottens a scris:
> Hello,
> 
> On Thursday, 28 March 2019 09:16:11 CET Ben Cooksley wrote:
> > Please note that the commits in this instance were pushed without
> > review, so restrictions on merge requests wouldn't make a difference
> > in this case unfortunately.
> 
> Maybe it's about time to make reviews mandatory... I know it's unpopular in 
> KDE, and I advocated for "don't force a tool if you can get someone to look 
> at 
> your screen or pair with you" in the past. Clearly this compromise gets 
> somewhat exploited and that's especially bad in the case of a fragile and 
> central component like KDE PIM.
> 
> Regards.
> 




signature.asc
Description: This is a digitally signed message part.

Re: CI system maintainability

[Andrius Štikonas]

+1 for this. I think running tests before merging is more acceptable than 
having mandatory reviews.

On 29 March 2019 11:10:52 GMT+00:00, Ovidiu-Florin Bogdan 
 wrote:
>Hello,
>
>A Merge Request in GitLab does not necessarily imply the need for a
>review by e person. It can just run a pipeline to validate that the
>code isn't broken. If the pipeline fails, the merge button is not
>available.
>
>We use GitLab at work and we have it set up like this:
>
>* Main branches (develop/master/release/etc) are proteted and cannot be
>directly commited/pushed to, and only updated through MR
>* Each project defines what it's build/validate pipeline is
>(Jenkinsfile in project repo)
>* The pipeline is executed uppon creating the MR
>* if the Pipeline passes, the MR can be merged to the mainline branch
>
>This way we ensure that no code gets in that fails the build or with
>tests failing.
>
>P.S. We also store the build artifacts in a binary repository from
>where other pipelines can fetch them to be used in compiling other
>projects.
>
>P.P.S. This is the "DevOps" process used in most companies. The tools
>might differ, but the process is the same. It's the same for most FOSS
>projects as well.
>
>Regards,
>Ovidiu
>
>În ziua de joi, 28 martie 2019, la 10:29:22 EET, Kevin Ottens a scris:
>> Hello,
>> 
>> On Thursday, 28 March 2019 09:16:11 CET Ben Cooksley wrote:
>> > Please note that the commits in this instance were pushed without
>> > review, so restrictions on merge requests wouldn't make a
>difference
>> > in this case unfortunately.
>> 
>> Maybe it's about time to make reviews mandatory... I know it's
>unpopular in 
>> KDE, and I advocated for "don't force a tool if you can get someone
>to look at 
>> your screen or pair with you" in the past. Clearly this compromise
>gets 
>> somewhat exploited and that's especially bad in the case of a fragile
>and 
>> central component like KDE PIM.
>> 
>> Regards.
>> 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

D20105: ecm_add_wayland_client_protocol: Improve error messages

[Aleix Pol Gonzalez]

This revision was automatically updated to reflect the committed changes.
Closed by commit R240:6b27bbf0b954: ecm_add_wayland_client_protocol: Improve 
error messages (authored by apol).

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20105?vs=55012=55030

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

AFFECTED FILES
  find-modules/FindWaylandScanner.cmake

To: apol, #kwin, #frameworks, zzag
Cc: kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

Re: CI system maintainability

[Volker Krause]

On Friday, 29 March 2019 08:59:59 CET Kevin Ottens wrote:
> On Thursday, 28 March 2019 21:53:06 CET Alexander Neundorf wrote:
> > Having mandatory reviews for a central and complex component like akonadi
> > looks like a very good and obvious idea.
> 
> Yep.

Looking at the 18.12 -> 19.04 timeframe the majority of changes to Akonadi 
went through pre-commit review, even more so if you discard commits doing 
release work (version bumps etc) or similar maintenance not touching the 
actual logic.

And specifically the changes that caused us the most headaches due to 
introducing a nasty regression went through review.

Sure, nothing is perfect, but I don't think code review in Akonadi is the most 
pressing issue here.
 
> > OTOH if there is only one developer who is really expert for akonadi, this
> > makes it kind of unfeasible.
> 
> That's the chicken and egg problem we're in concerning KDEPIM. The developer
> story is frankly really harder than most software out there which makes it
> unlikely for people to pick it over something else for contributions.
> That's in part tied to your next point below and partly tied to
> documentation, on- boarding etc. The unwillingness to be slowed down is
> getting in the way of fixing that situation: to be a desirable project to
> contribute to you need to spend time advocating, documenting and taking
> newbies by the hand until they become regular contributors.
> 
> Yes it's tough, and TBH I'm guilty of not doing this more on my own
> projects. But on such a strategic piece of software like KDEPIM there's
> some responsibility in carrying those duties for the well being of the
> project.

How to address the issue of bus factor ~1 components in PIM is the real 
question here, I completely agree. But this is getting way off topic from 
Ben's original issue, and for the wide range of recipients. 

Also, I don't think overly generic statements on that help us much, so maybe 
let's discuss concrete steps for this at the sprint next week?

Regards,
Volker


signature.asc
Description: This is a digitally signed message part.

Re: CI system maintainability

[Kevin Ottens]

Hello,

On Thursday, 28 March 2019 21:53:06 CET Alexander Neundorf wrote:
> On 2019 M03 28, Thu 16:04:01 CET Boudhayan Gupta wrote:
> > As a user, I simply do not want unreviewed crap running on my computer.
> > Yes, crap, because no software engineer writes bug-free code all the time,
> > and if you're so overconfident that you don't need reviews on even your
> > one-liners, you're probably too overconfident to be writing good code
> > anyway, so I'm going to operate on the presumption that if the code hasn't
> > had more than one pair of eyeballs ever looking at it, it's crap.
> 
> If you put it that strong, it's wrong.
> Code which has not been reviewed is not generally "crap". Code which has
> been reviewed is not generally "high quality".
> Unreviewed code can be good, and often is good, and reviews, maybe
> especially if they are mandatory, *can* be crappy: just pointing out
> formatting issues, Oking the patch without understanding the big picture
> (and feeling somewhat pressured to accept a patch because the review is
> mandatory and otherwise you are blocking the other developer, etc.)

Hell yeah, it's not a silver bullet. Actually it can be only one safety net 
among others. None of them are perfect, none of them will catch it all or be 
of good quality all the time, it's just about mitigating risks as much as 
possible.
 
> > As a developer, I know that even one-liners, and especially one-liners,
> > the sort where you think "meh, this is a tiny little thing, I don't have
> > to be careful" are the ones that have the most dangerous typos and
> > unintended bugs.
> 
> That's also a wrong argument. one-liners are not especially prone to causing
> most bugs. They may introduce bugs, but I think, since they are small, this
> is less likely than for bigger patches.

I don't think that's true. It's a question of complexity in the system really. 
In a trivial system indeed they are less likely to introduce bugs than for 
bigger patches... but as the software grows and complexity rises (especially 
with the multiplication of mutable states) one liners tend to be as error-
prone as bigger patches.

> ...
> 
> > In a project like PIM, if the code hasn't been through review, which
> > independent party do I trust to verify that you're not, for example,
> > leaking my Google password to some world-readable tempfile?
> 
> Having mandatory reviews for a central and complex component like akonadi
> looks like a very good and obvious idea.

Yep.

> OTOH if there is only one developer who is really expert for akonadi, this
> makes it kind of unfeasible.

That's the chicken and egg problem we're in concerning KDEPIM. The developer 
story is frankly really harder than most software out there which makes it 
unlikely for people to pick it over something else for contributions. That's 
in part tied to your next point below and partly tied to documentation, on-
boarding etc. The unwillingness to be slowed down is getting in the way of 
fixing that situation: to be a desirable project to contribute to you need to 
spend time advocating, documenting and taking newbies by the hand until they 
become regular contributors.

Yes it's tough, and TBH I'm guilty of not doing this more on my own projects. 
But on such a strategic piece of software like KDEPIM there's some 
responsibility in carrying those duties for the well being of the project.

> IMO this specific case is also a technical issue. Akonadi makes things
> complicated (and it's already 13 years old, so it should be mature in the
> meantime).

Yes, it's byzantine really. And the user experience is still not great (to be 
polite), had to setup some new hardware recently and I was honestly almost to 
the point of throwing it all through the window.

Regards.
-- 
Kevin Ottens, http://ervin.ipsquad.net


signature.asc
Description: This is a digitally signed message part.

D20059: Take clang-cl into account.

[Christian Mollekopf]

cmollekopf added a comment.


  FWIW, I have meanwhile used this patch to build all kube dependencies on 
linux and osx as well, and it seems like it doesn't break anything.
  
  I think clang-cl should receive all arguments it understands with this patch.

REPOSITORY
  R240 Extra CMake Modules

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

To: cmollekopf, #windows
Cc: apol, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D19092: Add bison minimum version of 2.4.1 due to %code

[Luigi Toscano]

ltoscano added a comment.


  Not on the 19.04 branch, the dependency freeze happened few weeks ago - you 
may want to ask to release-team@ for an exception if you think it's important 
there too.

REPOSITORY
  R309 KService

BRANCH
  bison_min_version (branched from master)

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

To: hindenburg, #frameworks, winterz
Cc: ltoscano, winterz, kde-frameworks-devel, michaelh, ngraham, bruns

D19092: Add bison minimum version of 2.4.1 due to %code

[Pino Toscano]

pino added a comment.


  In D19092#440060 , @ltoscano wrote:
  
  > Not on the 19.04 branch, ...
  
  
  This is a Framework (kservice).

REPOSITORY
  R309 KService

BRANCH
  bison_min_version (branched from master)

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

To: hindenburg, #frameworks, winterz
Cc: pino, ltoscano, winterz, kde-frameworks-devel, michaelh, ngraham, bruns

D20105: ecm_add_wayland_client_protocol: Improve error messages

[Vlad Zagorodniy]

zzag added a comment.


  Hmm it looks like we're using ancient `code` option.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

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

To: apol, #kwin, #frameworks, zzag
Cc: kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

Re: CI system maintainability

[Friedrich W. H. Kossebau]

Am Donnerstag, 28. März 2019, 23:06:17 CET schrieb laurent Montel:
> Le jeudi 28 mars 2019, 18:27:42 CET Friedrich W. H. Kossebau a écrit :
> > Am Donnerstag, 28. März 2019, 16:56:33 CET schrieb laurent Montel:
> > > Le jeudi 28 mars 2019, 16:11:12 CET Friedrich W. H. Kossebau a écrit :
> > > > Given the actual purpose of this thread, I would be more curious how
> > > > you
> > > > have CI integrated in your workflow?
> > > 
> > > CI: sometime I look at it, sometime not, sometime some guys informs me
> > > that
> > > it's broken (I remember that Luca told me some days ago that a package
> > > didn't compile, so I fixed it).
> > > Sometime my code compiles on local so for me it's ok but it's just that
> > > I
> > > forgot to trash my builddir.
> > 
> > So you do not go on yourself to build.kde.org and check yourself? Does
> > #kde- pim have a bot reporting build failures?
> 
> Long time ago we had it in #kontact.
> It's not the case now.

Do you remember a reason why it is no longer the case?

IMHO bringing the build report bot back to the chat channel could help with 
the issue this thread is about.

People tend to look more often into the chat channel then in their email 
folder, so this bot would improve the visibility of the state on 
build.kde.org, also be in a public place so people can directly chat about 
any reasons.

> > > > more? Given you said you work daily on KDE projects, it seems that 
the
> > > > brokeness of those projects on the KDE CI has slipped your attention.
> > > > So
> > > > how does this happen, and how could this be prevented, other than
> > > > people
> > > > having to hunt you down on irc and tell you :)
> > > 
> > > I think that Luca idea to send an email directly to the people which
> > > breaks
> > > code when it breaks from several commit is a good idea.
> > 
> > Noted. Personally I would also fancy that over the generic mass emailing,
> > where most of the time it was somebody else breaking stuff, so they 
should
> > care. Given the time-offset due to build times it is also unclear who
> > broke
> > things, given the email is not easy to parse, and one might already be 
off
> > again to other things in life.
> > 
> > Question is: when would you read the email, and how quick would you 
react?
> 
> I read it sometime as it arrives in my kdepim-devel mailbox, but indeed
> sometime we can have several mail in same time because we increase a 
package
> dependancy and it breaks all other packages. So indeed I didn"t look at all
> the time.
> 
> But when a people signals me a problem I try to fix it quickly.
> 
> An email send after 1 day that package is broken can be a good idea, so it
> can't be a dependancy problem because we increase package version just that
> it's really broken.

Increasing the package version ideally should not result in CI breakages. 
Ideally CI only fails if there is a real issue, otherwise people just get 
used to it failing and do not give the deserved attention.

What would prevent you to turn to a system like used with KDE Frameworks, 
where there is some internal dependency version and a separate actual 
version, with the actual version bumped earlier and the internal dependency 
version only bumped some days later? From what I saw, you increase versions 
quite often in PIM, so related breakages would happen quite often.

Cheers
Friedrich

PS: Okay if we start to slim the list of CC:s a bit now? Would leave out 
plasma, kdevelop, frameworks-devel on any next reply at least.

Re: CI system maintainability

[David Faure]

On jeudi 28 mars 2019 16:56:33 CET laurent Montel wrote:
> CI: sometime I look at it, sometime not, sometime some guys informs me that
> it's broken (I remember that Luca told me some days ago that a package
> didn't compile, so I fixed it).

I think the solution to all this is quite simple. If you don't want the 
community to impose mandatory code reviews on you, you need to make it part of 
your daily workflow to look at the state of CI for KDEPIM.

If you go to build.kde.org, Applications, Everything kf5-qt5, and sort by 
status, you can see what's currently broken (red = compilation broken, yellow 
= unittests failing).

I do this (on a frequency matching my own contributions) for all of 
Frameworks, so I'm often the one pinging others about broken unittests. 
Someone (who is not Ben) needs to do this for PIM, and as the most frequent 
contributor, it would make sense for you to do it -- you'd often catch your 
own breakages that way :)

@Ben : do you think it would be possible to have a PIM view on build.kde.org, 
with only the kde/pim/* repos?

(I also wish there was a linux-only view, given that Windows and FreeBSD have 
their own set of problems. Not that I want to ignore them, I did fix things in 
KF5 for those platforms - but once we get to a completely green state on Linux 
(which typically happens first), it would be extremely useful to be able to 
check in one glance that it stays that way.)

@Laurent : do you also have permissions to log into build.kde.org and trigger 
a build? I find this useful, to fix temporary CI problems, like 
https://build.kde.org/job/Applications/view/Everything%20-%20kf5-qt5/job/
korganizer/job/stable-kf5-qt5%20SUSEQt5.10/24/console
which failed with a weird "OSError: [Errno 26] Text file busy: '/home/
jenkins//install-prefix/bin/kdeinit5'".
Going one level up, I can click on, well, "Lancer un build" (why is this damn 
thing in French --- well, Frenglish? ;) ), to give it another chance.

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

KDE CI: Frameworks » kio » kf5-qt5 FreeBSDQt5.12 - Build # 71 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kio/job/kf5-qt5%20FreeBSDQt5.12/71/
 Project:
kf5-qt5 FreeBSDQt5.12
 Date of build:
Thu, 28 Mar 2019 21:34:53 +
 Build duration:
11 hr and counting
   JUnit Tests
  Name: projectroot Failed: 5 test(s), Passed: 47 test(s), Skipped: 0 test(s), Total: 52 test(s)Failed: projectroot.autotests.kiocore_kmountpointtestFailed: projectroot.autotests.kiowidgets_dropjobtestFailed: projectroot.autotests.kiowidgets_kdirlistertestFailed: projectroot.autotests.kiowidgets_kdirmodeltestFailed: projectroot.autotests.kiowidgets_kurifiltertestName: projectroot.autotests Failed: 0 test(s), Passed: 6 test(s), Skipped: 0 test(s), Total: 6 test(s)Name: projectroot.src.ioslaves.trash Failed: 1 test(s), Passed: 0 test(s), Skipped: 0 test(s), Total: 1 test(s)Failed: projectroot.src.ioslaves.trash.tests.testtrashName: projectroot.src.kpasswdserver Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)

D19092: Add bison minimum version of 2.4.1 due to %code

[Luigi Toscano]

ltoscano added a comment.


  Ups, sorry! So according https://community.kde.org/Schedules/Frameworks there 
are no special provisions when the change doesn't touch translatable strings 
and the change is properly tested.

REPOSITORY
  R309 KService

BRANCH
  bison_min_version (branched from master)

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

To: hindenburg, #frameworks, winterz
Cc: pino, ltoscano, winterz, kde-frameworks-devel, michaelh, ngraham, bruns

D20105: ecm_add_wayland_client_protocol: Improve error messages

[Vlad Zagorodniy]

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

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

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

To: apol, #kwin, #frameworks, zzag
Cc: kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

Re: CI system maintainability

[Kevin Ottens]

Hello,

On Friday, 29 March 2019 09:43:44 CET Volker Krause wrote:
> On Friday, 29 March 2019 08:59:59 CET Kevin Ottens wrote:
> > On Thursday, 28 March 2019 21:53:06 CET Alexander Neundorf wrote:
> > > Having mandatory reviews for a central and complex component like
> > > akonadi
> > > looks like a very good and obvious idea.
> > 
> > Yep.
> 
> Looking at the 18.12 -> 19.04 timeframe the majority of changes to Akonadi
> went through pre-commit review, even more so if you discard commits doing
> release work (version bumps etc) or similar maintenance not touching the
> actual logic.
> 
> And specifically the changes that caused us the most headaches due to
> introducing a nasty regression went through review.
> 
> Sure, nothing is perfect, but I don't think code review in Akonadi is the
> most pressing issue here.

Fair enough. I was thinking more PIM in general though than Akonadi in 
particular.
 
> > > OTOH if there is only one developer who is really expert for akonadi,
> > > this makes it kind of unfeasible.
> > 
> > That's the chicken and egg problem we're in concerning KDEPIM. The
> > developer story is frankly really harder than most software out there
> > which makes it unlikely for people to pick it over something else for
> > contributions. That's in part tied to your next point below and partly
> > tied to
> > documentation, on- boarding etc. The unwillingness to be slowed down is
> > getting in the way of fixing that situation: to be a desirable project to
> > contribute to you need to spend time advocating, documenting and taking
> > newbies by the hand until they become regular contributors.
> > 
> > Yes it's tough, and TBH I'm guilty of not doing this more on my own
> > projects. But on such a strategic piece of software like KDEPIM there's
> > some responsibility in carrying those duties for the well being of the
> > project.
> 
> How to address the issue of bus factor ~1 components in PIM is the real
> question here, I completely agree. But this is getting way off topic from
> Ben's original issue, and for the wide range of recipients.

Yes, I realized only too late that I kind of hijacked the thread somehow. I 
apologies about that.

> Also, I don't think overly generic statements on that help us much, so maybe
> let's discuss concrete steps for this at the sprint next week?

Definitely. It's in part because I know the sprint is coming that I started to 
wave that particular flag. :-)

I wish Laurent was there though, it'll make that particular discussion harder 
to conclude without him...

Regards.
-- 
Kevin Ottens, http://ervin.ipsquad.net


signature.asc
Description: This is a digitally signed message part.

D19887: KFileItem: call stat() on demand, add SkipMimeTypeDetermination option

[Robert Hoffmann]

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

INLINE COMMENTS

> dfaure wrote in kfileitem.cpp:361
> What about the other way around? I think this needs the symmetrical test to 
> call item.init() if needed
> (and the corresponding unittest, write it first)

You're right, fixed, unittest added.

> dfaure wrote in kfileitem.cpp:730
> This use of d->m_entry needs a call to init(), no?

It does, now there via entry(). And ACL() needs init(), too, it's in 
hasExtendedACL() there.

> dfaure wrote in kfileitem.cpp:766
> This kind of method (which only uses d->m_entry in one place) could be 
> simplified by just doing
> 
>   return entry().stringValue();
> 
> Then the init() would happen inside entry().
> 
> This would work in user() just above, too.

And in defaultACL(), setLocalPath(), linkDest(), hasExtendedACL() and 
overlays().

REPOSITORY
  R241 KIO

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

To: hoffmannrobert, dfaure, #frameworks, #dolphin
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D19887: KFileItem: call stat() on demand, add SkipMimeTypeDetermination option

[Robert Hoffmann]

hoffmannrobert updated this revision to Diff 55025.
hoffmannrobert added a comment.


  - Fix cmp() #3, add test for cmp()/init(), simplifications

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19887?vs=54934=55025

BRANCH
  add_skipStat

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

AFFECTED FILES
  autotests/kfileitemtest.cpp
  autotests/kfileitemtest.h
  src/core/kfileitem.cpp
  src/core/kfileitem.h

To: hoffmannrobert, dfaure, #frameworks, #dolphin
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D19887: KFileItem: call stat() on demand, add SkipMimeTypeDetermination option

[Robert Hoffmann]

hoffmannrobert edited the summary of this revision.

REPOSITORY
  R241 KIO

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

To: hoffmannrobert, dfaure, #frameworks, #dolphin
Cc: kde-frameworks-devel, michaelh, ngraham, bruns