Re: CI system maintainability
On Thursday, March 28, 2019 7:40:09 AM CEST Ben Cooksley wrote: > At this point given the amount of effort required to maintain a CI > system vs. the amount of care actually being given by some developers > (who are ignoring it's failure emails) it becomes questionable whether > the effort is worth the return (and if not, we should just shut it > down) In the large thread CI and code review are intertwined. They do not need to be. The issue brought up is failing CI. The solution is to not allow merges to shared branches until CI has checked your patch. If you want to a particular branch to be merged to a shared branch, do this: - push your code to the kde git server as a branch that is up to date with the branch that you want to merge with - CI will run on the branch you pushed (it should monitor each branch of each repo for changes) - after CI succeeds, it's now possible to merge the graph on the intended branch. If CI has failed, the git push will give an error. The error message contains an URL where you can read the error and, for cases where CI is fickle, restart the CI. This can all work independently of a system like Phabricator or GitLab, but does require some setup. ⤳Jos signature.asc Description: This is a digitally signed message part.
Re: CI system maintainability
On Sat, Mar 30, 2019 at 10:46 PM Volker Krause wrote: > > On Friday, 29 March 2019 20:54:54 CET Ben Cooksley wrote: > > On Fri, Mar 29, 2019 at 6:45 AM Johannes Zarl-Zierl > > > 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. > > I agree that anything on the CI level would be merely a workaround for this at > best. I'd rather suggest we address this at the source by turning the > externally used modules into frameworks. We did that last year already for > KHolidays and Syndication who were used by Plasma among others. KContacts, > KCalCore and KMime should follow that next IMHO. > > The next time window to do that relatively painlessly is coming up after the > 19.04 applications release I think, and all of those have been part of the > KDE4-era kdepimlibs module that complied with KF5-like ABI guarantees, so the > necessary work should be limited hopefully. Extra review of the public > interfaces would certainly help with making this happen I think. This would certainly help resolve many of the issues I think, because then the build issues should be mostly contained to one specific Product, which makes this much easier to work with in the long term. > > Regards, > Volker Cheers, Ben
Re: CI system maintainability
On Friday, 29 March 2019 20:54:54 CET Ben Cooksley wrote: > On Fri, Mar 29, 2019 at 6:45 AM Johannes Zarl-Zierl > > 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. I agree that anything on the CI level would be merely a workaround for this at best. I'd rather suggest we address this at the source by turning the externally used modules into frameworks. We did that last year already for KHolidays and Syndication who were used by Plasma among others. KContacts, KCalCore and KMime should follow that next IMHO. The next time window to do that relatively painlessly is coming up after the 19.04 applications release I think, and all of those have been part of the KDE4-era kdepimlibs module that complied with KF5-like ABI guarantees, so the necessary work should be limited hopefully. Extra review of the public interfaces would certainly help with making this happen I think. Regards, Volker signature.asc Description: This is a digitally signed message part.
Re: CI system maintainability
Am 29.03.19 um 21:01 schrieb Ben Cooksley: > 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). In case, an example is wanted: https://gitlab.freedesktop.org/dbus/dbus/blob/master/.gitlab-ci.yml specifies a working configuration for building several builds variants for linux and cross compiled Windows. Ralf
Re: CI system maintainability
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
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
Re: CI system maintainability
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
+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.
Re: CI system maintainability
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
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
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
Re: CI system maintainability
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
Re: CI system maintainability
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
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.
Re: CI system maintainability
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
[cutting the cross-post by half] On jeudi 28 mars 2019 16:45:51 CET Daniel Vrátil wrote: > That said, I do put up for review everything that is not Akonadi/PIM core > (even changes to PIM components that are not "mine"). The reason I don't do > this for Akonadi is that there's no-one really to review my code, because > no- one else works on it, or at least that has been my perception of the > situation lately. I have been more involved in akonadi lately, and I volunteer to review your patches. -- David Faure, fa...@kde.org, http://www.davidfaure.fr Working on KDE Frameworks 5
Re: CI system maintainability
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.
Re: CI system maintainability
Le jeudi 28 mars 2019, 18:27:42 CET Friedrich W. H. Kossebau a écrit : > Thanks for reply. More below: > > 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 : > > > Hi Laurent, > > > > > > Am Donnerstag, 28. März 2019, 14:33:59 CET schrieb laurent Montel: > > > > For example I works all days on kde (pim or other) when I wake up, or > > at > > > > > noon after my lunch or the evening, I will not wait several days for a > > > > review because nobody has time to do it. > > > > > > > > (For example I make ~ 15 commits by days on pim/ruqola/framework, I > > > > don't > > > > want to wait several days/weeks until someone wants to review my > > patchs) > > > > Something might be lost in translation here, do you think, because you > > > work > > > daily on code of KDE projects, and other people (so potential reviewers) > > > do > > > not, this is an argument to do instant pushes of unreviewed commits? > > > > I maintain pim* from several years, I fix bugs, I improve apps, I fix all > > bugs that I put in my code, so for this one I consider that I can commit > > without review them (as other guys on other project that they maintain). > > For framework I mainly use phabricator. > > I was thinking of projects where there are multiple persons contributing/ > maintaining, like Frameworks. For framework I use mainly phabricator See https://phabricator.kde.org/people/revisions/196/ > > So for projects where you are the only person who has any real insight, > indeed I agree to pushing directly, as I do that as well :) > > Because IMHO the costs for having to hope & wait for somebody else to do a > "review", where one then spends lots of time rather explaining things to > someone, who is not really into the project and also never might be, is not > anywhere worth the time one could have otherwise invested in fixing other > existing bugs or adding new features or improving code quality. > > If people want to get into a project, doing own patches or just watching the > commits and asking normally on irc or by email about the architecture > should work good enough. Abusing reviews for teaching about some project > would annoy me at least, usually the patch is to fix some annoying bug or > add a wanted feature, so it should get in as early as possible. > > > > Not sure where this is from, but often I have seen an unwritten policy > > > applied where people for a patch uploaded for review after one week of > > > no > > > response add a ping and then wait another week, before finally pushing > > the > > > > change. To me this seems a fair and reasonable policy, only ignores > > people > > > > who are on vacation for some more weeks or otherwise inactive, but I > > > have > > > not seen that ever been a real issue. > > > > 2 weeks for a commit, for me it's too long. > > I understand why people can be demotivated when they must wait long time > > until having an answer. > > Well, 2 weeks is the time-out, only reached in worst case. Ideally people > give some feedback before, which often enough happens. And indeed one also > has to hunt people down to get a review, like at meetings or in chat (or > trade review work with each other :) ). But isn't this the same also at > work- work, unless there is a dedicated review team which needs to react by > itself? Co-operating on something needs social interaction after all. > > > > 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. > > For what I can tell e.g. for KDevelop, personally I rely on the bot on > #kdevelop reporting the CI state/build results, as I only look at emails > rather once a day, while the chat channel is more real-time, and one also > can immediately talk to peers about why some build now fails, as well as > coordinate who will fix that. > > > > And where things could be improved, to > > > prevent the current state of unhappiness for people who care about CI > > some > > > > 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. >
Re: CI system maintainability
Hi, On 2019 M03 28, Thu 16:04:01 CET Boudhayan Gupta wrote: > Hi, > > On Thu, 28 Mar 2019 at 15:21, Kevin Ottens wrote: > > Hello, > > > > On Thursday, 28 March 2019 14:33:59 CET laurent Montel wrote: > > > I am against to force mandatory review, as it will create a lot of lose > > > > of > > > > > time, > > > > As I said, unpopular. > > I don't get why mandatory code reviews are so unpopular. > > I don't care if you lose time. Oh, yes, you do. Assuming a developers would severly loose time due to waiting for reviews, at some point motivation would decline, at some point contributions and progress would decline... > I don't want the guys building my house to > cut corners mixing my concrete because it's going to save time. I would certainly be happy enough if an experienced guy would build my house in a reasonable time relatively quickly, more happy than if he would have to wait for some authority to check hs work after every day, and so make almost no progress anymore... (I'm not saying that mandatory reviews would have that effect, I just think the argument is wrong). > Why are you > in such a massive hurry to make changes to software which for example holds > access to my Google Account password? In fact, the very fact that you make > this argument makes me wonder if I'm running trustable code on my computer > at all, Now, when you raise the question whether kmail+akonadi is "trustable code", the ice is getting thin... I think not having mandatory code reviews for akonadi + kde pim is not the problem in the case of kdepim. ... > 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.) > 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. ... > 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. OTOH if there is only one developer who is really expert for akonadi, this makes it kind of unfeasible. 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). ... > Let me be very clear - even if you're the best damn programmer on the > planet, if *you* wrote the code, I do not trust *you* one inch to tell me > that that code is correct. That verification needs to come from someone > else, someone who does not have a conflict of interest in seeing that code > get into production. Sorry, your arguments are too black & white. Alex
Re: CI system maintainability
On 2019 M03 28, Thu 17:43:36 CET Friedrich W. H. Kossebau wrote: ... > Though I assume no-one is using it anyway, given there are so few bugs > reported ;) I use it whenever I need to look at a file "raw", and I can't remember having noticed bugs :-) Alex
Re: CI system maintainability
Hi, > Hi, > > On Thu, 28 Mar 2019 at 15:21, Kevin Ottens wrote: > >> Hello, >> >> On Thursday, 28 March 2019 14:33:59 CET laurent Montel wrote: >> > I am against to force mandatory review, as it will create a lot of lose >> of >> > time, >> >> As I said, unpopular. >> > > I don't get why mandatory code reviews are so unpopular. > > I don't care if you lose time. I don't want the guys building my house to > cut corners mixing my concrete because it's going to save time. Why are you > in such a massive hurry to make changes to software which for example holds > access to my Google Account password? In fact, the very fact that you make > this argument makes me wonder if I'm running trustable code on my computer > at all, because apparently doing it quickly is far more important than > doing it right. > > 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. > > 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. Reviews catch that. > > 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? Do you really > expect every user to read the entire codebase for themselves and make sure > that's not being done? The whole point of having all the code out in the > open for independent audit purposes, to protect your security and privacy > and what not is completely moot if no one else actually looks at the code > anyway. And let's be honest, the code quality of some of KDE's projects - I > wouldn't touch them with a six-foot pole. The ones I would touch though, > all have multiple people looking at the code and reviewing everything that > goes in. > > Let me be very clear - even if you're the best damn programmer on the > planet, if *you* wrote the code, I do not trust *you* one inch to tell me > that that code is correct. That verification needs to come from someone > else, someone who does not have a conflict of interest in seeing that code > get into production. This is nothing personal, this is confirmation bias on > the author's part which leads to issues that even though they might be > infrequent, usually have catastrophic impact. > > And if "culture" trumps over engineering best practices, it follows that I > should just stop using software produced by this entity because who knows > what it's doing. Mandatory code reviews are nice, if you have the manpower. Look at our phabricator, look for example at the queue of reviews for KTextEditor. The team is small and the code is complex and old (not rocket-science, it is a text editor, but still...). 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. In most cases I fixed it myself afterwards or reverted again, but a few times I needed to get ping'd by others that I was stupid, too. In the current case discussed an error happend, it could have happend exactly the same way if for example "me" would have reviewed it. A lot of the changes which are at the moment in the queue are stuck because a) lack of time to review the change b) lack of time to ever be able to test the change For any non-trivial behavior change (especially for features you not use yourself at all), any meaningful review is a full-time job. e.g. in our company you would let some student test the changed behavior some days. This is just not feasible for me, and yes, for some of these changes, rather than abandoning them (and trashing precious work others did), I will somewhen apply them with close to zero real testing. Greetings Christoph -- - Dr.-Ing. Christoph Cullmann - AbsInt Angewandte Informatik GmbH Email: cullm...@absint.com Science Park 1 Tel: +49-681-38360-22 66123 Saarbrücken Fax: +49-681-38360-20 GERMANYWWW: http://www.AbsInt.com Geschäftsführung: Dr.-Ing. Christian Ferdinand Eingetragen im Handelsregister des Amtsgerichts Saarbrücken, HRB 11234
Re: CI system maintainability
With regards to the discussion about mandatory code review, I think it's important to avoid immediately rushing to create new policy as a result of a particular event or abuse. It's always tempting to try to put in place a rule that would have avoided the problem if it had existed and was being followed, but usually in these circumstances, existing rules or conventions were already being violated. So adding new ones usually doesn't help as much as people would want because they don't address the underlying issue of why rules are being broken in the first place. In this case, it seems like the problem is that there are certain individuals or teams that are pushing risky, breaking changes without code review, and then ignoring failures in the CI. I think we might do well to try to answer the question of why that's happening before we create a new rule aimed at stopping it. Nate
Re: CI system maintainability
Hi, On Thu, 28 Mar 2019 at 15:21, Kevin Ottens wrote: > Hello, > > On Thursday, 28 March 2019 14:33:59 CET laurent Montel wrote: > > I am against to force mandatory review, as it will create a lot of lose > of > > time, > > As I said, unpopular. > I don't get why mandatory code reviews are so unpopular. I don't care if you lose time. I don't want the guys building my house to cut corners mixing my concrete because it's going to save time. Why are you in such a massive hurry to make changes to software which for example holds access to my Google Account password? In fact, the very fact that you make this argument makes me wonder if I'm running trustable code on my computer at all, because apparently doing it quickly is far more important than doing it right. 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. 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. Reviews catch that. 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? Do you really expect every user to read the entire codebase for themselves and make sure that's not being done? The whole point of having all the code out in the open for independent audit purposes, to protect your security and privacy and what not is completely moot if no one else actually looks at the code anyway. And let's be honest, the code quality of some of KDE's projects - I wouldn't touch them with a six-foot pole. The ones I would touch though, all have multiple people looking at the code and reviewing everything that goes in. Let me be very clear - even if you're the best damn programmer on the planet, if *you* wrote the code, I do not trust *you* one inch to tell me that that code is correct. That verification needs to come from someone else, someone who does not have a conflict of interest in seeing that code get into production. This is nothing personal, this is confirmation bias on the author's part which leads to issues that even though they might be infrequent, usually have catastrophic impact. And if "culture" trumps over engineering best practices, it follows that I should just stop using software produced by this entity because who knows what it's doing. Thanks, Boudhayan
Re: CI system maintainability
On Thu, Mar 28, 2019, 6:36 AM Friedrich W. H. Kossebau wrote: > Am Donnerstag, 28. März 2019, 09:29:22 CET schrieb Kevin Ottens: > > 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. > Then fix what's broken. If these projects need manditory reviews fine but don't take a one-size-fits-all approach. > > > Mandatory reviews in my experience only result in more fake reviews due to > people pressuring each other to quickly get their simple patches reviewed, > lowering the general quality of reviews. > Also does the overhead reduce the number of minor improvements, where one > (as > qualified person) simply would have pushed in a minute a fix and get back > to > concentrate on the real work, instead of starting an overhead of having to > juggle with yet another patch-under-review where the current work depends > on. > > IMHO the actual problem here is: people do not do their post-push work and > care for the state on CI. > Agreed. > > From what I saw, many breakages happened with reviewed patches. Whole > releases even get done while CI is reporting failed builds, or at least > lots > of failing tests. > Requiring pre-commit hooks which run these could be helpful. They could stop this at the local machine. Perhaps also a reminder to check ci. Not sure this completely solves the issue but it would be workable for small projects like kdiff3 and would reduce overhead for minor typo correction. > > I have no idea how to fix that. We would need to ask the people for whom > this > happens why it does happen, and how we can improve things so that CI > checks > become part of their workflow. > > Cheers > Friedrich > > >
Re: CI system maintainability
On Чт, Mar 28, 2019 at 19:40, Ben Cooksley wrote: Hi all, We currently have a rather substantial issue, in that the CI system has been once again left in a position where it isn't possible to make any changes to the system. This means we can't update to newer versions of packages, add new packages or correct for binary incompatible changes which periodically get introduced to non-Frameworks. This issue has arisen because currently we have a recurring failure to build from source, within KDE PIM. Specifically, KContacts fails due to broken CMake logic. Despite this breakage having been in place for several days now, and the relevant mailing list being informed automatically by the CI system, the issue has not been corrected. While the most immediate fix is to correct this failure to build from source, that is only a short term fix and does not fix the underlying issue which makes the CI system difficult to maintain - and that is build failure reports being ignored, and people pushing broken code that doesn't even build. (For those wondering, the CI system uses OpenSUSE Tumbleweed, a rolling release distribution, for it's builds, so it isn't a case of old CMake or anything along those lines) We therefore need a long term fix for this. Note that pre-commit (as part of review) CI is not a solution in this instance, as the offending commits did not go through review. Does anyone have any ideas for a long term, proper fix to this? At this point given the amount of effort required to maintain a CI system vs. the amount of care actually being given by some developers (who are ignoring it's failure emails) it becomes questionable whether the effort is worth the return (and if not, we should just shut it down) Regards, Ben Cooksley KDE Sysadmin I don't know abut the current CI, but judging by recent discussion that is about KDE migrating to gitlab; quick search shows gitlab does allow prohibiting a merge if CI failed¹ Note however, in my experience of contributing to diffrent project CI often fails for reasons absolutely irrelevant to code being tested (e.g. errors in a CI script), in this case prohibiting a merge may worsen the situation. 1: https://docs.gitlab.com/ee/user/project/merge_requests/merge_when_pipeline_succeeds.html#only-allow-merge-requests-to-be-merged-if-the-pipeline-succeeds
Re: CI system maintainability
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... Johannes 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
Re: CI system maintainability
Kevin Ottens schrieb am Do., 28. März 2019, 09:29: > 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... We could make it mandatory with a possible backdoor. Like if you have a line in your commit log saying "I know what I am doing." then the commit could be done without review. Of course this sounds like everyone could use this backdoor always, but I doubt this would be the case. Also, unreviewed commits could have a "[not reviewed]" suffix on kde-comm...@kde.org like the License additions so that it's easier to spot unreviewed commits. We could even CC the author or respective mailing list recommending that reviews should be done for future commits. Just some thoughts :) Greetings Dominik 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. > -- > Kevin Ottens, http://ervin.ipsquad.net >
Re: CI system maintainability
Thanks for reply. More below: 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 : > > Hi Laurent, > > > > Am Donnerstag, 28. März 2019, 14:33:59 CET schrieb laurent Montel: > > > For example I works all days on kde (pim or other) when I wake up, or at > > > noon after my lunch or the evening, I will not wait several days for a > > > review because nobody has time to do it. > > > > > > (For example I make ~ 15 commits by days on pim/ruqola/framework, I > > > don't > > > want to wait several days/weeks until someone wants to review my patchs) > > > > Something might be lost in translation here, do you think, because you > > work > > daily on code of KDE projects, and other people (so potential reviewers) > > do > > not, this is an argument to do instant pushes of unreviewed commits? > > I maintain pim* from several years, I fix bugs, I improve apps, I fix all > bugs that I put in my code, so for this one I consider that I can commit > without review them (as other guys on other project that they maintain). > For framework I mainly use phabricator. I was thinking of projects where there are multiple persons contributing/ maintaining, like Frameworks. So for projects where you are the only person who has any real insight, indeed I agree to pushing directly, as I do that as well :) Because IMHO the costs for having to hope & wait for somebody else to do a "review", where one then spends lots of time rather explaining things to someone, who is not really into the project and also never might be, is not anywhere worth the time one could have otherwise invested in fixing other existing bugs or adding new features or improving code quality. If people want to get into a project, doing own patches or just watching the commits and asking normally on irc or by email about the architecture should work good enough. Abusing reviews for teaching about some project would annoy me at least, usually the patch is to fix some annoying bug or add a wanted feature, so it should get in as early as possible. > > Not sure where this is from, but often I have seen an unwritten policy > > applied where people for a patch uploaded for review after one week of no > > response add a ping and then wait another week, before finally pushing the > > change. To me this seems a fair and reasonable policy, only ignores people > > who are on vacation for some more weeks or otherwise inactive, but I have > > not seen that ever been a real issue. > > 2 weeks for a commit, for me it's too long. > I understand why people can be demotivated when they must wait long time > until having an answer. Well, 2 weeks is the time-out, only reached in worst case. Ideally people give some feedback before, which often enough happens. And indeed one also has to hunt people down to get a review, like at meetings or in chat (or trade review work with each other :) ). But isn't this the same also at work- work, unless there is a dedicated review team which needs to react by itself? Co-operating on something needs social interaction after all. > > 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? For what I can tell e.g. for KDevelop, personally I rely on the bot on #kdevelop reporting the CI state/build results, as I only look at emails rather once a day, while the chat channel is more real-time, and one also can immediately talk to peers about why some build now fails, as well as coordinate who will fix that. > > And where things could be improved, to > > prevent the current state of unhappiness for people who care about CI some > > 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? One could say, Ben has had
Re: CI system maintainability
Am Donnerstag, 28. März 2019, 16:04:01 CET schrieb Boudhayan Gupta: > I don't care if you lose time. I don't want the guys building my house to > cut corners mixing my concrete because it's going to save time. There is a difference here though, no? The people building your house will not live in that house. They only make money from building, so: less effort & lesser time for same money received -> better. We here create software we use ourselves, no? So we are building our own house, and would not be expected to cut corners on our own grounds. > 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. Hmmm... (I cannot stop myself typing the following :) ) In that case, I have to immediately notify you to make sure that you remove Okteta from any of the devices you have reach to, if you even ever installed it, best recommend your distribution to delete the package. Because shockingly all the >4000 commits of its >100k lines of code have been done without review. So it surely is an insane pile of crap by presumption, unless finally someone will give it another pair of eyeballs. Though I assume no-one is using it anyway, given there are so few bugs reported ;) > 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. Reviews catch that. This sounds to me like review is magically preventing bugs. Well, it increases the chance things get catched. Though reviews also increase the chance to be sloppy, as there is: review to catch that. Then after all is the reviewer also a developer, who also only sees a one-liner, where they think "meh, this is a tiny little thing, I don't have to be careful". A review is only useful if the reviewers are qualified and invest the proper resources. I prefer one responsible experienced developer over an unresponsible unexperienced developer & unresponsible unexperienced reviewer any time. More I prefer of course one responsible experienced developer & one responsible experienced reviewer :) Based on my personal experience bubble, not by any scientific means. Cheers Friedrich
Re: CI system maintainability
On Thursday, 28 March 2019 16:11:12 CET Friedrich W. H. Kossebau wrote: > Am Donnerstag, 28. März 2019, 14:33:59 CET schrieb laurent Montel: > > For example I works all days on kde (pim or other) when I wake up, or at > > noon after my lunch or the evening, I will not wait several days for a > > review because nobody has time to do it. > > > > (For example I make ~ 15 commits by days on pim/ruqola/framework, I don't > > want to wait several days/weeks until someone wants to review my patchs) > > Something might be lost in translation here, do you think, because you work > daily on code of KDE projects, and other people (so potential reviewers) do > not, this is an argument to do instant pushes of unreviewed commits? > > While I understand one can get impatient if not getting instant review of > changes one would like to depend on with further changes (I know this well > :) ), still this seems a flawed argument at least for > part-time-contributors based KDE projects, where the people one co-operates > with only have time now and then, like once per week. It could be seen > unfair & ignorant to them if one simply ignores their opinion, because one > has more time reserved/ available. I don't think any of that was meant here. The scenario that Laurent has in mind I think, and that I'm facing too, is that putting up a few dozen patches a week in a single repository for review and then having to wait for the review timeout because there's nobody else working on it is not even remotely practical, let alone with the current toolset of arc/phab. > Not sure where this is from, but often I have seen an unwritten policy > applied where people for a patch uploaded for review after one week of no > response add a ping and then wait another week, before finally pushing the > change. To me this seems a fair and reasonable policy, only ignores people > who are on vacation for some more weeks or otherwise inactive, but I have > not seen that ever been a real issue. This works fine if you have less than a handful of patches in a single repo, and people actually review things. And we make plenty of use of that: https://phabricator.kde.org/people/revisions/196/ https://phabricator.kde.org/people/revisions/208/ In fact I was just criticized last weekend at the privacy sprint for sending too many reviews ;-) Regards, Volker signature.asc Description: This is a digitally signed message part.
Re: CI system maintainability
On Thu, Mar 28, 2019 at 3:57 PM David Jarvie wrote: > I agree. Mandatory reviews might work if there is a team of active people > working on a project, but if there is only one person with real knowledge of > the code We do have common ownership of code, so if there is only one person with real knowledge of the code that is a problem in of itself... A problem which really should get solved... For example by having mandatory reviews so someone actually has to review code they know just as little about as everyone else, so in turn they now know a bit more and can more confidently do reviews ;) Now to be sure, I am not certain mandatory reviews are in fact the answer to the problem at hand, nor if they would in fact be possible to implement reliable. From personal experience I'll say that reviews almost always are worth it, even for the simple typo fixes. And I also almost find someone to give at least a casual review even when they know nothing of the code base. Sometimes perhaps even "I couldn't say if this change is good, but the code looks correct at least +1" is better than nobody having looked at the code at all. It ultimately also becomes a matter of busfactor. If nobody ever has reason to look at code with a single principal author the busfactor will never improve. HS
Re: CI system maintainability
On Thursday, 28 March 2019 16:32:34 CET Luca Beltrame wrote: > In data giovedì 28 marzo 2019 15:15:23 CET, Nate Graham ha scritto: > > In this case, it seems like the problem is that there are certain > > individuals or teams that are pushing risky, breaking changes without code > > review, and then ignoring failures in the CI. I think we might do well to > > try to answer the question of why that's happening before we create a new > > rule aimed at stopping it. That "risky breaking change" was a 5 line fix for a Qt 5.13 build issue that had been successfully deployed to multiple repos before. The failure was also not ignored but noticed and fixed within about an hour, just not in all affected branches. > Well put. Review or not review, clearly something in the process has failed, > and I suspect "friction" rather than actual bad-faith ignorance of the CI > results. > > So, perhaps to force myself out of the bikeshedding (mea culpa) on reviews, > I'll steal Friedrich's points from another mail and put them here > synthetically: > > - Why are the CI mails ignored / filtered? Too many, hard to parse, > difficult to interpret? > - What can be done to have people look at them and make sure they don't > overlook breakage? > > At least for the second point, as I mentioned earlier in the thread, > probably having the committer being mailed in case of failure (GitLab does > this IIRC) might be better than just on the mailing list. The second > approach is what we use in openSUSE, where I usually don't subscribe to > failure notifications (almost 300 packages is overkill) but a bot starts > pestering me with mails the moment build failures go unfixed (granted, the > time scale is different). > > For the first, I'd like people more involved in the development to say their > word. See my earlier mail in this thread on how this slipped through for me. Regards, Volker signature.asc Description: This is a digitally signed message part.
Re: CI system maintainability
Le jeudi 28 mars 2019, 16:11:12 CET Friedrich W. H. Kossebau a écrit : > Hi Laurent, > > Am Donnerstag, 28. März 2019, 14:33:59 CET schrieb laurent Montel: > > For example I works all days on kde (pim or other) when I wake up, or at > > noon after my lunch or the evening, I will not wait several days for a > > review because nobody has time to do it. > > > > (For example I make ~ 15 commits by days on pim/ruqola/framework, I don't > > want to wait several days/weeks until someone wants to review my patchs) > > Something might be lost in translation here, do you think, because you work > daily on code of KDE projects, and other people (so potential reviewers) do > not, this is an argument to do instant pushes of unreviewed commits? I maintain pim* from several years, I fix bugs, I improve apps, I fix all bugs that I put in my code, so for this one I consider that I can commit without review them (as other guys on other project that they maintain). For framework I mainly use phabricator. > > While I understand one can get impatient if not getting instant review of > changes one would like to depend on with further changes (I know this well > :) ), still this seems a flawed argument at least for > part-time-contributors based KDE projects, where the people one co-operates > with only have time now and then, like once per week. It could be seen > unfair & ignorant to them if one simply ignores their opinion, because one > has more time reserved/ available. KPIM doesn't have a big active team... > Not sure where this is from, but often I have seen an unwritten policy > applied where people for a patch uploaded for review after one week of no > response add a ping and then wait another week, before finally pushing the > change. To me this seems a fair and reasonable policy, only ignores people > who are on vacation for some more weeks or otherwise inactive, but I have > not seen that ever been a real issue. 2 weeks for a commit, for me it's too long. I understand why people can be demotivated when they must wait long time until having an answer. I know that I don't participate a lot on qtcreator dev as we need to wait long time to have some review... > > 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. > And where things could be improved, to > prevent the current state of unhappiness for people who care about CI some > 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. If I received directly a message about kcontact 19.04 after 1 days I fixed it directly. Master build correctly and unfortunately I don't have 19.04 and master in parallel. Regards > > Cheers > Friedrich -- Laurent Montel | laurent.mon...@kdab.com | KDE/Qt Senior Software Engineer KDAB (France) S.A.S., a KDAB Group company Tel. France +33 (0)4 90 84 08 53, www.kdab.fr KDAB - The Qt, C++ and OpenGL Experts - Platform-independent software solutions
Re: CI system maintainability
On Thursday, March 28, 2019 3:39:54 PM CET Friedrich W. H. Kossebau wrote: > Am Donnerstag, 28. März 2019, 11:27:44 CET schrieb Daniel Vrátil: > > I'm completely fine with mandatory code review for everything and I'd be > > happy to have this in PIM. I think the biggest problem in PIM to overcome > > will be finding the reviewers - I dare say I'm currently the only one who > > has at least a little idea about what's going on in Akonadi, so getting > > for > > instance Laurent to review my Akonadi patches might be hard - same for me > > reviewing Laurent's KMail patches. This will require non-trivial amount of > > effort for all members of the community to gain deeper understanding of > > the > > other components within the project and a willingness to step up and do a > > code review even if they don't feel 100% comfortable with the code base. > > But I believe that in the long run the benefits clearly out-weight the > > cost. > So why do you not do this already? Why would you only do this is there is a > policy requiring you to do it? > And why do you think this policy-driven behavioural change will work or is > needed with everyone else? I wrote this with my PIM hat on and targeted the PIM community, without realizing this is a cross-post, so I apologize for the confusion. I believe each project should choose whatever workflow and policy suits them. That said, I do put up for review everything that is not Akonadi/PIM core (even changes to PIM components that are not "mine"). The reason I don't do this for Akonadi is that there's no-one really to review my code, because no- one else works on it, or at least that has been my perception of the situation lately. I've been thinking about bringing this topic up on the upcoming PIM sprint as I would like to have my code reviewed, I think it's a very good and healthy thing, but in case of KDE PIM, I think we need to agree that we'll actually review each others code, even if it's not "our own" code-base. > Also, how do you ensure the review is actually of quality? How do you ensure that the code people commit without a review is of quality? > And not just socially driven "+1 for my buddie!", where buddy then also > mentally shifts part of responsibility to other buddy, because, he, more > eyes means I do not need to do the full work myself, glitches will be > caught be them surely! Reviewer found a code formatting issue, done here, > review happened. This is a fair point, and I certainly am guilty of doing this occasionally in the past. But even reviewer can have a reviewer: if you keep accepting patches of "dubious quality" (break build, don't work, contain typos ...), someone else from the community will eventually notice and point out you should maybe be more careful with your reviews or pay attention to certain aspects (like "does it actually work?). > The opposite also has been seen, reviewers feel they need to do "real" > review and find things, so start to nitpick or to demand wrong changes, > wasting everyone's time. > > For myself I know I will happily have other people review my patches, but > only if there are qualified people to be expected to do it with the needed > quality in a reasonable time. > > Then, trying to force by that policy other people to become proper > specialist for certain other code projects to do qualified reviews, > actually is a trade- off. They will have less time for their actual code > project, becoming less a specialist there (or having time to review other > people's contribution to that project). > We need more contributors, not force existing contributors to distribute > their abilities & resources over more projects (and thus having less for > their actual one). AFAIU the policy right now is keep putting up patches for review until you get commit access, then continue doing so until you feel like you don't have to anymore. But having experienced contributors putting up their code for review is a good thing as it allows newcomers (and not just them!) to better understand what is happening in the project and simple reviews can be a good starting point for them to get more involved in the community. > I am also not aware of many contributors to KDE projects who are not capable > to make a responsible decision between the few obvious simple fixes and > those normal changes which better get feedback from others first. If one is > unsure about one's post-beer late-night quick hack, one will upload it for > review, no? At least if one's previous late-night commits turned out to be > late-night mental state impacted. This I think is somewhat related to the tooling as I said in my previous email. If your workflow for simple obvious fix would be "git push HEAD:refs/ for/master" instead of "git push master", you wouldn't care. If that means using arc, I can understand > To deal with those few which seem challenged to do that decision properly, I > find such a policy for everyone harmful, I
Re: CI system maintainability
Hello, On Thursday, 28 March 2019 16:11:12 CET Friedrich W. H. Kossebau wrote: > Am Donnerstag, 28. März 2019, 14:33:59 CET schrieb laurent Montel: > > For example I works all days on kde (pim or other) when I wake up, or at > > noon after my lunch or the evening, I will not wait several days for a > > review because nobody has time to do it. > > > > (For example I make ~ 15 commits by days on pim/ruqola/framework, I don't > > want to wait several days/weeks until someone wants to review my patchs) > > Something might be lost in translation here, do you think, because you work > daily on code of KDE projects, and other people (so potential reviewers) do > not, this is an argument to do instant pushes of unreviewed commits? > > While I understand one can get impatient if not getting instant review of > changes one would like to depend on with further changes (I know this well > :) ), That particular point is in part a tooling problem though. Phab doesn't make this situation easy to handle. I have to do very naughty things to git and arc to deal with those. I'm guilty of often piling a dozen patches and rewriting extensively my history locally before the lots hits the first round of reviews. :-) > still this seems a flawed argument at least for part-time-contributors based > KDE projects, where the people one co-operates with only have time now and > then, like once per week. It could be seen unfair & ignorant to them if one > simply ignores their opinion, because one has more time reserved/ available. This is one of the big flaws of self-proclaimed meritocratic communities. You can out-commit someone and end up being the big decision maker hard to convince. Doesn't happen by malice in most cases I think, but it's a clear slippery slope. > Not sure where this is from, but often I have seen an unwritten policy > applied where people for a patch uploaded for review after one week of no > response add a ping and then wait another week, before finally pushing the > change. To me this seems a fair and reasonable policy, only ignores people > who are on vacation for some more weeks or otherwise inactive, but I have > not seen that ever been a real issue. Agreed. It makes sense. > Given the actual purpose of this thread, I would be more curious how you > have CI integrated in your workflow? And where things could be improved, to > prevent the current state of unhappiness for people who care about CI some > 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 :) Definitely interested in this as well. It's not the first time we have Ben having to escalate to some form of threat regarding the state of the CI in PIM components. Although it's clear from the kde-pim list that the CI is making a lot of noise there. Either we collectively became too good at ignoring those emails, or it's too verbose (after all it's one email per component per build type, it piles up quickly). Regards. -- Kevin Ottens, http://ervin.ipsquad.net signature.asc Description: This is a digitally signed message part.
Re: CI system maintainability
In data giovedì 28 marzo 2019 15:15:23 CET, Nate Graham ha scritto: > In this case, it seems like the problem is that there are certain > individuals or teams that are pushing risky, breaking changes without code > review, and then ignoring failures in the CI. I think we might do well to > try to answer the question of why that's happening before we create a new > rule aimed at stopping it. Well put. Review or not review, clearly something in the process has failed, and I suspect "friction" rather than actual bad-faith ignorance of the CI results. So, perhaps to force myself out of the bikeshedding (mea culpa) on reviews, I'll steal Friedrich's points from another mail and put them here synthetically: - Why are the CI mails ignored / filtered? Too many, hard to parse, difficult to interpret? - What can be done to have people look at them and make sure they don't overlook breakage? At least for the second point, as I mentioned earlier in the thread, probably having the committer being mailed in case of failure (GitLab does this IIRC) might be better than just on the mailing list. The second approach is what we use in openSUSE, where I usually don't subscribe to failure notifications (almost 300 packages is overkill) but a bot starts pestering me with mails the moment build failures go unfixed (granted, the time scale is different). For the first, I'd like people more involved in the development to say their word. -- Luca Beltrame GPG key ID: A29D259B signature.asc Description: This is a digitally signed message part.
Re: CI system maintainability
In data giovedì 28 marzo 2019 16:04:01 CET, Boudhayan Gupta ha scritto: > I don't get why mandatory code reviews are so unpopular. It's not "unpopular". As far as the discussion goes, the opinions (from several parties) say that they're not a silver bullet, and that some projects benefit from them more than others. The ultimate solution is actually more developers (yeah, I know, easy...). CI, OTOH, has been IMO very useful (despite the headaches Ben mentions) for all the projects in KDE. > I don't care if you lose time. I don't want the guys building my house to You should if the review stays there for years when there's no one else to review it. > As a user, I simply do not want unreviewed crap running on my computer. Well, reviews help but they're just part of the equation. CI helps as well (and IMO, it should be more visible as I mentioned earlier in the thread). And perfectly reviewed code (as well as unreviewed code) can still be a problem (as an integrator, I see that often). > 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. I would say that there's no need to be like this. There is bound to be disagreement (and there is) but not as much as to define quality on assumptions. To be clear: I'm neither on the side of "review all the things" nor on the anarchist side. I just want to make sure we don't engage in policies that can be (potentially, just potentially) harmful for some parts of KDE (while they are perfectly OK for others). -- Luca Beltrame GPG key ID: A29D259B signature.asc Description: This is a digitally signed message part.
Re: CI system maintainability
Hi Laurent, Am Donnerstag, 28. März 2019, 14:33:59 CET schrieb laurent Montel: > For example I works all days on kde (pim or other) when I wake up, or at > noon after my lunch or the evening, I will not wait several days for a > review because nobody has time to do it. > > (For example I make ~ 15 commits by days on pim/ruqola/framework, I don't > want to wait several days/weeks until someone wants to review my patchs) Something might be lost in translation here, do you think, because you work daily on code of KDE projects, and other people (so potential reviewers) do not, this is an argument to do instant pushes of unreviewed commits? While I understand one can get impatient if not getting instant review of changes one would like to depend on with further changes (I know this well :) ), still this seems a flawed argument at least for part-time-contributors based KDE projects, where the people one co-operates with only have time now and then, like once per week. It could be seen unfair & ignorant to them if one simply ignores their opinion, because one has more time reserved/ available. Not sure where this is from, but often I have seen an unwritten policy applied where people for a patch uploaded for review after one week of no response add a ping and then wait another week, before finally pushing the change. To me this seems a fair and reasonable policy, only ignores people who are on vacation for some more weeks or otherwise inactive, but I have not seen that ever been a real issue. Given the actual purpose of this thread, I would be more curious how you have CI integrated in your workflow? And where things could be improved, to prevent the current state of unhappiness for people who care about CI some 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 :) Cheers Friedrich
Re: CI system maintainability
On 28 March 2019 13:33:59 GMT, laurent Montel wrote: > Le jeudi 28 mars 2019, 09:29:22 CET Kevin Ottens a écrit : > > 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. > > Hi, > > I am against to force mandatory review, as it will create a lot of > lose of > time, and we will not be sure that review is correct (see comment from > Volker > about "transaction lock regression") > > It's necessary to having a big team for doing it. > > Ok a repo was broken, but it was just that fix was created in master > not > 19.04, I didn't see nobody on IRC told us "this package is always > broken", > when I saw it this morning I just cherry pick (2 seconds for fixing > it). > > > For example I works all days on kde (pim or other) when I wake up, or > at noon > after my lunch or the evening, I will not wait several days for a > review > because nobody has time to do it. (we can see a review from zanshin > for > example https://phabricator.kde.org/D16210 we can see that David > waited 2 > months until having an answer...). > > (For example I make ~ 15 commits by days on pim/ruqola/framework, I > don't want > to wait several days/weeks until someone wants to review my patchs) > > I will not lose my time to review some code that I don't understand... > I never > reviewed Akonadi patch as I don't understand this code, and I will > take time > on it just for the pleasure as I prefer fixing bug or adding new > features in > components that I maintain. > > When we have a big team as Qt team it can help but in pim component > where we > don't have any redundant guy, we will lose a lot of time. > > So for each increase version for each package I will wait a review. > For sure > not. > > Each time that I will improve code as coding style I will wait that > someone > wants to review... I agree. Mandatory reviews might work if there is a team of active people working on a project, but if there is only one person with real knowledge of the code, or there is nobody else with much time to spare, who is going to do the review? It is likely to just sit waiting indefinitely. If getting code reviewed is too difficult, the developer may have to give up and abandon the project. Mandatory reviewing could only work if individual projects can decide whether to adopt it or not. -- David Jarvie KAlarm author, KDE developer http://www.astrojar.org.uk/kalarm
Re: CI system maintainability
Am Donnerstag, 28. März 2019, 11:27:44 CET schrieb Daniel Vrátil: > I'm completely fine with mandatory code review for everything and I'd be > happy to have this in PIM. I think the biggest problem in PIM to overcome > will be finding the reviewers - I dare say I'm currently the only one who > has at least a little idea about what's going on in Akonadi, so getting for > instance Laurent to review my Akonadi patches might be hard - same for me > reviewing Laurent's KMail patches. This will require non-trivial amount of > effort for all members of the community to gain deeper understanding of the > other components within the project and a willingness to step up and do a > code review even if they don't feel 100% comfortable with the code base. > But I believe that in the long run the benefits clearly out-weight the > cost. So why do you not do this already? Why would you only do this is there is a policy requiring you to do it? And why do you think this policy-driven behavioural change will work or is needed with everyone else? Also, how do you ensure the review is actually of quality? And not just socially driven "+1 for my buddie!", where buddy then also mentally shifts part of responsibility to other buddy, because, he, more eyes means I do not need to do the full work myself, glitches will be caught be them surely! Reviewer found a code formatting issue, done here, review happened. The opposite also has been seen, reviewers feel they need to do "real" review and find things, so start to nitpick or to demand wrong changes, wasting everyone's time. For myself I know I will happily have other people review my patches, but only if there are qualified people to be expected to do it with the needed quality in a reasonable time. Then, trying to force by that policy other people to become proper specialist for certain other code projects to do qualified reviews, actually is a trade- off. They will have less time for their actual code project, becoming less a specialist there (or having time to review other people's contribution to that project). We need more contributors, not force existing contributors to distribute their abilities & resources over more projects (and thus having less for their actual one). I am also not aware of many contributors to KDE projects who are not capable to make a responsible decision between the few obvious simple fixes and those normal changes which better get feedback from others first. If one is unsure about one's post-beer late-night quick hack, one will upload it for review, no? At least if one's previous late-night commits turned out to be late-night mental state impacted. To deal with those few which seem challenged to do that decision properly, I find such a policy for everyone harmful, I know it would impact my contribution willingness, when I come across a typo or other simple and obvious to fix things. And just leave the garbage around along my current working path. Besides, it will not solve the issue this thread is about, with some people not caring (quickly) enough if CI builds fail or if there are regressions in tests. Review builds once implemented and deployed will help there partially as a side-effect only, catching some build fails before. But also not if this breaks (e.g. due to API/behavioural changes) depending projects, as CI at least currently does not rebuild dependent projects (cmp. also T7374). A society with people doing things only due to policies and not intrinsic motivation seems very flawed to me, makes me wonder why people are in there given no-one forced them into this society. https://community.kde.org/Policies/ Commit_Policy#Code_review_by_other_developers has some policies already, which should be enough: 1.1 Think Twice before Committing 1.2 Never commit code that doesn't compile 1.3 Test your changes before committing 1.4 Double check what you commit 1.10 Code review by other developers 1.11 Take responsibility for your commits 1.12 Don't commit code you don't understand Well, perhaps could be amended by an explicit note about keeping CI working. Enforcing review of any commits IMHO should be only done for people who notoriously failed to comply with those existing rules. If we are unable to pinpoint those people, talk with them and make them comply or sort out their reasons to not yet comply, but instead create as workaround new generic policies for everyone, we make this a worse society. And also a less effective, with more rubber stamps needed. Cheers Friedrich
Re: CI system maintainability
Hello, On Thursday, 28 March 2019 14:33:59 CET laurent Montel wrote: > I am against to force mandatory review, as it will create a lot of lose of > time, As I said, unpopular. > and we will not be sure that review is correct (see comment from > Volker about "transaction lock regression") This argument is absurd: "Hey, this vest isn't 100% bullet-proof, thus I'll walk in the middle of a war naked". > It's necessary to having a big team for doing it. I agree having a team of 1 for each KDE PIM component is a problem (and often the same person). > Ok a repo was broken, but it was just that fix was created in master not > 19.04, I didn't see nobody on IRC told us "this package is always broken", > when I saw it this morning I just cherry pick (2 seconds for fixing it). > > For example I works all days on kde (pim or other) when I wake up, or at > noon after my lunch or the evening, I will not wait several days for a > review because nobody has time to do it. > (we can see a review from zanshin for example https://phabricator.kde.org/ > D16210 we can see that David waited 2 months until having an answer...). Unfair, if you read the comments you'll see that this particular patch didn't appear in my dashboard for some odd reason (I suspect it comes from how arc associates a patch to a repository or not, but I digress...). > (For example I make ~ 15 commits by days on pim/ruqola/framework, I don't > want to wait several days/weeks until someone wants to review my patchs) > > I will not lose my time to review some code that I don't understand... I > never reviewed Akonadi patch as I don't understand this code, and I will > take time on it just for the pleasure as I prefer fixing bug or adding new > features in components that I maintain. > > When we have a big team as Qt team it can help but in pim component where we > don't have any redundant guy, we will lose a lot of time. Ah, the mythical big team... Because it's well known that on Qt the reviewers aren't stretch thin and that patches never end up weeks in limbo... Also you know that if you don't change your way of working you'll stay alone on your components? I know, chicken and egg... but all those commits and stuff randomly changing all the time doesn't help people to get in even if they'd want to. But you know that very well, we just discussed it half a dozen times in the past... > So for each increase version for each package I will wait a review. For sure > not. > > Each time that I will improve code as coding style I will wait that someone > wants to review... > > > I know that it's easy to write "make reviews mandatory" there is not an > impact about your work as we know that you are not really active on kde at > the moment... Right, let's bring something irrelevant to the discussion. You know it's mainly due to health issues the past few months right? (yes, March has been a real joy again... it keeps on giving) Besides, I apply mandatory reviews to myself on zanshin. > For sure I broke kcontact 2 days ago, but as a friend told me when I started > to work on kde "We can't create a bug when we don't code..." And this is true, writing code will create bugs, that's why responsible people like safety nets even to the price of throughput. Now can we use a more adult tone again? Maybe re-read Dan's email again who has a more balanced view despite being in a similar situation? Regards. -- Kevin Ottens, http://ervin.ipsquad.net signature.asc Description: This is a digitally signed message part.
Re: CI system maintainability
Le jeudi 28 mars 2019, 09:29:22 CET Kevin Ottens a écrit : > 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. Hi, I am against to force mandatory review, as it will create a lot of lose of time, and we will not be sure that review is correct (see comment from Volker about "transaction lock regression") It's necessary to having a big team for doing it. Ok a repo was broken, but it was just that fix was created in master not 19.04, I didn't see nobody on IRC told us "this package is always broken", when I saw it this morning I just cherry pick (2 seconds for fixing it). For example I works all days on kde (pim or other) when I wake up, or at noon after my lunch or the evening, I will not wait several days for a review because nobody has time to do it. (we can see a review from zanshin for example https://phabricator.kde.org/D16210 we can see that David waited 2 months until having an answer...). (For example I make ~ 15 commits by days on pim/ruqola/framework, I don't want to wait several days/weeks until someone wants to review my patchs) I will not lose my time to review some code that I don't understand... I never reviewed Akonadi patch as I don't understand this code, and I will take time on it just for the pleasure as I prefer fixing bug or adding new features in components that I maintain. When we have a big team as Qt team it can help but in pim component where we don't have any redundant guy, we will lose a lot of time. So for each increase version for each package I will wait a review. For sure not. Each time that I will improve code as coding style I will wait that someone wants to review... I know that it's easy to write "make reviews mandatory" there is not an impact about your work as we know that you are not really active on kde at the moment... For sure I broke kcontact 2 days ago, but as a friend told me when I started to work on kde "We can't create a bug when we don't code..." Regards -- Laurent Montel | laurent.mon...@kdab.com | KDE/Qt Senior Software Engineer KDAB (France) S.A.S., a KDAB Group company Tel. France +33 (0)4 90 84 08 53, www.kdab.fr KDAB - The Qt, C++ and OpenGL Experts - Platform-independent software solutions
Re: CI system maintainability
Hello, On Thursday, 28 March 2019 11:27:44 CET Daniel Vrátil wrote: > I'm completely fine with mandatory code review for everything and I'd be > happy to have this in PIM. I think the biggest problem in PIM to overcome > will be finding the reviewers - I dare say I'm currently the only one who > has at least a little idea about what's going on in Akonadi, so getting for > instance Laurent to review my Akonadi patches might be hard - same for me > reviewing Laurent's KMail patches. This will require non-trivial amount of > effort for all members of the community to gain deeper understanding of the > other components within the project and a willingness to step up and do a > code review even if they don't feel 100% comfortable with the code base. > But I believe that in the long run the benefits clearly out-weight the > cost. This! :-) > Btw we practice this policy at work and I do truly appreciate it, not only > as a huge learning experience but so many times just having a second pair > of eyes to glance over my code has revealed issues that sometimes almost > make me question my career choice as a programmer :-) I think this is > especially important for projects like PIM, where most of us contribute at > work in between waiting for CI and replying to 15 Slack threads or in the > evening after a long day And this too of course. I fully support this message. ;-) Cheers. -- Kevin Ottens, http://ervin.ipsquad.net signature.asc Description: This is a digitally signed message part.
Re: CI system maintainability
Am Donnerstag, 28. März 2019, 09:29:22 CET schrieb Kevin Ottens: > 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. Mandatory reviews in my experience only result in more fake reviews due to people pressuring each other to quickly get their simple patches reviewed, lowering the general quality of reviews. Also does the overhead reduce the number of minor improvements, where one (as qualified person) simply would have pushed in a minute a fix and get back to concentrate on the real work, instead of starting an overhead of having to juggle with yet another patch-under-review where the current work depends on. IMHO the actual problem here is: people do not do their post-push work and care for the state on CI. From what I saw, many breakages happened with reviewed patches. Whole releases even get done while CI is reporting failed builds, or at least lots of failing tests. I have no idea how to fix that. We would need to ask the people for whom this happens why it does happen, and how we can improve things so that CI checks become part of their workflow. Cheers Friedrich
Re: CI system maintainability
On Thursday, March 28, 2019 9:50:47 AM CET Kevin Ottens wrote: > Hello, > > On Thursday, 28 March 2019 09:41:29 CET Luca Beltrame wrote: > > In data giovedì 28 marzo 2019 09:29:22 CET, Kevin Ottens ha scritto: > > > 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. > > > > I'm not sure I agree. I can't speak for seasoned developers, but I've > > found > > myself in a situation (more than once) where the fix is trivial (compile > > error, missing ";", etc) and being forced to go through review would (IMO) > > unnecessarily raise friction. This is partially a problem of tooling IMO - review for even a trivial change will not cause any friction, if the tooling makes it super-easy and natural part of your workflow (and then you can always poke someone on IRC "hey, do you have 5 seconds for this super-simple review?"). Using arc with Phab is a bit annoying, so I can understand people fighting this. Gitlab with their "click this link to turn your commit into a merge request" is much better, plus it can be merged by the reviewer with a single click so you as a developer don't even need to care about the MR anymore. > I don't fully disagree with this (although I'd have stories on nefarious > typos even for what was supposed to be a "trivial fix"). But it becomes a > question of trade-off, IOW "how hurtful to the project mandatory reviews?" > vs "how hurtful to the project a central component being very regularly > broken?". > > I'd argue we're loosing more with the current state of PIM than we'd loose > with mandatory reviews. I'm completely fine with mandatory code review for everything and I'd be happy to have this in PIM. I think the biggest problem in PIM to overcome will be finding the reviewers - I dare say I'm currently the only one who has at least a little idea about what's going on in Akonadi, so getting for instance Laurent to review my Akonadi patches might be hard - same for me reviewing Laurent's KMail patches. This will require non-trivial amount of effort for all members of the community to gain deeper understanding of the other components within the project and a willingness to step up and do a code review even if they don't feel 100% comfortable with the code base. But I believe that in the long run the benefits clearly out-weight the cost. Btw we practice this policy at work and I do truly appreciate it, not only as a huge learning experience but so many times just having a second pair of eyes to glance over my code has revealed issues that sometimes almost make me question my career choice as a programmer :-) I think this is especially important for projects like PIM, where most of us contribute at work in between waiting for CI and replying to 15 Slack threads or in the evening after a long day Cao, Dan > Regards. -- Daniel Vrátil www.dvratil.cz | dvra...@kde.org IRC: dvratil on Freenode (#kde, #kontact, #akonadi, #fedora-kde) GPG Key: 0x4D69557AECB13683 Fingerprint: 0ABD FA55 A4E6 BEA9 9A83 EA97 4D69 557A ECB1 3683 signature.asc Description: This is a digitally signed message part.
Re: CI system maintainability
On Thursday, 28 March 2019 09:50:47 CET Kevin Ottens wrote: > Hello, > > On Thursday, 28 March 2019 09:41:29 CET Luca Beltrame wrote: > > In data giovedì 28 marzo 2019 09:29:22 CET, Kevin Ottens ha scritto: > > > 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. > > > > I'm not sure I agree. I can't speak for seasoned developers, but I've > > found > > myself in a situation (more than once) where the fix is trivial (compile > > error, missing ";", etc) and being forced to go through review would (IMO) > > unnecessarily raise friction. > > I don't fully disagree with this (although I'd have stories on nefarious > typos even for what was supposed to be a "trivial fix"). But it becomes a > question of trade-off, IOW "how hurtful to the project mandatory reviews?" > vs "how hurtful to the project a central component being very regularly > broken?". > > I'd argue we're loosing more with the current state of PIM than we'd loose > with mandatory reviews. Review all relevant changes is nice in theory, but for this to work you need at least two people spending comparable amount of time on this. I wish we had that luxury in KDE PIM. It works to some extend for Akonadi between Dan and David, I don't see it working for Laurent on KMail or for me on KItinerary, there's simply not enough review bandwidth there. Also, when looking at such drastic changes we should keep in mind the amount of changes that go in without trouble. There's always the risk of breakage when we change stuff with the best intentions of improving things, we need to deal with that no matter how many people review a change (see the nasty transaction lock regression in 18.12.x Akonadi). What failed in this specific case is not the lack of review IMHO, I'm not sure I would have spotted the issue from the diff. It's also not that nobody cared, or that people ignored the issue. The underlying problem was fixed within 62 minutes of its occurrence according to the Git log, it was however forgotten to push this to the 19.04 branch. This resulted in a single build failure in the stable build for kcontacts, which I (and I guess others too) did not immediately act on. That does not mean it's being ignored, but for a change I did not do myself the overwhelming majority of failures is transient (as either the author fixes it upon being notified, or it's a dependency issue that will resolve itself with the next build). That single build failure resulted in the dependent builds failing, which again I did not immediately act on as the error message made me believe it's the a dependency issue that will resolve itself. Combined with the fact that this is in the stable branch, which is less active than master, I had indeed not realized we have a persistent issue here that nobody else felt responsible for until I saw this email. At that point Laurent had already fixed it btw, which was a matter of a simple cherry-pick from master. If I missed other earlier communication about this somehow, I apologize of course. So, yes, things went wrong and it's a valid question how to improve this going forward. But rather than questioning the usefulness of the CI or the entire development workflow, how about just simply pinging the people who feel responsible for the affected repos? It's not like we miss every single build issue after all. I simply might not always manage to keep the state of 120+ repos in various configurations in my head, and which of those needs most urgent attention (I for example broke half of binary factory's Android builds with a kpackage change recently, despite review, and yet have to find the time for a proper fix for that). Regards, Volker signature.asc Description: This is a digitally signed message part.
Re: CI system maintainability
Hello, On Thursday, 28 March 2019 10:35:37 CET Luca Beltrame wrote: > In data giovedì 28 marzo 2019 10:32:39 CET, Kevin Ottens ha scritto: > > OK, to be fair not 100% today's situation because of the above. It was > > based on best judgment maybe we're missing such a set of guidelines. I > > admit I'm slightly doubtful though. > > I can't claim it may work 100%, but I've seen other communities (many, many > years ago) where a "semi anarchy" replaced by "iron-gripped rules" from one > day to another actually killed them. I wouldn't call that iron grip from one day to the other though. It's not like we've not been working toward mandatory review for the past ten years or so. Most KDE projects de facto apply mandatory reviews AFAICT. Regards. -- Kevin Ottens, http://ervin.ipsquad.net signature.asc Description: This is a digitally signed message part.
Re: CI system maintainability
In data giovedì 28 marzo 2019 10:32:39 CET, Kevin Ottens ha scritto: > OK, to be fair not 100% today's situation because of the above. It was based > on best judgment maybe we're missing such a set of guidelines. I admit I'm > slightly doubtful though. I can't claim it may work 100%, but I've seen other communities (many, many years ago) where a "semi anarchy" replaced by "iron-gripped rules" from one day to another actually killed them. -- Luca Beltrame GPG key ID: A29D259B signature.asc Description: This is a digitally signed message part.
Re: CI system maintainability
Hello, On Thursday, 28 March 2019 10:08:54 CET 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. I'm almost tempted to reply "been there, done that". It's kind of the situation we have today. > 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) OK, to be fair not 100% today's situation because of the above. It was based on best judgment maybe we're missing such a set of guidelines. I admit I'm slightly doubtful though. Regards. -- Kevin Ottens, http://ervin.ipsquad.net signature.asc Description: This is a digitally signed message part.
Re: CI system maintainability
In data giovedì 28 marzo 2019 10:17:18 CET, Tomaz Canabrava ha scritto: > 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. It's a "social" problem and not a technical one: you can see it across repositories managed by different groups. Some are very used to reviews, others not (and not necessarily PIM). Hence the (even debatable, if you may) proposal. -- Luca Beltrame GPG key ID: A29D259B signature.asc Description: This is a digitally signed message part.
Re: CI system maintainability
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
Re: CI system maintainability
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. -- Luca Beltrame GPG key ID: A29D259B signature.asc Description: This is a digitally signed message part.
Re: CI system maintainability
Hello, On Thursday, 28 March 2019 09:41:29 CET Luca Beltrame wrote: > In data giovedì 28 marzo 2019 09:29:22 CET, Kevin Ottens ha scritto: > > 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. > > I'm not sure I agree. I can't speak for seasoned developers, but I've found > myself in a situation (more than once) where the fix is trivial (compile > error, missing ";", etc) and being forced to go through review would (IMO) > unnecessarily raise friction. I don't fully disagree with this (although I'd have stories on nefarious typos even for what was supposed to be a "trivial fix"). But it becomes a question of trade-off, IOW "how hurtful to the project mandatory reviews?" vs "how hurtful to the project a central component being very regularly broken?". I'd argue we're loosing more with the current state of PIM than we'd loose with mandatory reviews. Regards. -- Kevin Ottens, http://ervin.ipsquad.net signature.asc Description: This is a digitally signed message part.
Re: CI system maintainability
In data giovedì 28 marzo 2019 09:29:22 CET, Kevin Ottens ha scritto: > 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. I'm not sure I agree. I can't speak for seasoned developers, but I've found myself in a situation (more than once) where the fix is trivial (compile error, missing ";", etc) and being forced to go through review would (IMO) unnecessarily raise friction. -- Luca Beltrame GPG key ID: A29D259B signature.asc Description: This is a digitally signed message part.
Re: CI system maintainability
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. -- Kevin Ottens, http://ervin.ipsquad.net signature.asc Description: This is a digitally signed message part.
Re: CI system maintainability
On Thu, Mar 28, 2019 at 7:56 PM Konstantin Kharlamov wrote: > > > > On Чт, Mar 28, 2019 at 19:40, Ben Cooksley wrote: > > Hi all, > > > > We currently have a rather substantial issue, in that the CI system > > has been once again left in a position where it isn't possible to make > > any changes to the system. > > > > This means we can't update to newer versions of packages, add new > > packages or correct for binary incompatible changes which periodically > > get introduced to non-Frameworks. > > > > This issue has arisen because currently we have a recurring failure to > > build from source, within KDE PIM. Specifically, KContacts fails due > > to broken CMake logic. Despite this breakage having been in place for > > several days now, and the relevant mailing list being informed > > automatically by the CI system, the issue has not been corrected. > > > > While the most immediate fix is to correct this failure to build from > > source, that is only a short term fix and does not fix the underlying > > issue which makes the CI system difficult to maintain - and that is > > build failure reports being ignored, and people pushing broken code > > that doesn't even build. > > > > (For those wondering, the CI system uses OpenSUSE Tumbleweed, a > > rolling release distribution, for it's builds, so it isn't a case of > > old CMake or anything along those lines) > > > > We therefore need a long term fix for this. Note that pre-commit (as > > part of review) CI is not a solution in this instance, as the > > offending commits did not go through review. > > > > Does anyone have any ideas for a long term, proper fix to this? > > > > At this point given the amount of effort required to maintain a CI > > system vs. the amount of care actually being given by some developers > > (who are ignoring it's failure emails) it becomes questionable whether > > the effort is worth the return (and if not, we should just shut it > > down) > > > > Regards, > > Ben Cooksley > > KDE Sysadmin > > I don't know abut the current CI, but judging by recent discussion that > is about KDE migrating to gitlab; quick search shows gitlab does allow > prohibiting a merge if CI failed¹ > > Note however, in my experience of contributing to diffrent project CI > often fails for reasons absolutely irrelevant to code being tested > (e.g. errors in a CI script), in this case prohibiting a merge may > worsen the situation. 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. > > 1: > https://docs.gitlab.com/ee/user/project/merge_requests/merge_when_pipeline_succeeds.html#only-allow-merge-requests-to-be-merged-if-the-pipeline-succeeds > > Cheers, Ben
