How to deal with uncooperative developers
On Tue, 2014-03-25 at 23:59 +0000, J.B.C. Engelen (Johan) wrote:
Inkscape is open source
IANABM*
I always thought Inkscape was more like a cake sale than open source. ;-)
For fixing code, there should be some sort of timer which says that is code is in trunk via the process set up by the project, then it should be fixed if found defective by the original developer if found within 2 or maybe 6 months. Otherwise the code should be considered divorced from it's original writer.
As for policy regarding commit access; we don't do enough to encourage code review. Anything to encourage code review would be great, I don't even mind if I were downgraded to B level 'code must be reviewed before merge' status. Because really, it should be.
Martin,
* I am not a board member
On Tue, Mar 25, 2014 at 11:39:51PM -0400, Martin Owens wrote:
On Tue, 2014-03-25 at 23:59 +0000, J.B.C. Engelen (Johan) wrote:
Inkscape is open source
IANABM*
I always thought Inkscape was more like a cake sale than open source. ;-)
You'll have to explain that one.
For fixing code, there should be some sort of timer which says that is code is in trunk via the process set up by the project, then it should be fixed if found defective by the original developer if found within 2 or maybe 6 months. Otherwise the code should be considered divorced from it's original writer.
As for policy regarding commit access; we don't do enough to encourage code review. Anything to encourage code review would be great, I don't even mind if I were downgraded to B level 'code must be reviewed before merge' status. Because really, it should be.
I like this. I'll even put my hand up to do some code reviews. Can we just turn code reviews on with our current tooling?
njh
On Wed, 2014-03-26 at 14:56 +1100, Nathan Hurst wrote:
I always thought Inkscape was more like a cake sale than open source. ;-)
You'll have to explain that one.
Anyone can bring a cake to sell, but not everyone is any good at baking.
i.e. The inkscape code-base is more like wikipedia and less like the linux kernel or other traditional model of development at the moment.
Martin,
On Wed, 2014-03-26 at 14:56 +1100, Nathan Hurst wrote:
For fixing code, there should be some sort of timer which says that is code is in trunk via the process set up by the project, then it should be fixed if found defective by the original developer if found within 2 or maybe 6 months. Otherwise the code should be considered divorced from it's original writer.
As for policy regarding commit access; we don't do enough to encourage code review. Anything to encourage code review would be great, I don't even mind if I were downgraded to B level 'code must be reviewed before merge' status. Because really, it should be.
I like this. I'll even put my hand up to do some code reviews. Can we just turn code reviews on with our current tooling?
The tooling is there, Launchpad supports this easily, but I think the social aspect is more difficult than the tooling.
Personally I'd prefer to not have "A" or "B" level developers, instead say everyone has to get their code reviewed by someone else. I think that usually makes a significant dent in making someone accountable.
I'd also suggest that if we're making a change like this we move to a tool as being the only one with permission to land, at least at first. Otherwise people will just commit to trunk as they've always done. It seems like after a release would be a good time to make such a change.
Ted
On Wed, Mar 26, 2014 at 01:04:47PM -0500, Ted Gould wrote:
On Wed, 2014-03-26 at 14:56 +1100, Nathan Hurst wrote:
For fixing code, there should be some sort of timer which says that is code is in trunk via the process set up by the project, then it should be fixed if found defective by the original developer if found within 2 or maybe 6 months. Otherwise the code should be considered divorced from it's original writer.
As for policy regarding commit access; we don't do enough to encourage code review. Anything to encourage code review would be great, I don't even mind if I were downgraded to B level 'code must be reviewed before merge' status. Because really, it should be.
I like this. I'll even put my hand up to do some code reviews. Can we just turn code reviews on with our current tooling?
The tooling is there, Launchpad supports this easily, but I think the social aspect is more difficult than the tooling.
Personally I'd prefer to not have "A" or "B" level developers, instead say everyone has to get their code reviewed by someone else. I think that usually makes a significant dent in making someone accountable.
I agree, I should have deleted that part from my reply. I was only agreeing with the code review part.
I'd also suggest that if we're making a change like this we move to a tool as being the only one with permission to land, at least at first. Otherwise people will just commit to trunk as they've always done. It seems like after a release would be a good time to make such a change.
In some ways this is exactly what we have for initial committers - get two patches accepted. I think it would be a nice step to say that anyone can approve, but it must be approved.
njh
On Wed, Mar 26, 2014 at 01:04:47PM -0500, Ted Gould wrote:
I'd also suggest that if we're making a change like this we move to a tool as being the only one with permission to land, at least at first. Otherwise people will just commit to trunk as they've always done. It seems like after a release would be a good time to make such a change.
Does a tool like this exist for git? I know the Launchpad folks used one for bzr, though I forget the name.
For this to work, we would need to have a box somewhere running the bot to do the checking, right? Or does that run on Launchpad?
Bryce
On Wed, 2014-03-26 at 19:36 -0700, Bryce Harrington wrote:
On Wed, Mar 26, 2014 at 01:04:47PM -0500, Ted Gould wrote:
I'd also suggest that if we're making a change like this we move to a tool as being the only one with permission to land, at least at first. Otherwise people will just commit to trunk as they've always done. It seems like after a release would be a good time to make such a change.
Does a tool like this exist for git? I know the Launchpad folks used one for bzr, though I forget the name.
For this to work, we would need to have a box somewhere running the bot to do the checking, right? Or does that run on Launchpad?
I'm not as familiar with the git world, but I can't imagine there's not. Tarmac¹ is the one that's pretty widely used in the LP world. It does need a box to run on, though it's not very intensive.
Ted
Just a quick reply, will be away from home for a few days.
On 26-3-2014 4:56, Nathan Hurst wrote:
On Tue, Mar 25, 2014 at 11:39:51PM -0400, Martin Owens wrote:
As for policy regarding commit access; we don't do enough to encourage code review. Anything to encourage code review would be great, I don't even mind if I were downgraded to B level 'code must be reviewed before merge' status. Because really, it should be.
I like this. I'll even put my hand up to do some code reviews. Can we just turn code reviews on with our current tooling?
I think we lack the resources for code reviewing everything, at least in minute detail. Note that patches very often go unreviewed for a long time, and I don't think that is just because they go unnoticed by a potential reviewer. This is the reason I am pushing for "automated review", in the form of -Werror(*) and static analyzers, and possibly automated code pretty-printing by clang-format. This way, at least we can focus our review on more design-like choices, and not get bogged down in reasoning about hard-to-read and/or easily-bugged code.
I try to look at every commit, and I believe some others do too. But it becomes tiring when simple things are 'wrong' that would have been easily caught by e.g. -Werror.
regards, Johan
(*) please think with me. I mean -Werror with all the improvements as discussed on maillist, for example disabling some errors for certain files.
On Wed, 2014-03-26 at 23:15 +0100, Johan Engelen wrote:
I think we lack the resources for code reviewing everything, at least in minute detail. Note that patches very often go unreviewed for a long time, and I don't think that is just because they go unnoticed by a potential reviewer.
I don't think we lack the resources for code review. Even a simple code review doesn't take that much time really. We're not saying every branch needs to be compiled (automated checkers can take over there) but just staged in such a way that we can encourage to use of branches and merging and thus someone to look at the commit and say ok.
Having this as a part of our project culture rather than an optional extra will make it work. It's a self fulfilling prophecy, but the good verity.
Martin,
On 27-3-2014 1:03, Martin Owens wrote:
On Wed, 2014-03-26 at 23:15 +0100, Johan Engelen wrote:
I think we lack the resources for code reviewing everything, at least in minute detail. Note that patches very often go unreviewed for a long time, and I don't think that is just because they go unnoticed by a potential reviewer.
I don't think we lack the resources for code review. Even a simple code review doesn't take that much time really. We're not saying every branch needs to be compiled (automated checkers can take over there) but just staged in such a way that we can encourage to use of branches and merging and thus someone to look at the commit and say ok.
Having this as a part of our project culture rather than an optional extra will make it work. It's a self fulfilling prophecy, but the good verity.
I find that code review takes *a ton* of time.
I'm happy that you seem to be in favor of having automated checkers.
-Johan
On Sun, 2014-03-30 at 22:00 +0200, Johan Engelen wrote:
I find that code review takes *a ton* of time.
This is probably because your doing it properly :-) I have a much smaller set of red flags in C++.
I think we don't need to check everything, just setup your red flag list to a subset of things you're really good and keen on. If two or three parties run their particular bugbears over the code and are quick to report back with a low threshold then the developer can fail quicker and get things fixed quicker.
The evidence for getting high quality out of multiple low quality signals is clear to me. Get two or more people for each review but set the bar really low and set the speed really quick. That turn around speed is really important for keeping flow going.
Martin,
On Tue, Mar 25, 2014 at 11:59:26PM +0000, J.B.C. Engelen (Johan) wrote:
Hi all, I'd like your opinion on how to handle developers... My question is, how should I/we deal with such a person, without running away from the project in anger.
First off, be aware the board mailing list archives are publically visible. Thus I think discussions about specific individuals probably is not well suited for this list.
Second, this board's charter focuses on financial matters, not technical or interpersonal, so probably this is off topic here anyway.
However, on the second point, I've been noticing a number of technical questions come up that need decisions beyond what individual developers can do, and wondering if it would be worth establishing an "Inkscape Technical Board".
I don't think this board's charter gives it the power to unilaterally establish a technical board. So, I think to do this we'd need to have the Inkscape developer community vote to grant us this ability. Then we could run an election to fill such a board, and charge it with the duty to provide Inkscape with technical decisions raised to it.
The alternative would be for Josh or myself or one of the other "project leads" to take a more authoritative handle on the project to drive decisions like these. But I've never been comfortable doing that, and frankly don't have the stamina (or time!) to fight such fights.
Either in addition or instead of a tech board, we could establish a membership board. This would be chartered to judge and address interpersonal problems, provide/withdraw commit rights, and other decisions regarding who can do what in the project. They'd not be chartered to handle any technical or financial decisions. Again, the Inkscape Board would need to be explicitly granted the power to establish such a board, via a general election.
I'm tired of confessing to people that, yes, indeed, Inkscape is open source and "hence" all the crashes that you experience.
I like Martin's Bake Sale analogy, and it fits well. It was indeed our in Inkscape's original founding principles that the bar of entry be kept very low. Many of us had been excluded from contributing to Sodipodi for non-transparent reasons and so were sensitive to anything that could inhibit anyone from contributing. Thus we adopted permissive, wikipedia-style inclusiveness which stands in contrast to most other successful FOSS projects.
There are pros and cons to this development model. The cons might be more immediately visible than the pros! Some cons include the stress and effort of building consensus among all developers, rather than just having an authority make a ruling; cliques; variability in code quality; reimplementation churn; difficult to prevent people reinventing things for whatever random reasons. Some pros of this model are that it is more resilient to core developer burnout (since it's easy to pull in new developers with all rights and privs), empowers creatively unique contributions (since developers don't have to get pre-approval or go before a (potentially scary) review panel or whatnot before getting stuff in trunk), and can achieve a higher development velocity since contributions aren't gated by a review layer.
There are many alternative models in between these. For instance, allow anyone to commit changes, but only if it has received a Reviewed-by or a Tested-by at least one other person. And permit anyone to revert code that doesn't have one or the other. Or allow everyone to commit to a Working branch, but then cherrypick changes from that into the Official tree, once evidence proves them ready to go. etc.
Particularly, how can we enforce coding standards, coding style, etc.?
I certainly agree with the others that this looks like a job for more tools. Are there any code style checker tools that we've used on the Inkscape codebase before? If not, might be time to dig one up.
Related to this, how strict do we want to be on inclusion of library code copies in our codebase, that have but a single maintainer or no maintainer at all?
I am personally not a fan of having any library codes included in our codebase. I understand why we do, and would not advocate getting rid of them. But I believe packaging exists for a reason, and if done well would remove the need to pile external codebases in.
I personally believe any codebase with no maintainer should be treated as a contribution to the Inkscape codebase, and be required to conform to all of our stylistic, test, and performance requirements.
Bryce
On 27-3-2014 2:24, Bryce Harrington wrote:
On Tue, Mar 25, 2014 at 11:59:26PM +0000, J.B.C. Engelen (Johan) wrote:
Hi all, I'd like your opinion on how to handle developers... My question is, how should I/we deal with such a person, without running away from the project in anger.
First off, be aware the board mailing list archives are publically visible. Thus I think discussions about specific individuals probably is not well suited for this list.
I know. My mail was motivated by a specific individual, but the questions were not just about that specific individual.
Second, this board's charter focuses on financial matters, not technical or interpersonal, so probably this is off topic here anyway.
However, on the second point, I've been noticing a number of technical questions come up that need decisions beyond what individual developers can do, and wondering if it would be worth establishing an "Inkscape Technical Board".
I don't think this board's charter gives it the power to unilaterally establish a technical board. So, I think to do this we'd need to have the Inkscape developer community vote to grant us this ability. Then we could run an election to fill such a board, and charge it with the duty to provide Inkscape with technical decisions raised to it.
I'm not sure if we need another board for this. Maybe we do. I do think that indeed we need a group of people with executive power for technical issues.
The alternative would be for Josh or myself or one of the other "project leads" to take a more authoritative handle on the project to drive decisions like these. But I've never been comfortable doing that, and frankly don't have the stamina (or time!) to fight such fights.
Either in addition or instead of a tech board, we could establish a membership board. This would be chartered to judge and address interpersonal problems, provide/withdraw commit rights, and other decisions regarding who can do what in the project. They'd not be chartered to handle any technical or financial decisions. Again, the Inkscape Board would need to be explicitly granted the power to establish such a board, via a general election.
I don't think we need this.
I'm tired of confessing to people that, yes, indeed, Inkscape is open source and "hence" all the crashes that you experience.
I like Martin's Bake Sale analogy, and it fits well. It was indeed our in Inkscape's original founding principles that the bar of entry be kept very low. Many of us had been excluded from contributing to Sodipodi for non-transparent reasons and so were sensitive to anything that could inhibit anyone from contributing. Thus we adopted permissive, wikipedia-style inclusiveness which stands in contrast to most other successful FOSS projects.
There are pros and cons to this development model. The cons might be more immediately visible than the pros! Some cons include the stress and effort of building consensus among all developers, rather than just having an authority make a ruling; cliques; variability in code quality; reimplementation churn; difficult to prevent people reinventing things for whatever random reasons. Some pros of this model are that it is more resilient to core developer burnout (since it's easy to pull in new developers with all rights and privs), empowers creatively unique contributions (since developers don't have to get pre-approval or go before a (potentially scary) review panel or whatnot before getting stuff in trunk), and can achieve a higher development velocity since contributions aren't gated by a review layer.
I like the open development model of Inkscape, I am not in favor of changing it dramatically.
Particularly, how can we enforce coding standards, coding style, etc.?
I certainly agree with the others that this looks like a job for more tools. Are there any code style checker tools that we've used on the Inkscape codebase before? If not, might be time to dig one up.
As I wrote earlier, good options: -Werror, clang static analyzer, clang-format. These are tests that many people can run, and do not a scary review panel. They provide a very simple objective measure of what is not accepted. (we would have to specify e.g. compiler version, but those are details that are easily worked out)
Related to this, how strict do we want to be on inclusion of library code copies in our codebase, that have but a single maintainer or no maintainer at all?
I am personally not a fan of having any library codes included in our codebase. I understand why we do, and would not advocate getting rid of them. But I believe packaging exists for a reason, and if done well would remove the need to pile external codebases in.
I personally believe any codebase with no maintainer should be treated as a contribution to the Inkscape codebase, and be required to conform to all of our stylistic, test, and performance requirements.
What if there /is/ a maintainer? The immediate problem is that all those libs trigger a lot of bug warnings (many of which *are* bugs), that cloud our own bugs. A reworked build system that allows excluding those libs for compilers warnings / static analysis would help a lot. Also, I think we should move all these libs to a directory called "external" or something, so that it is clear no-one should make any change there without committing it upstream.
-Johan
On Sun, Mar 30, 2014 at 09:22:24PM +0200, Johan Engelen wrote:
On 27-3-2014 2:24, Bryce Harrington wrote:
Related to this, how strict do we want to be on inclusion of library code copies in our codebase, that have but a single maintainer or no maintainer at all?
I am personally not a fan of having any library codes included in our codebase. I understand why we do, and would not advocate getting rid of them. But I believe packaging exists for a reason, and if done well would remove the need to pile external codebases in.
I personally believe any codebase with no maintainer should be treated as a contribution to the Inkscape codebase, and be required to conform to all of our stylistic, test, and performance requirements.
What if there /is/ a maintainer? The immediate problem is that all those libs trigger a lot of bug warnings (many of which *are* bugs), that cloud our own bugs. A reworked build system that allows excluding those libs for compilers warnings / static analysis would help a lot. Also, I think we should move all these libs to a directory called "external" or something, so that it is clear no-one should make any change there without committing it upstream.
Seems reasonable to me. I do definitely believe that if there's an active maintainer, it is good FOSS citenzenship to be sending patches to fix issues we discover in their code.
Bryce
On Sun, 2014-03-30 at 19:01 -0700, Bryce Harrington wrote:
Seems reasonable to me. I do definitely believe that if there's an active maintainer, it is good FOSS citenzenship to be sending patches to fix issues we discover in their code.
It would be nice to automate it. If a script sees a change in any of a set of directories, then pass the diff and the bug report mentioned in the commit (and/or --fixes) to the right place via a dictionary.
It's probably too involved, given bug-tracker accounts and such. But a man can dream.
Martin,
participants (6)
-
Bryce Harrington -
J.B.C. Engelen (Johan) -
Johan Engelen -
Martin Owens -
Nathan Hurst -
Ted Gould