Re: pkglint before committing

[Jonathan Perkin <jperkin%joyent.com@localhost>]

* On 2020-03-24 at 20:43 GMT, Frédéric Fauberteau wrote:

> I don't commit very often and I forget something almost every time.
> Would it be possible to have a pre-commit hook script that asks if
> we have executed pkglint before committing (y/n)?

For me this is the wrong way to approach the problem.  As a developer,
you have a responsibility to other developers and the thousands of
people that depend upon our work to ensure that any change you make
has been well thought-out, tested, reviewed where appropriate, and is
the best work you can produce, minimising any possibility that someone
will be negatively affected by your change.

It's important to make a distinction between this responsibility and
any infrastructure we have in place to make that work easier.  As most
people know I'm a big believer in the latter, and have a setup that
allows me to test even trivial changes in full bulk builds across
multiple OS with extra checks to catch most problems.

However, there still needs to be that human factor that kicks in
whenever I type "cvs commit" that should stop me in my tracks,
increase the heart rate, and make me pause to think "did I do
absolutely everything I can to ensure this isn't going to break
someone?"

Our focus should be on ensuring that is what we're aiming for, that we
strive for excellence in our work.  The problem with adding things
like a cvs hook to say "did you run pkglint?" is it does the opposite.
It adds complacency and an expectation that the magic infrastructure
will cover any mistakes we make.

Running pkglint should be an absolute minimum requirement for any
commit.  Here's a list of other things that should ideally be done off
the top of my head:

 * If possible, test on more than 1 OS, especially for anything that
   has per-OS PLIST etc.

 * Test the build in a clean sandbox to ensure all dependencies have
   been correctly accounted for (don't rely on installed packages for
   USE_TOOLS, for example).

 * Manual review of the entire 'cvs diff'.

 * Ensure SUBST patterns haven't been expanded in patches.

 * If adding packages or changing dependencies, run a pbulk scan to
   ensure you haven't broken dependency resolution across the tree.

 * Enable extra PKG_DEVELOPER checks, e.g. CHECK_SHLIBS_BLACKLIST and
   CHECK_WRKREF across more directories, if appropriate for some OS.

 * If updating a non-leaf package, rebuild all its dependents to
   ensure no ABI/API fallout.

 * Check for any revbump requirements.

 * Test all possible PKG_OPTIONS combinations.

There are no doubt more, depending on the type of change.  My point is
that these are the things that everyone should at least be aware of
and thinking actively about every time they "cvs commit".

If you commit rarely and find it difficult to context switch back,
then perhaps just write yourself a text file with this list of things
that you can use as a checklist?  Or write a cvsci() shell function
that asks the questions before running "cvs commit"?

Having checklists and reminders are good, but putting them in a
pre-commit hook is bad for a few reasons other than those I already
listed.

Firstly, it makes it someone else's problem rather than something you
should be taking personal responsibility for.

Secondly it puts an artificial limit on the number of things you
should be thinking about.  Just because something is pkglint-clean or
happens to pass some arbitrary checklist we come up with doesn't mean
it's necessarily a good change.  As above, different changes come with
different approaches for how you should be testing and reviewing them.

And finally, it adds extra annoyance and burden on people who are
already good at ensuring their changes are done to a high standard or
have their own checklists and procedures.

I hope that's helpful rather than coming across as a rant, it isn't
meant to, but I do acknowledge that among our group of developers
there are different opinions on our responsibilities, and these are
just mine.

Cheers,

-- 
Jonathan Perkin  -  Joyent, Inc.  -  www.joyent.com