ports/96379: please do not send openvpn 2.0.7 reminders!

[Matthias Andree]

I'm Cc: James Yonan as he can see how one of the substream distributions
works this way and perhaps draw his own conclusions for code changes. I
know from my own experience that the first stab at fixing a problem
isn't necessarily the best when other people try to audit one's code,
even if it's sufficient to fix the actual problem for good.

Jim,

please skip forward to "FOR JIM" below.

On Thu, 27 Apr 2006, Andrew Pantyukhin wrote:

> Chill out, man :-) This handful of men are the ones we
> should praise, not scorn. In fact, they invested their time
> to learn the basics of making/updating a port, they took
> the trouble to notice 2.0.7 is out, to try it on their boxes,
> make a patch and go through a not-so-transparent
> process of submitting a PR.

Did they? They stopped short of reading the changelog, removed a
gazillion of %%PORTDOCS%% tags that prior to their patch made
-DNOPORTDOCS fly and turned it ineffective with their patch,
did not notice etc/rc.d/openvpn[.sh] still got
installed and was also mentioned in USE_RC_SUBR which is the official
way to list rcNG scripts as per the porter's handbook:
<http://www.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/rc-scripts.html>

There's a German adage, literally translated:
well-meant does not imply well-done.

While I don't ask people to write a novel on why they change which line
in pkg-plist or wherever, a single line documenting why literally ALL
%%PORTDOCS%% gets removed should be in order - and I'm looking rather
closely at the patch/PR if someone with FreeBSD 7 has an issue with my
port.

> And what do they expect
> in return? Well, if they do a great job and the patch
> gets into the tree they'll get a heart-warming "Committed,
> thanks!" If not, if they make mistakes, at least we should
> try not to discourage them. They get no money, fame or
> glory. Should we really bash them for not being attentive
> enough?

I am thinking that the removal of %%PORTDOCS%% from pkg-plist followed a
certain intent that did not get stipulated in the PR or communicated to
me some other way. I frown upon such massive changes in my PRs without
reason.

Perhaps the relevant section of the porter's handbook,
http://www.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/dads-freedback.html
could ask that users give reasons for non-trivial changes when
submitting PRs for ports they do not maintain themselves.

FOR JIM:

> Back to 2.0.6-2.0.7. Below is a fragment of the difference
> between the versions. As far as I can tell without studying
> the rest of the code, this does not only fix windows versions.

Right. I should have liked the constant USE_64_BIT_COUNTERS define in
common.h rather than syshead.h better, since that would have made
auditing trivial -- but it got placed into syshead.h.

> Granted, *nix systems might have get away with a wrong
> typedef, but it doesn't mean the old code is safe.

If someone uses a compiler that doesn't understand %llu as "unsigned
long long" then indeed not. I only fear the newer IEEE Std 1003.1-2001
inttypes.h/stdint.h defines aren't sufficiently portable to older
systems that are still in wide use - and perhaps not to Windows anyways.
(I don't know, not being a Windows hacker.)

> Only the most important things go to changelogs. What we see here
> tells us that further investigation might be needed to know the effect
> of the mistake, even if USE_64_BIT_COUNTERS is defined by default.
> 
> It doesn't mean that you're wrong about holding the horses and staying
> with 2.0.6. Saving users the time of download and recompilation is a
> nice decision. I only ask you to keep an eye out beyond changelogs. We
> should all go to the source when in doubt.

That's what I did (although not document, my fault for being in the lazy
maintainer's role), and I also added #warn statements to see if any of
the code actually compiled used the new windows or the narrow branches
and saw none - so I'm confident the ChangeLog is exhaustive with respect
to the change and format string and typedef at hand.

There's another part of the diff which is, IMO, relevant to quote here
when discussing the source and gives the whole diff its meaning (cut & pasted):

--- /tmp/openvpn-2.0.6/syshead.h        2005-12-08 21:57:49.000000000 +0100
+++ /tmp/openvpn-2.0.7/syshead.h        2006-04-12 11:08:10.000000000 +0200
@@ -366,6 +366,11 @@
 }

 /*
+ * Should statistics counters be 64 bits?
+ */
+#define USE_64_BIT_COUNTERS
+
+/*
  * Do we have point-to-multipoint capability?
  */

And we see by performing the #ifdef manually that this is what remains
whenever syshead.h is included before common.h.

(If the symbol size changed between modules, the linker would complain
about the size growing for some modules.)

| /*
|  * Statistics counters and associated printf formats.
|  */
|   typedef unsigned long long int counter_type;
| #  define counter_format  "%llu"

Which is what 2.0.6 already did on FreeBSD.

> BTW, thanks for a great port (this and many other ones)
> and for doing such a great job maintaining them all through
> the years. We use openvpn at several locations and we
> really appreciate the speed at which your provide updates.
> Thanks!

You're welcome, glad you like it.

I hope that settles the dust I raised.

Kind regards,

-- 
Matthias Andree