95970 - Need a "Bugzilla Hacking Guide"

Status: RESOLVED FIXED

(Bugzilla :: Documentation, defect, P1)

Description

[Dave Miller [:justdave]]

We need to create a "Reviewers Guide" for Bugzilla which contains preferred
coding practices and also common gotchas that that reviewers often overlook
because they're not something you'd normally think about.

The situation this last weekend at bugzilla.mozilla.org is a real good example
of a tragedy that could have been prevented had a common perl gotcha been
noticed by either of the two people who reviewed a certain patch.  (See bug
95890 if you want all the gory details - short story is b.m.o is having to pull
a backup and reconstruct a good portion of the bugs_activity table)

To start off the common gotchas section:
1) Any variable included inside a regular expression must be quoted with \Q..\E
unless the variable is intended to contain a regular expression.
2) avoid using regexps for grep if you're looking for an exact match, instead
use a code block to compare equivalency ( grep($_ eq $matchstr, @list); )

These two are what bit us in the butt on b.m.o the other day.I'm sure we can
come up with more, too.

In this guide, we should also come up with some standard testing procedures for
testing new code before it's checked into cvs.  We have landfill at our
disposal, but it doesn't seem to be enough.  Is there stuff we can add to
landfill to assist testing?  Or certain tests we need to remember to perform? 
Maybe we should make a requirement in the reviewer guide that a list of things
that need to be tested to validate a given patch should be included in the bug
report before the patch is tested and checked in...  A checklist of things that
patch stands a remote chance of affecting, etc....
thoughts anyone?

Comment 1

[Zach Lipton [:zach]]

I'm working on an automatic testing engine (which I really need some 
help with) which should help catch some of these things. Basically, the 
idea is to use the frontend code to make changes to bugs and create 
bugs, and then manually read the same info from the db and shadow db 
to see that what we intended is put in to place. I would then run the output 
script (show_bug.cgi, etc) and parse that to see that it outputs the data I 
am expecting. In addition, once the framework is up, we make the rule 
that if you fix a bug, you write a test to ensure that it doesn't come back to 
haunt you. Simple as that. No regressions.

Of course, this may take a while.

Comment 2

[Jacob Steenhagen]

A working draft of a Style/Reviewer's guide can be seen at
http://landfill.tequilarista.org/REVIEWERGUIDE.txt

This guide contains some information listed here as well as stuff talked about
in IRC.  Please let me know of any addional content that should be added (as
well as spelling/grammer/fact errors).

Comment 3

[Matthew Tuck [:CodeMachine]]

I think Tinderbox is the place for the /$value/ and bad character in filename
checks.  We should make attempt to make it check for them and should try to get
everyone to check the Tinderbox tests before attaching or reviewing a patch (and
if that isn't possible now, make it so).

I don't think it's appropriate for reviewers to be worrying about this sort of
stuff explicitly because if we go down that road we're going to end up with ten
million things that get in the way of the real review.  This stuff should only
be included if it's impossible for Tinderbox to do the checks completely.

Furthermore, we should be being proactive rather than reactive and searching out
instances of bad Perl before they happen.  Surely there's somewhere that has
this sort of stuff documented ...

Comment 4

[Zach Lipton [:zach]]

The testsuite that I am cooking up will do the tinderbox tests (or more
specifically, the tinderboxen will run the test suite). I have approval to land
the testsuite code framework after 2.14 ships. In the mean time, we should be
coming up with things to test.

Comment 5

[Gervase Markham [:gerv]]

Surely we need a Bugzilla Hacker's Guide, not a Reviewer's Guide - after all, if
the original patch author had known about this stuff, the reviewer wouldn't need
to catch it, if you see what I mean.

Gerv

Comment 6

[Gervase Markham [:gerv]]

Comments on the text:

Bugzilla has never before had a mandated braces standard. If you want to mandate
one, that's fine - but duplicates.cgi, for one, will need totally reformatting.

Gerv

Comment 7

[Jacob Steenhagen]

Matty: I can see your point about being too much to remember, esp. in the
future.  Currently the tinderbox scripts are only set up to run on checked out
versions of the code (that are automatically updated before being check, at
least AFAIK).  I'm not sure who much work it would be to make them (or Zach's
test suit) work on "aribtrary" versions (and with out e-mail the tinderbox server).

Gerv: I guess this really is more of a hacker's doc.  I only called it
REVIEWERGUIDE 'cause that's what the summary of the bug says :)

Also, Gerv: That's what the disclaimer is all about:

...not all of the code currently in Bugzilla adheres to this styleguide...

It also says in there (or at least it should) that there is no concetrated
effort to make the code fit the styles specified, but any new work of
signifigance (fe, more then just a few lines here and there) should fit the "new
style" (note that the new style is basically what the majority of the code uses).

Comment 8

[Zach Lipton [:zach]]

Jake: this is the exact point of my test suit. You put it on, tie on the tie and 
the tie invokes Test::Suit to see that the suit functions properly. 

Oops, wrong project. The test suite consists of a t/ directory where the 
tests are (with a .t extention) and a script in the bugzilla/ directory called 
runtests.sh. When you run ./runtests.sh it triggers all the tests in the t/ 
directory using Test::Harness (included with perl) and Test::More (part of 
the latest devel. version of perl and availble as part of the Test::Simple 
package on CPAN for older versions, it is already installed on landfill, 
though it needs to be upgraded to the latest version). Basically, we can 
make it a precheckin requirement to run the suite, and to add any 
additional tests that are relevent to what you just did. This will ensure 
some level of basic quality. 

Comment 9

[Dave Miller [:justdave]]

I suppose it could be conceivable that these testws could be invoked by
tinderclient or even from sanitycheck as well as from the command line?

Comment 10

[Jacob Steenhagen]

Zach: That's a nice feature... might save the embarasment of showing up at the
party w/an outdated suit ... of course, you'll have to keep your perl modules up
to date...
See why I want people to look at the REVIEWERGUIDE.txt for spelling and/or
grammer errors :)

OK, that sound good.  We'll, of course, have to publish those extra
dependencies.  It'd also be a good idea to document exactly what the test suite
tests for to put in a "simplified" hackers guide.

Comment 11

[Jacob Steenhagen]

OK, it looks like the consensous for this to to go a HACKERSGUIDE instead of a
REVIEWERGUIDE, so I've taken my STYLEGUIDE and revised it once again to start
the process of making it into a HACKERSGUIDE.  Of course, the difference between
the two is a name and some explination of common bugzilla subroutines (IMHO, at
least).  Take a look at the URL below and let me know if this is the type of
thing you're looking for.  To the best of my knowledge, nothing like this exists
in Barnboy's Bugzilla guide.

http://landfill.tequilarista.org/HACKERSGUIDE.txt

Comment 12

[Jacob Steenhagen]

As I look this over, I relize that it won't take long for this document to
become unmanagable in its current format.  When it was a simple STYLEGUIDE, the
plaintext format worked fine.  Even the REVIERGUIDE wasn't terribly bad as the
only addition it really had was the grep problem, but I think what it's becoming
would be better done in SGML (docbook).  At the time of this comment, I don't
really know docbook nor do I have an environment where I can compile it, but I
suppose I should look into solving both those problems.

Comment 13

[Matthew Barnson]

*cheer*Docbook is actually pretty easy.  However, getting your environment right can be a pain sometimes.  If you follow some of the coding guidelines set forth in the CVS copy of the Bugzilla Guide's SGML, it helps a bit.  I highly recommend PSGML mode in emacs for editing SGML... it automatically inserts required fields, and comments for those fields which aren't required.  Pressing C-c C-e brings up a context-sensitive menu of available tags.  Reading through the SGML of the Guide, while not nearly perfect, provides a decent starting place for understanding how entities, DTD's, and such work.If you run Linux-Mandrake. I can provide you with a list of the RPM's you should install and environment variables you should set to make DocBook XML work for you.  Everything needed for Docbook development is included on the installation CD's.  This is one reason why I like Mandrake : )

Comment 14

[Jacob Steenhagen]

Matt: I'm running RedHat, which in theory should be similar (it at least uses
.rpm files).  If you could provide my with that list, that'd be great.  I had it
working once in my install, but I reformated my hard drive and don't remember
what I did :(

Comment 15

[Matthew Barnson]

Hmm... guess I should put this into the Guide sometime as well.
This is just off the top of my head, but here goes.  Note some of these may 
NOT be necessary, but I don't think they hurt anything by being installed.
rpms:
openjade
jadetex
docbook-dtd41-sgml
docbook-style-dsssl
docbook-dtd31-sgml
docbook-style-dsssl-doc
xemacs
psgml
sgml-tools
sgml-common

Set up environment:

in your .bashrc add this line (after installing above RPMS):
export SGML_CATALOG_FILES=/etc/sgml/catalog

xemacs on Mandrake has psgml by default.  I will attach my .emacs file.  Put 
this .emacs into your home directory, and you'll have a fairly functional 
psgml setup.  Probably my favorite function from my .emacs is 
"sgml-fill-and-save".  Just hit F2 within an element, and emacs will justify 
it for you.  It has a little problem right now with ulink tags and xrefs with 
a period immediately following the end of the xref.  I'll have to look into 
that.

Download "ldp.dsl" from the Resources page on linuxdoc.org.  This is the 
stylesheet I use to get the HTML and text output.  It works well, and has a 
nice, consistent look with the rest of the linuxdoc documents.  You'll have to 
adjust the paths in ldp.dsl at the top of the file to reflect the actual 
locations of your docbook catalog files.  I created a directory, 
/usr/share/sgml/docbook/ldp, and put the ldp.dsl file there.  I then edited 
ldp.dsl and changed two lines near the top:
<!ENTITY docbook.dsl SYSTEM "../dsssl-stylesheets-1.62/html/docbook.dsl" CDATA 
dsssl>
...and...
<!ENTITY docbook.dsl SYSTEM "../dsssl-stylesheets-1.62/print/docbook.dsl" CDATA
dsssl>

  Note the difference is the top one points to the HTML docbook stylesheet, 
and the next one points to the PRINT docbook stylesheet.

  You know, this sure looks awful involved.  Anyway, once you have this in 
place, add to your .bashrc:
export LDP_HOME=/usr/share/sgml/docbook/ldp

  There are some compilation instructions in README.docs in the docs/ folder 
of the Bugzilla distribution that describe how to compile stuff.  Perhaps 
these instructions should go into readme.docs as well...

Comment 16

[Matthew Barnson]

Attachment: My .emacs file

Comment 17

[Jacob Steenhagen]

Yippie! I can compile docbook now :)  I only have one minor problem to solve
before all the world is right.  RedHat 7.1 came with all the proper RPMs
installed (at least it seems to be the case with the install options I used,
whatever they were).  It seems all I was really missing was the ldp.dsl and then
environment variables.  The only problem is it installs "Modular DocBook HTML
Stylesheet Version 1.59" instead of 1.61.  So after doing:

cd /var/www/bugzilla/docs/html
rm *
jade -t sgml -i html -d $LDP_HOME/ldp.dsl\#html ../sgml/Bugzilla-Guide.sgml
cvs diff

I get that one line difference in every file :)  So as soon as I figure out how
to upgrade the Style Sheet, I think I'm good to go.

Comment 18

[Matthew Barnson]

Jake:  I'd like to include the Reviewer Guide in the Bugzilla Guide.  Do I have
your permission to include the ReviewerGuide/Bugzilla Hacking Guide info into
The Bugzilla Guide, and mention you in the credits?

Comment 19

[Matthew Barnson]

I think Red Hat has some allegory to Mandrake's "cooker" that you could download
newer RPM's from.  Or you could simply download the dsssl from linuxdoc.org's
"Resources" link, extract it to /usr/share/sgml/docbook/dsssl-1.61, symlink
dsssl-1.61 to dsssl (if you want) and be in business.

Comment 20

[Matthew Barnson]

Changed Summary to be more in-line with what we need.

Comment 21

[Jacob Steenhagen]

Barnboy: That'd be great.  Of course, the hacking guide portion (describing
common subroutines) is a little lacking, but more can always be added in the future.

I'll try updating my dsssl when I get a chance (I've got a down mailserver, ATM)

Comment 22

[Matthew Barnson]

I just tacked the straight text in with a <literallayout> tag.  That tag is
wonderful when I don't have time to format something correctly in time for
release -- the info is there, but looks kind of weird when compared to the rest
of the Guide.