Re: Changes to our Git infrastructure
On Monday, 5 January 2015 20:57:47 CEST, Frank Reininghaus wrote: Ultimately, a constant stream of newcomers is the only thing that keeps a free software project alive in the long term. Yes, as long as these newcomers eventually get enough interest and enough skills to become maintainers. I agree with the importance of getting new blood in, but there's also a need to educate these contributors so that their skills become better over time. I'm a bit worried that any new patch review system which requires more effort before one can submit a patch for review might put some potential new contributors off. That's a valid point, so yeah, I agree that we should evaluate our tool(s) in light of being usable by newbies as well as by professionals. Thanks for saying that. Cheers, Jan -- Trojitá, a fast Qt IMAP e-mail client -- http://trojita.flaska.net/
Re: Changes to our Git infrastructure
On Tuesday, 6 January 2015 07:40:01 CEST, Ian Wadham wrote: a) I do not know anything about Dr K, but I will try and find someone who does. b) Unfortunately there is nobody available any more who knows anything about Dr K, but I (or another suggested guy) will try to help. How about we take this offline via email or IRC and then you can walk us through the problem you are trying to fix, its significance and impact and how you are going about fixing it… This has a risk of splitting the discussion about a patch into multiple independent streams where people will have hard time keeping track of all the results, IMHO. The polishing (fixing nitpicks, etc.) should come *after* the stone is cut. That's a good suggestion. Going straight to that mode is inappropriate because it conveys the message, The problem you are trying to fix is unimportant to us. Would it work for you if there was a bot which pointed out these issues to the patch author? That way it would be obvious which part of the review is random nitpicking which is indeed not important when one considers the general direction of the patch, and in addition it would come from a machine so there won't be any bad feelings about people not appreciating the contributor's work. No amount of new technology, neither Gerritt nor the energy of cats confined in a bag, can help. There are management solutions to technical problems, but there are no technical solutions to management problems, as a colleague of mine used to say. Agreed. So the actual problem is lack of skilled maintainers who just aren't around. I agree that tooling cannot fix it -- the tooling can only help by a bit by making their job easier. If the maintainer is simply not here, then you cannot get a proper review, sure. This is an interesting discussion, and I think that there is no problem for it happening in parallel to the current talk about reshaping the git infrastructure -- but maybe in another ML thread. but the problem is that there's completely unmaintained code where nobody feels qualified to sign off patches. Exactly. And there are simple, technology-free solutions to that problem, if anybody is interested. What are these solutions? a) There is no encouragement for the reviewer to build and TEST the patch, independently of the reviewee. My personal opinion is that people should not be approving patches unless they tested them, or unless they have sufficient reason to believe that the change is OK (such as a trivial change with an associated unit test pre-approved by a CI run). How can we motivate people to test these changes without discouraging them from doing reviews at all? c) Patching encourages incremental, evolutionary development, rather than standing back and taking a design view of the way things are developing. BTW, ReviewBoard supports post-commit reviews of several patches or commits together, but that feature is turned off in the KDE usage [1]. So we need another approach (or tool) to help us perform review of the architecture of our software. Got some suggestions? Launchpad blueprints? Wiki pages? Cheers, Jan -- Trojitá, a fast Qt IMAP e-mail client -- http://trojita.flaska.net/
Re: Changes to our Git infrastructure
On Monday, 5 January 2015 22:22:19 CEST, Boudewijn Rempt wrote: Usually, half-way through they ask me, why doesn't KDE use github I do not understand how stuff would change if we used GitHub, though. There would still be that huge gap of not understanding which of the repos to use. I think that this is easy to solve with individual apps, but it's a hard problem when it comes to a platform, or indeed to an environment where the border between the individual pieces is unclear (e.g. which repo contains the plasma clock applet in KDE4, and where is it found in plasma5)? I do not have an answer on how to make this more obvious or beginner friendly. The contributors still have to know what a DVCS is, and have to understand the concept of a commit and what a push means. But all of that applies to GitHub as well. About the only difference that I can see are GitHub's pull requests. I understand that the knowledge of how to create a PR there is pretty widespread among our contributors. The fact that ReviewBoard doesn't allow actual approval (i.e. where it's approved *and* merged into our SCM) doesn't help here at all, that's also true. I would encourage you to read https://techbase.kde.org/Development/Gerrit#Getting_Started . Do you think that the workflow proposed there is reasonably beginner-friendly? We can then discuss ways in which it can be simplified. E.g. the need to set up an extra remote can go away easily *if* we agree on directing pushes to Gerrit once (and if) it's adopted. A website listing all projects can easily show a copy-pasteable link on how to clone and get the change-id hook in place during the initial clone, etc. We can add some scripting to sync SSH keys from LDAP to Gerrit, so that people only have to register with Identity and they'll be all set. That all is possible. What I'm saying here is that I believe that the feature set supported by Gerrit is actually very close to that of GitHub. It's different because there's no fork me button and the concepts do not map 1:1 to each other, but the general ideas are very similar. Cheers, Jan -- Trojitá, a fast Qt IMAP e-mail client -- http://trojita.flaska.net/
Re: Changes to our Git infrastructure
On Monday, 5 January 2015 14:03:13 CEST, Thomas Friedrichsmeier wrote: I think there is an easy test for this (well, not a real test, but a useful initial heuristic): Can you explain exactly how to submit a patch for your project - to someone without prior knowledge of the tools involved - without assuming the required tools/keys/accounts are already set up - without any further reading - covering all required steps in sufficient detail in no more than 2000 words, or (if it's based on a web-wizard) in less than 20 minutes? This includes links to other pages which explain how to work with git, but I think that it does qualify: https://techbase.kde.org/Development/Gerrit#Getting_Started Does it match your requirements? That page started as an attempt to provide a documentation for existing KDE developers, so it does go into depth of how to manualy Cc reviewers. If we decide to use Gerrit, then I think it would make a lot of sense to intorduce a single-page Submitting Patches Quickstart which would just describe the absolute basics on one page, including a git primer. Cheers, Jan -- Trojitá, a fast Qt IMAP e-mail client -- http://trojita.flaska.net/
Re: Changes to our Git infrastructure
On Tue, 6 Jan 2015, Jan Kundrát wrote: On Monday, 5 January 2015 22:22:19 CEST, Boudewijn Rempt wrote: Usually, half-way through they ask me, why doesn't KDE use github I do not understand how stuff would change if we used GitHub, though. I'm just relaying what usually happens when I get a newcomer in #krita. I would encourage you to read https://techbase.kde.org/Development/Gerrit#Getting_Started . Do you think that the workflow proposed there is reasonably beginner-friendly? Well, I find the page not very penetrable, to the point that I don't get the proposed workflow, but that might just be me, or the prose, which certainly needs editing. But I'm the wrong person to ask in any case. What you should do is hang out on #kde-devel and the next time someone comes on the channel who says, Hello! I'm new here. I want to develop on KDE. What do I need to do?, grab him and ask them to go through this process, and make notes whenever they get confused. Boudewijn
Re: Changes to our Git infrastructure
Hello Jan, On 06/01/2015, at 10:48 PM, Jan Kundrát wrote: On Tuesday, 6 January 2015 07:40:01 CEST, Ian Wadham wrote: a) I do not know anything about Dr K, but I will try and find someone who does. b) Unfortunately there is nobody available any more who knows anything about Dr K, but I (or another suggested guy) will try to help. How about we take this offline via email or IRC and then you can walk us through the problem you are trying to fix, its significance and impact and how you are going about fixing it… This has a risk of splitting the discussion about a patch into multiple independent streams where people will have hard time keeping track of all the results, IMHO. No, the review would be suspended while a) or b) occurred. If b) occurs, the operative words are offline via email or IRC (or face-to-face if possible). When both parties have reached an understanding of the problem and the issues involved, the formal review can resume. The polishing (fixing nitpicks, etc.) should come *after* the stone is cut. That's a good suggestion. Thanks. Going straight to that mode is inappropriate because it conveys the message, The problem you are trying to fix is unimportant to us. Would it work for you if there was a bot which pointed out these issues to the patch author? That might be even worse, like joining a telephone queue… And what about Krazy? That way it would be obvious which part of the review is random nitpicking which is indeed not important when one considers the general direction of the patch, and in addition it would come from a machine so there won't be any bad feelings about people not appreciating the contributor's work. No amount of new technology, neither Gerritt nor the energy of cats confined in a bag, can help. There are management solutions to technical problems, but there are no technical solutions to management problems, as a colleague of mine used to say. Agreed. So the actual problem is lack of skilled maintainers who just aren't around. I agree that tooling cannot fix it -- the tooling can only help by a bit by making their job easier. If the maintainer is simply not here, then you cannot get a proper review, sure. This is an interesting discussion, and I think that there is no problem for it happening in parallel to the current talk about reshaping the git infrastructure -- but maybe in another ML thread. Thank you, Jan. I am really glad you find it interesting. but the problem is that there's completely unmaintained code where nobody feels qualified to sign off patches. Exactly. And there are simple, technology-free solutions to that problem, if anybody is interested. What are these solutions? A quote from DOT: https://dot.kde.org/2014/06/03/ian-wadham-venerable-kde-programmer Q. You've gotten the applications into good shape, and are ready to hand them off. What type of person would you like to see take over? What will they get out of working on these applications? A. I would like to see KDE set up a maintenance group and standards for maintainability of code. Programs that reach a reasonably good standard could then be maintained interchangeably by members of the group. The group could be continually changing. Nobody can stay interested in such work for long. Also the group and its stock of programs would be a good source of Junior Jobs and a place for newbies to start. It would need to have some experienced members, or ready access to such people, because some bugs are too hard for trainees to solve. This is not a new idea. It is roughly what has been happening everywhere I have worked since about 1967, when the burden of people quitting jobs and leaving behind unmaintainable, half-finished messes became intolerable for most organizations. Albert's Gardening group is a good start in this direction. Re maintainability: KDE Community code is usually not good in this regard, IMHO. As a result, I was not at all confident about my understanding of Dr Konqi and how to patch it. Luckily Bugzilla had quite good documentation, so *what* to do was fairly clear. Nevertheless, I completely missed the side-effects (re cookies) inherent in XML RPC and kio_http. Those side-effects are not even visible on my platform, only on Linux. a) There is no encouragement for the reviewer to build and TEST the patch, independently of the reviewee. My personal opinion is that people should not be approving patches unless they tested them, or unless they have sufficient reason to believe that the change is OK (such as a trivial change with an associated unit test pre-approved by a CI run). How can we motivate people to test these changes without discouraging them from doing reviews at all? Mainly by example and praise, I would say. Plus maybe a small checklist when clicking Ship It. c) Patching encourages incremental, evolutionary
Re: Plasma 5.2 bits for kdereview
Updates on this.. I plan to ask for Bluedevil and libbluedevil, libkscreen and kscreen, libmm-qt and ksshaskpass to be moved. I see some comments that the libraries may be used outside of Plasma but there's no problem with that happening, they don't quality for frameworks and they already get released with Plasma so it's just an update in projects.kde.org user-manager looks like it could do with KConfig::KEMailSettings ported and then Account Details done away with, I'll keep it in kdereview for now. kcm-touchpad needs the feature of the shortcut for ktouchpadenabler taken and ktouchpadenabler disabled, I'll keep it in kdereview for now. polkit-kde is already in and has now been merged so master is KF5. Jonathan
Re: Changes to our Git infrastructure
On Tue, 06 Jan 2015 13:19:45 +0100 Jan Kundrát j...@kde.org wrote: On Monday, 5 January 2015 14:03:13 CEST, Thomas Friedrichsmeier wrote: I think there is an easy test for this (well, not a real test, but a useful initial heuristic): Can you explain exactly how to submit a patch for your project - to someone without prior knowledge of the tools involved - without assuming the required tools/keys/accounts are already set up - without any further reading - covering all required steps in sufficient detail in no more than 2000 words, or (if it's based on a web-wizard) in less than 20 minutes? This includes links to other pages which explain how to work with git, but I think that it does qualify: https://techbase.kde.org/Development/Gerrit#Getting_Started Does it match your requirements? It's great to see there is a starting point, but no, that's not it, yet (and btw, I was a bit confused when calling 2.000 words the limit. Something on the order of 500 or 600 would be more like it. So the length of that page (the first few sections) looks ok to me, but it's still asking a lot of prior knowledge of a _first time_ contributor. Things I see missing: - What exact commands do you have to run to create and enable your SSH key? (And will these work cross-platform?) - What are the commands to configure name and email in git? - When and where do I do the step of adding the gerrit remote? (And just in case, include the initial git clone command) - What command(s) do I run to create a git branch? What is an example of a good branch name, what is bad branch name? - Explain the basic usage of git commit, and git commit --amend. Including some words on what is or is not a useful commit message. - Also explaining basic usage git diff and git status, of course, for verifying the commit is as desired. - Arguably, some words on undoing undesired changes (before / after staging / committing (/ pushing)) would be rather needful, too. - Might have to explain the Change-Id-thing some more (or does it really happen all automagically, esp. for follow-up commits)? - I am personally unclear on whether I'd need to follow any of the steps after Submitting Changes, in order to actually receive notifications of what happens to my request. Or how - Also, what is the easy / standard way to get the change in, once reviewed? This may sound like nitpicking, but well, all of these are things that you will have to know for your first review, and while many contributors will know some of this, none of this should be assumed, IMO. That page started as an attempt to provide a documentation for existing KDE developers, so it does go into depth of how to manualy Cc reviewers. If we decide to use Gerrit, then I think it would make a lot of sense to intorduce a single-page Submitting Patches Quickstart which would just describe the absolute basics on one page, including a git primer. Absolutely. My point is: Try writing that page, and you'll get an idea of what exactly you're asking a first timer to learn (in addition to the coding) before they can get their first positive feedback. And well, so I have followed you quite some way into discussing gerrit, in particular, here. But I'd like not to go any further on this, at this point. Let's wait for sysadmins to compile their list of candidates (and I'd be rather sure that gerrit will be among the candidate, at least). *Then* we can reasonably discuss the pros and cons of specific alternatives. Regards Thomas signature.asc Description: PGP signature
Re: Review Request 121879: Improve KCoreConfigSkeleton tests
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121879/#review73296 --- +1 from me. - Matthew Dawson On Jan. 6, 2015, 6:51 a.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121879/ --- (Updated Jan. 6, 2015, 6:51 a.m.) Review request for kdelibs, David Faure and Matthew Dawson. Repository: kdelibs Description --- Backport from tests from https://git.reviewboard.kde.org/r/121838/ We have not decided how to fix yet in KConfig but i think adding the test to kdelibs makes sense to make sure we don't regress by mistake in a future LTS release of kdelibs. Diffs - kdeui/tests/kconfigskeletontest.h 8167c62 kdeui/tests/kconfigskeletontest.cpp 2bac0e1 Diff: https://git.reviewboard.kde.org/r/121879/diff/ Testing --- Tests pass Thanks, Albert Astals Cid
Re: Review Request 120376: drKonqi Fix Bug 337742 - Unable to send report: error code 410 from Bugzilla
On sep. 26, 2014, 11:54 matin, Ian Wadham wrote: Hi Frédéric, As announced on KDE Core devel, in http://lists.kde.org/?l=kde-core-develm=141016488132293w=2 about 3 weeks ago, I also am working on Dr Konqi. I am about to publish a general patch, which is aimed at the present problem posed by the change to tokens in Bugzilla https://bugs.kde.org/show_bug.cgi?id=337742, but also intends to avoid such problems in future and to be forward-portable to KF5. My approach is to check the version number of the Bugzilla software and to switch to whichever security method is appropriate for that version: cookies, tokens or passwords-only. Of course, my patch will implement tokens for the time being, but the idea is to switch automatically to passwords-only when the time comes, without any new release of KDE software being necessary. See http://www.bugzilla.org/docs/tip/en/html/api/Bugzilla/WebService.html#LOGGING_IN in the documentation for Bugzilla 4.5.5 (the next version), as opposed to 4.4.5 (the current version). Bugzilla 4.5.5 allows tokens, but I believe they will be discontinued at some stage, though I cannot put my finger on where I have seen that discussion. This should avoid users having to experience further bugs in Dr Konqi's connection to bugs.kde.org. My patch is also intended to be extendable, so that Dr Konqi can be updated and re-released, ahead of time, if any further feature change is announced by Bugzilla and could adversely affect Dr Konqi. Frédéric Sheedy wrote: Great, good news! My patch was only a quick fix to what you are doing. Ian Wadham wrote: I did not mean that you should drop what you are doing and discard this review and patch completely... :-) Instead, we should work together and each be aware of what the other is doing. Please revive your patch and this review. From what I can tell, the patch (after review and shipping) will be good immediately and at least until the version after Bugzilla 4.5.x. Also, your patch has some improvements to the testing, which is important. And I think we need to get a fix into the closing versions of KDE 4 ASAP (next deadline Thursday, 9 October). My fixes will be more long-term and I am running short of days to work on them, due to other commitments, and anyway they may take a while to review... So please revive your review and patch, Frédéric. One immediate comment: I found that I could retrieve the token by using the tag token, but I could not use token in the args map. I had to use Bugzilla_token. I have no idea why that is. I am using an Apple OS X platform, but that should not make a difference to a web dialog. Ian Wadham wrote: Frédéric, please have a look at review https://git.reviewboard.kde.org/r/120431/ particularly the comments of the last 24 hours. Somebody is going to have to commit a patch for Dr Konqi before Albert Astals Cid starts tagging KDE 4.14.2 on Thursday night. It will be either your patch, my patch or a simplified version of my patch. If the consensus is to use your patch in KDE 4.14.2 for now, I would like to give it a test on Thursday (Australian time, UTC + 11 hours). I am otherwise engaged today (Wednesday). All being well, I could commit your patch, but do you have commit rights yourself? Frédéric Sheedy wrote: Hi Ian, I do have an account to commit the patch. Let me know of the consensus! Ian Wadham wrote: As you may have seen on https://git.reviewboard.kde.org/r/120431/ the consensus was in favour of a simplified patch, which I edited, tested and later committed on Thursday. It is regrettable that neither of our patches received a review from a KDE core developer who is familiar with the Dr Konqi code. Had that happened, things could have proceeded in a more orderly fashion and I am sure that your patch could have been shipped immediately, to fix bug 337742, and mine could have been refined and shipped within the KDE 4.14.3 or 14.12 timeframe. Frédéric, I think it is important that your fixes for the Dr Konqi test processes should go into KDE 4.14.3 or 14.12 and also into KF5. Thank you very much for your help. Frédéric Sheedy wrote: Hi Ian, thanks for the answer! As my fixes for Dr Konqui are not for the end users, should I commit it without a review? Ian Wadham wrote: If nobody objects, I would say yes, but I am no authority. However, it seems rather hard to get a review of changes to Dr Konqi and we are just talking about some small changes to file drkonqi/tests/bugzillalibtest/bugzillalibtest.cpp. Albert Astals Cid wrote: Are we speaking of commiting just the test changes? Or also the other changes? I understand the other changes are no longer needed? Ian Wadham wrote: @Albert: Yes. No. Yes.
Re: Re: Changes to our Git infrastructure
On Tuesday 06 January 2015 12:48:41 Jan Kundrát wrote:
On Tuesday, 6 January 2015 07:40:01 CEST, Ian Wadham wrote:
a) I do not know anything about Dr K, but I will try and
find someone who does.
b) Unfortunately there is nobody available any more who
knows anything about
Dr K, but I (or another suggested guy) will try to
help. How about we take this
offline via email or IRC and then you can walk us
through the problem you are
trying to fix, its significance and impact and how you
are going about fixing it…
This has a risk of splitting the discussion about a patch into multiple
independent streams where people will have hard time keeping track of all
the results, IMHO.
The polishing (fixing nitpicks, etc.) should come *after* the stone is
cut.
That's a good suggestion.
I have to disagree here. I'm not able to do a good functional review if the
change is full of unintended nitpick things. Each changed line triggers a
what is that change for? and if it's just a change like
- if (foo) {
+ if (foo)
+ {
then I have a hard time as a reviewer. For me as a reviewer it's important
that the change is free of those nitpick things before I start looking at
the functional things. It's also something I expect from a contributor to
spend the time to run astyle on the change and configure kate to remove
trailing white spaces before requiring my time.
For me it's difficult to recognize the true change of a patch if it's full
of unintended changes. It just makes everything more difficult. If I have to
review a change with such nitpick items I normally do a nitpick round first,
then wait for the next patch version to do the proper review. This is
optimizing my time as a reviewer. (just to get some feeling on my review
costs: since yesterday I received 20 new mails in my reviewboard mail folder).
Also I do not think that reviewboard makes nitpicking to easy - if it wouldn't
highlight the trailing whitespaces I would still point them out as I consider
these things as important.
Going straight to that mode is inappropriate because it conveys the
message, The problem you are trying to fix is unimportant to us.
Would it work for you if there was a bot which pointed out these issues to
the patch author? That way it would be obvious which part of the review is
random nitpicking which is indeed not important when one considers the
general direction of the patch, and in addition it would come from a
machine so there won't be any bad feelings about people not appreciating
the contributor's work.
I would certainly appreciate it. I don't like doing things a computer could
do. It might also make my life as a reviewer easier as I can see that the
computer already handled it.
Qt does that and if I hit such a problem the first thing I do is fixing those
items to upload a clean patch.
Cheers
Martin
signature.asc
Description: This is a digitally signed message part.
Re: Changes to our Git infrastructure
On Tue, 6 Jan 2015, Thiago Macieira wrote: Unfortunately, as long as the tool permits line-by-line commenting, you're going to get nitpicking. My experience is that people are linear and will start reading the patch, calling out what they see when they see it. They should instead look at the big picture first and that isn't easy. See http://sarah.thesharps.us/2014/09/01/the-gentle-art-of-patch-review/ Lovely article. I know some people would object that the awful indentation makes the patch impossible to read, but apart from that specious argument, it's great advice. I wonder if we can tweak a tool to only allow line-by-line comments after two high-level reviews have been written... Boudewijn
