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
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
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: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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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 kno
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, 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
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: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
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
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
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
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
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
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
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
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 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
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
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: KDE Developer Documentation Support update
Hi Juan Carlos, Le mercredi 27 mars 2019, 14:03:54 CET Juan Carlos Torres a écrit : > Greetings KDE Community! > > I'm Juan Carlos Torres (Jucato on IRC) and I've recently been hired as a > contractor to get the ball rolling on updating our developer documentation. > Over the years, our community has produced an incredible wealth of > knowledge that now needs our attention and love. While the job description > focuses on new contributors, improving the documentation can benefit even > KDE veterans. > > I made a cursory survey of what I considered to be four important areas: > > 1. https://kde.org/develop - where most will probably start their search > (if they didn't use Google) > 2. https://api.kde.org/frameworks/index.html - the building blocks of all > KDE software > 3. https://techbase.kde.org/Development/Tutorials - where most of our > tutorials still live I started porting some tutorials to the repos where they belongs as examples (for instance KWallet (https://phabricator.kde.org/D14955) and KMessageBox (https://phabricator.kde.org/D14957). In my opinion the tutorials should be all ported to their repos so that we are always sure they compile and are up-to-date. And the tutorial pages can thus be removed or archived. Another bonus is that the examples can be referred to in the apidox at api.kde.org by adding \snippet keywords... > 4. https://community.kde.org - various pages like > https://community.kde.org/Get_Involved/development, or project-specific > ones like https://community.kde.org/Plasma > > These present opportunities where we can make a significant impact just by > updating tutorials or ensuring the apidocs meet the library documentation > policy or having Project pages that contain information newcomers need to > quickly become part of the team. > > This is something anyone and everyone can be involved in and I'm looking > forward to making this journey with the community I've grown up with for 13 > years now. Over the next few days, I will be knocking on some developers' > and teams' doors to get your input and feedback on how we can work together > towards this common goal. > > Let's make KDE rock not just for its software and for its community but > also for having first-class developer documentation for new and old > developers alike! > > > Cheers Olivier
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
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: KDE Developer Documentation Support update
On Fri, Mar 29, 2019 at 3:11 AM Olivier Churlaud wrote: > I started porting some tutorials to the repos where they belongs as > examples (for instance KWallet (https://phabricator.kde.org/D14955) and > KMessageBox (https://phabricator.kde.org/D14957). That's awesome! Thanks for getting this started! > In my opinion the tutorials should be all ported to their repos so that we > are always sure they compile and are up-to-date. And the tutorial pages can > thus be removed or archived. > Another bonus is that the examples can be referred to in the apidox at > api.kde.org by adding \snippet keywords... > That definitely makes sense for the frameworks at least. It will be easier to track their status and history that way. Then it could just be a matter of linking to them in the wikis (if only it were possible to embed the examples without copying the content). Hopefully, we can come up with a similar solution for the other tutorials that are not framework/API-centric. -- Regards, Juan Carlos Torres Jucato
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. >
