Re: gEDA-user: rant: pcb print from command line

[Peter Clifton <pcjc2@xxxxxxxxx>]

On Mon, 2010-03-08 at 17:40 +0000, Kai-Martin Knaak wrote:

> <rant=on> 
> I filed a bug report, I provided a patch on this list, I nagged twice on 
> on this about it. How come, the patch is still not applied to the source? 
> What should I do to get this annoying bug fixed?
> Yes, I feel ignored by the developers. 
> No, I am not happy about it. 
> </rant>

Some developers - like (sorry to say, I'm speaking purely for myself
here), are damned right lazy, and sometimes need a little cajoling to
get things done. I get a lot of email, and messages I've flagged as
"unread" or "important" to remind me about them often get buried. I have
587 unread messages in my geda-user mailbox currently. Mostly from a
period when I was away, but many are things I (at the time) thought I
would take a second look at.

Getting patches merged is sometimes about reminding people, and making
things _easy_ for them.. If I want someone to build test my repository,
or look at a patch - I send a link, and possibly even commands to
step-by-step check out the repository, for example.

I'm about 90% more likely to look at something if someone provides me a
click-able link, than says "it is sourceforge bug #12345", or just says
"my patch is in the bug tracker". Similarly, patches are more likely to
get applied if they are small, and come with a header specifying the
commit message and detailed reasoning about what the patch is for (and
expected changes in behaviour).


KMK, I've found the patch you're referring to in my email client, but it
makes me nervous to apply it directly. Touching hidnogui.c to remove the
CRASH; statement - why is that needed? The patch isn't from a local "git
commit", so doesn't have a log entry explaining what it does.

As a developer "applying" your patch, I would have to figure that out,
and write one (possibly incorrect), or I'd have to come back and query
you for it. By applying the patch, I'm effectively vouching for it, and
that is difficult to do without testing.

Since I don't know what the patch's intended changes are... testing is
hard! I have to reverse engineer what the patch is supposed to change,
and invent a test-case. (Remember that I'm lazy... hacking on my own pet
project stuff has a much lower entry level!)


Trawling email on this specific patch:

"
A patch produced by this simple approach is attached. It moves the processing 
of action scripts and strings a little up. So they are executed with export 
options too. The file hidnogui.c needs a little patch to, to not exit if 
nogui_invalidate_lr is hit.
"

The idea seems reasonable, but the patch is potentially dangerous to me,
since many things in PCB rely on the GUI being up to work. I need to see
some evidence of what testing has been performed to make sure things are
safe. (You might also include the detail that the patch doesn't change
any behaviour for PCB invocations which aren't passed an action script).

This is an example of the kind of side-effect you might expect to see
because the GUI is expected to be up.

"
HID error: pcb called GUI function nogui_invalidate_lr without having a
GUI available.

Grep couldn't didn't find this function anywhere else in the source. 
So I am a bit clueless where this error message is triggered.
As a workaround I simply commented the corresponding CRASH line 
in hidnogui.c
"

The "workaround" worries me. It might be a valid thing to do, but I want
to know why - and I've probably not got the time to figure it out
myself. Is this the _only_ side-effect of the patch? I know PCB's
startup code has been fragile before now, this is why I'm reluctant to
dive in and just apply patches changing code I'm not currently
intimately familiar with.

FWIW, gdb ought to show you what is happening when the CRASH call is
triggered, and if it doesn't help you identify the issue - taking it up
with a PCB developer might reveal something more.


Some of this failure to apply patches is down to the "I'm busy - or
unsure, perhaps someone knows better and will look at it..." syndrome.
I'm sure everyone things this, and nothing gets done.

If you want to approach someone directly (albiet this isn't the usual
suggestion of how to interact), you might email me, or one of the other
PCB developers, and ask to work through getting the patch committed on
IRC. Peter B and I often write patches and review each others work this
way.

We might be able to fix whatever is causing the GUI updates, or declare
that the "workaround" you proposed is in fact harmless. In any case, we
don't usually /* comment out */ code we've decided to remove. It leads
to a load of cruft over time.

At most, you might put,

/* NB: This function is a NOP. We don't call CRASH; as in other hidnogui
 *     functions, since this function can be called when exporting from
 *     a command line specified aciton script.
 */

Much more verbose, but very clear to anyone than just wondering why
/* CRASH; */ is commented out.


It might seem unfair that we're (or I am) trying to third party
contributions to a higher standard than some of PCB's existing legacy
code, but assuming it doesn't completely stifle contribution,
encouraging high standard commits should aid PCB's long term quality.


On the other hand, I am reluctant to bash down people's patch
contributions with continual bombardment to polish them endlessly.
Falling back to the "I'm busy" stance is lots less confrontational than
me batting the patch back to you and demanding revisions before I apply
it.


Perhaps we should just apply the patch as is, and catch any bugs (if)
they appear. I don't want to discourage future contributions.

Best wishes,

-- 
Peter Clifton

Electrical Engineering Division,
Engineering Department,
University of Cambridge,
9, JJ Thomson Avenue,
Cambridge
CB3 0FA

Tel: +44 (0)7729 980173 - (No signal in the lab!)
Tel: +44 (0)1223 748328 - (Shared lab phone, ask for me)



_______________________________________________
geda-user mailing list
geda-user@xxxxxxxxxxxxxx
http://www.seul.org/cgi-bin/mailman/listinfo/geda-user