Re: svn commit: r822263 - /apr/apr-util/branches/1.4.x/CHANGES

[Graham Leggett]

trawick@... wrote:

> URL: http://svn.apache.org/viewvc?rev=822263&view=rev
> Log:
> Remove entries for iterative fixes to the crypto feature since
> it has not yet been in a release.
>
> Remove entry for a change to the apu_dso_load() interface since
> it is a private function.

Can someone explain (and document) the new thinking behind how CHANGES
is supposed to work?

Regards,
Graham
--

[Jeff Trawick]

On Tue, Oct 6, 2009 at 9:58 AM, Graham Leggett <minfrin@...> wrote:

trawick@... wrote:

> URL: http://svn.apache.org/viewvc?rev=822263&view=rev
> Log:
> Remove entries for iterative fixes to the crypto feature since
> it has not yet been in a release.
>
> Remove entry for a change to the apu_dso_load() interface since
> it is a private function.

Can someone explain (and document) the new thinking behind how CHANGES
is supposed to work?

Generally: If it isn't useful info to library users, it doesn't go in CHANGES.  We track CHANGES based on releases, not based on Subversion commits, so a further refinement is that entries should be useful info for library users who download a release from us, not for those who svn up every few days.

If before a new feature is delivered the first time it has to be patched n times (like many new features do), the only entry needed in CHANGES is the one that announces the feature.  Once the feature has actually been in a release, CHANGES entries for following releases should show how it was fixed.

If some internal detail is changed (like a change to the private apu_dso_load function), it doesn't go in CHANGES.

I don't see anything new about this philosophy.  (That's not to say that the philosophy has been followed faithfully until now.  It just seemed obvious to me that this particular CHANGES should be cleaned up on behalf of folks wondering why there is going to be an APR-util 1.4.0 release.)

[Graham Leggett]

Jeff Trawick wrote:

> Generally: If it isn't useful info to library users, it doesn't go in
> CHANGES.  We track CHANGES based on releases, not based on Subversion
> commits, so a further refinement is that entries should be useful info
> for library users who download a release from us, not for those who svn
> up every few days.
>
> If before a new feature is delivered the first time it has to be patched
> n times (like many new features do), the only entry needed in CHANGES is
> the one that announces the feature.  Once the feature has actually been
> in a release, CHANGES entries for following releases should show how it
> was fixed.
>
> If some internal detail is changed (like a change to the private
> apu_dso_load function), it doesn't go in CHANGES.
>
> I don't see anything new about this philosophy.  (That's not to say that
> the philosophy has been followed faithfully until now.  It just seemed
> obvious to me that this particular CHANGES should be cleaned up on
> behalf of folks wondering why there is going to be an APR-util 1.4.0
> release.)

Right, there is nothing new about this philosophy at all, but I see no
match between the philosophy you've mentioned above, and criteria for
entries you removed from CHANGES.

For example, you removed this:

  *) Add apr_crypto implementations for OpenSSL and Mozilla NSS. Add a unit
     test to verify the interoperability of the two modules. Builds default
     to disabled unless explicitly enabled.
     [Graham Leggett]

One of the key reasons the original apr_ssl code was vetoed was because
no second implementation existed that proved the abstraction was viable.
To fix that veto, implementations for OpenSSL and NSS were provided, and
the above changes entry confirms that.

Someone asking the question "was my veto resolved" is going to go
straight to CHANGES to check. Only you've just removed the entry, so
there isn't a way they'll know, and they'll end up reopening the
discussion unnecessarily.

The entries you've removed from CHANGES represent a significant period
of work that cover a number of months of development, I suspect you have
underestimated just how much scope these entries covered. They detail
key differences between APR v1.3 and APR v1.4, and are definitely of
interest to library users. Please revert.

Regards,
Graham
--

[Jeff Trawick]

On Tue, Oct 6, 2009 at 11:24 AM, Graham Leggett <minfrin@...> wrote:

Jeff Trawick wrote:

> Generally: If it isn't useful info to library users, it doesn't go in
> CHANGES.  We track CHANGES based on releases, not based on Subversion
> commits, so a further refinement is that entries should be useful info
> for library users who download a release from us, not for those who svn
> up every few days.
>
> If before a new feature is delivered the first time it has to be patched
> n times (like many new features do), the only entry needed in CHANGES is
> the one that announces the feature.  Once the feature has actually been
> in a release, CHANGES entries for following releases should show how it
> was fixed.
>
> If some internal detail is changed (like a change to the private
> apu_dso_load function), it doesn't go in CHANGES.
>
> I don't see anything new about this philosophy.  (That's not to say that
> the philosophy has been followed faithfully until now.  It just seemed
> obvious to me that this particular CHANGES should be cleaned up on
> behalf of folks wondering why there is going to be an APR-util 1.4.0
> release.)

Right, there is nothing new about this philosophy at all, but I see no
match between the philosophy you've mentioned above, and criteria for
entries you removed from CHANGES.

For example, you removed this:

 *) Add apr_crypto implementations for OpenSSL and Mozilla NSS.

It is in CHANGES; it isn't listed separately from the apr_crypto announcement.
 

Add a unit
    test to verify the interoperability of the two modules.

IMO that is developer-only information that isn't needed by users.

Builds default
    to disabled unless explicitly enabled.

That could be interesting; I'll add it back.
 

    [Graham Leggett]

That part is on the one apr_crypto announcement.
 

One of the key reasons the original apr_ssl code was vetoed was because
no second implementation existed that proved the abstraction was viable.
To fix that veto, implementations for OpenSSL and NSS were provided, and
the above changes entry confirms that.

As I said, it is in CHANGES still.
 

Someone asking the question "was my veto resolved" is going to go
straight to CHANGES to check. Only you've just removed the entry, so
there isn't a way they'll know, and they'll end up reopening the
discussion unnecessarily.

The entries you've removed from CHANGES represent a significant period
of work that cover a number of months of development, I suspect you have
underestimated just how much scope these entries covered.

"period of work" and "number of months" aren't relevant to CHANGES
 

They detail
key differences between APR v1.3 and APR v1.4, and are definitely of
interest to library users.

I believe the key differences are in there.  I'll add back the "disabled by default" aspect.  First, 

please call out explicitly the other CHANGES entries I removed for which you would like an explanation.

[Graham Leggett]

Jeff Trawick wrote:

> I believe the key differences are in there.  I'll add back the "disabled
> by default" aspect.  First,
> please call out explicitly the other CHANGES entries I removed for which
> you would like an explanation.

wrowe has made a number of calls for people to review the crypto code in
APR v1.4, and the entries in CHANGES are a key part of such a review.

As I requested in my previous mail, please revert (as in, -1).

Regards,
Graham
--

[Jeff Trawick]

On Wed, Oct 7, 2009 at 5:11 PM, Graham Leggett <minfrin@...> wrote:

> Jeff Trawick wrote:
>
>> I believe the key differences are in there.  I'll add back the "disabled
>> by default" aspect.  First,
>> please call out explicitly the other CHANGES entries I removed for which
>> you would like an explanation.
>
> wrowe has made a number of calls for people to review the crypto code in
> APR v1.4, and the entries in CHANGES are a key part of such a review.
>
> As I requested in my previous mail, please revert (as in, -1).

No, not at this time.  I don't think your reasoning for the veto
applies to these changes.  (I don't think CHANGES entries are a key
part of any new feature review.  Users just have to know the feature
exists.  Presumably programmers will then look through the include
files for keywords and find the documentation.)

I think it is worth mentioning that I posted the patch four days
before I committed, and two of our colleagues approved it.  That
doesn't mean it can't be changed, or even veto-ed, but perhaps with
that in mind you'd accept that it is fair to call out specific entries
within the commit that you disagree with for the reason you stated.

Here they are one by one:

 *) Fix the saving of the old LIBS, CPPFLAGS and LDFLAGS when OpenSSL
-     and NSS are detected. [Graham Leggett, Ruediger Pluem]
-
-  *) apr_crypto_nss: Oh that it was this easy. Use pkg-config to find
-     NSS where possible. [Graham Leggett]
-
-  *) apr_crypto_nss: The nss.h header file could be in nss or nss3, the
-     prerror.h header file could be in nspr4. [Graham Leggett]

-  *) Fix a bogus initialisation of the IV size in the NSS crypto driver.
-     [Graham Leggett]
-
-  *) Make sure that the underlying result code during driver initialisation
-     is exposed to the caller. [Graham Leggett]
-
-  *) Provide a mechanism to provide the recommended crypto driver to
-     calling application. [Graham Leggett]
-
-  *) Move APU_HAVE_CRYPTO from private apu_config.h to public apu.h.
-     [Ruediger Pluem, Graham Leggett]

My understanding: Each of these deleted entries thus far is a minor
implementation detail related to the new apr_crypto feature.  As the
feature was never released, they do not fix a problem for the users of
our library.  OTOH, any of these if tied to a particular user symptom
could be interesting in the change log for a bug fix release, such as
1.4.1.

Why is this understanding is wrong, or at least why is this
information needed by reviewers of the apr_crypto feature?  (We can
find those reviewers a better place to find this kind of information,
such as a handful of URLs taking them to the the commit logs.)

<I've omitted the combination of the apr_crypto new-feature entries,
which we discussed earlier and which I'm already said I'd change once
I understand what to do with the rest.>

-
-  *) Expose the apr_dso_handle_t when calling apu_dso_load, so that the
-     crypto code can call apr_dso_error and find out why the dso load
-     failed. The existing LDAP and DBD code ignores this, as their APIs
-     do not yet allow for errors to be returned. [Graham Leggett]

My understanding: Internal implementation detail that does not fix any
released code.  Released or not, by itself it isn't interesting for
CHANGES.  (If APR-util 1.4.x had a new apr_dbd_foo_ex() API that has a
new parameter to be filled in with a low-level load failure, then that
new API would be logged in CHANGES; but this particular aspect isn't
interesting.)

I don't see why any of the apr_crypto reviewers need to see this.

[Jeff Trawick]

On Wed, Oct 7, 2009 at 5:48 PM, Jeff Trawick <trawick@...> wrote:

> On Wed, Oct 7, 2009 at 5:11 PM, Graham Leggett <minfrin@...> wrote:
>> Jeff Trawick wrote:
>>
>>> I believe the key differences are in there.  I'll add back the "disabled
>>> by default" aspect.  First,
>>> please call out explicitly the other CHANGES entries I removed for which
>>> you would like an explanation.
>>
>> wrowe has made a number of calls for people to review the crypto code in
>> APR v1.4, and the entries in CHANGES are a key part of such a review.
>>
>> As I requested in my previous mail, please revert (as in, -1).

This is not subject to veto.

> <I've omitted the combination of the apr_crypto new-feature entries,
> which we discussed earlier and which I'm already said I'd change once
> I understand what to do with the rest.>

I just restored the missing piece of information (build disabled by
default).  I hope this helps.

[Graham Leggett]

Jeff Trawick wrote:

> My understanding: Each of these deleted entries thus far is a minor
> implementation detail related to the new apr_crypto feature.  As the
> feature was never released, they do not fix a problem for the users of
> our library.  OTOH, any of these if tied to a particular user symptom
> could be interesting in the change log for a bug fix release, such as
> 1.4.1.

Now that I have some time to draft a proper reply.

The argument that "the feature was never released" becomes moot the
moment the code is released, and that will happen when we ship APR
v1.4.0 (or APR v2.0.0).

At that point, end users are going to want to know what's changed
between the last release and this one. And in the case of APR v2.0.0,
people are *really* going to want to know what has changed.

More specifically, people are going to want to know exactly how that
change got there, and that feature morphed over time to become what it
is now.

What you're arguing for is a "dumbed down" version of CHANGES, where
only a general announcement of a feature is included, but the core
details of how the feature got there are erased. And I'm firmly opposed
to that.

CHANGES isn't a marketing brochure, it's the thing that will show up in
google searches when end users are trying to debug a problem. And we can
fully expect problems to found when APR v2.0.0 is released, and people
to want to fix them.

There is a further, deeper reason why I oppose these changes to CHANGES
- the changes documented were committed 18 months ago. Without having
recent memory to go by, we are arbitrarily chopping out sections of our
history based in whether an entry looks "important" or not. We are
changing our historical record, and that is a really bad idea.

We have no idea what information might be vital to an end user to solve
a particular problem they have. An innocuous looking change could be the
cause of a bug. And no, end users of the library aren't going to be
checking out apr from svn to try and find the history.

We certainly should not be making end user's lives more difficult for
the sake of saving a few bytes.

On this basis, my -1 stands. Please revert the CHANGES (so to speak).

Regards,
Graham
--

[Jeff Trawick]

On Tue, Oct 13, 2009 at 8:22 AM, Graham Leggett <minfrin@...> wrote:

> Jeff Trawick wrote:
>
>> My understanding: Each of these deleted entries thus far is a minor
>> implementation detail related to the new apr_crypto feature.  As the
>> feature was never released, they do not fix a problem for the users of
>> our library.  OTOH, any of these if tied to a particular user symptom
>> could be interesting in the change log for a bug fix release, such as
>> 1.4.1.
>
> Now that I have some time to draft a proper reply.
>
> The argument that "the feature was never released" becomes moot the
> moment the code is released, and that will happen when we ship APR
> v1.4.0 (or APR v2.0.0).

You're not understanding what I'm attempting to say.  The distinction
of whether or not the feature was released and how that affects the
necessary change entries is exemplified by the following:

If apr_crypto in future APR-util 1.4 had some problem like not finding
nss.h in all the possible locations and that broke the apr_crypto
feature for somebody and it was fixed in the .1 release, then the .1
release would list the fix in CHANGES.

If such a problem was found before apr_crypto was released, the fix is
just part of the initial delivery and doesn't need to be in changes.

>
> At that point, end users are going to want to know what's changed
> between the last release and this one. And in the case of APR v2.0.0,
> people are *really* going to want to know what has changed.
>
> More specifically, people are going to want to know exactly how that
> change got there, and that feature morphed over time to become what it
> is now.

I don't agree.  I also don't think trivial stuff like how to find
nss.h, for example, is a morphing of a feature.

> What you're arguing for is a "dumbed down" version of CHANGES, where
> only a general announcement of a feature is included, but the core
> details of how the feature got there are erased. And I'm firmly opposed
> to that.

The idea I'm trying to state is that CHANGES should be limited to
listing new features or fixes for problems that existed in the
previous release.  I don't think that is dumbed down.  CHANGES is end
user documentation.

Subversion history covers the trail from first commit to the present.

>
> CHANGES isn't a marketing brochure, it's the thing that will show up in
> google searches when end users are trying to debug a problem. And we can
> fully expect problems to found when APR v2.0.0 is released, and people
> to want to fix them.

I don't see your point.  Are you saying that if apr_crypto isn't
enabled for somebody trying to use 1.4 because it can't find some
OpenSSL or NSS artifact, then they should be able to find in CHANGES
that other similar problems were fixed before 1.4 was released?

The intended use of CHANGES in resolving a problem is to show which
problem symptoms in previous releases have been corrected.

> There is a further, deeper reason why I oppose these changes to CHANGES
> - the changes documented were committed 18 months ago. Without having
> recent memory to go by, we are arbitrarily chopping out sections of our
> history based in whether an entry looks "important" or not. We are
> changing our historical record, and that is a really bad idea.

Use Subversion history.

> We have no idea what information might be vital to an end user to solve
> a particular problem they have. An innocuous looking change could be the
> cause of a bug. And no, end users of the library aren't going to be
> checking out apr from svn to try and find the history.

CHANGES is not for solving new problems.  It is for recording what was
fixed or enhanced in released levels of APR.

>
> We certainly should not be making end user's lives more difficult for
> the sake of saving a few bytes.

This is not about saving a few bytes.  It is about getting needless
implementation details out of the documentation.

>
> On this basis, my -1 stands. Please revert the CHANGES (so to speak).

You cannot veto documentation.

You should follow the group consensus in what sort of details are
included in CHANGES.  If you look at the entries for the unreleased
levels of APR and APR-util, you won't find any trail of iterations for
any of the new features, even though some was required to get it
building everywhere and fixing initial problems.  Also, recall that
two other developers approved my editing of the trail for the new
apr_crypto feature before I committed.

Including these details in CHANGES is unsustainable.  CHANGES would be
completely unusable if these iterative fixes for new features were
included.  The few people who need to know this kind of information
will have to get it from Subversion history.

[Joe Orton]

On Tue, Oct 13, 2009 at 09:14:54AM -0400, Jeff Trawick wrote:
> Including these details in CHANGES is unsustainable.  CHANGES would be
> completely unusable if these iterative fixes for new features were
> included.  The few people who need to know this kind of information
> will have to get it from Subversion history.

I agree with everything Jeff is saying here, especially this paragraph.

Regards, Joe

[William A. Rowe Jr.]

Jeff Trawick wrote:
> On Tue, Oct 13, 2009 at 8:22 AM, Graham Leggett <minfrin@...> wrote:
>
>> We certainly should not be making end user's lives more difficult for
>> the sake of saving a few bytes.
>
> This is not about saving a few bytes.  It is about getting needless
> implementation details out of the documentation.

Precisely.  The changes file is not for developers, not for egos, it exists
entirely for the benefit of end users, who don't know the different between
one trunk pre-apr-1.4.0 and released apr-1.4.0.

Anyone following trunk is following *svn commits*.  The egoboo derives from
svn commits.  Only the necessary information to tell consumers (end users)
"what changed?!?" belongs in CHANGES.

>> On this basis, my -1 stands. Please revert the CHANGES (so to speak).
>
> You cannot veto documentation.

Rather; you must provide a technical rational for your veto, and reading the
post which Jeff has replied to, it is groundless.

Every point you raised would have applied to changes made after 1.4.0 had been
released, if applied to the code prior to 1.4.1.  All of the points were very
salient.  And they were apropos of nothing; APR 1.4.x does not exist until this
group releases it, and we have not.