Forum OpenACS Improvement Proposals (TIPs): TIP # 23 (Clarify/Resubmit): Core code changes and bug fixes require approved TIP.

Anyone with commit rights to the OpenACS core must agree that code changes and bug fixes must be in response to an approved TIP and related item in bugtracker.

Those who don't agree, or violate this rule can have commit privileges removed immediately by any OCT member. Privileges can be restored with 2/3 vote of all members.

Rational is that if the OpenACS has survived this long with the current code, another week isn't too long to wait to fix it. Simply placing items in bugtracker doesn't provide enough notice or opportunity for response from other developers.

Excluded are changes such as mis-spelled words, or variable names, or other changes which do not effect the logic of the internal or external working of an api.

Guidelines should be suggested for what 'doesn't require' a TIP. When in doubt TIP it out.

I should clarify that this was meant to apply to API changes, not every line of every file in a core package.

Disapprove. I think small API changes done appropriately, for example adding of a new switch, or deprecating of an old proc and making it invoke a new proc, should not require a whole voting process. It would slow us down too much. Major API changes and architecture changes should go through TIP though.
I think that at a minimum, anything that breaks backwards compatibility should require a TIP.
I agree with Peter, I think Tom's proposal is a bit too far reaching.  We should not break existing code but minor extensions to an existing proc that don't break anything shouldn't require a full-blown TIP.

Tom - care to comment?

Is there currently any other place someone can follow changes? I had thought a separate forum where api changes could be at least logged might be helpful.

My opinion is that a lot of thought goes into changing an api, but this thought remains in the head of the person making the changes. It might be obvious in a lot of cases why the change is being made. Other times not.

There's Jeff's CVS browser.

Nonetheless, I still wish we had a cvs commit diffs mailing list.

I think that at a minimum, anything that breaks backwards compatibility should require a TIP.

Joel - do you mean changes to "actual" or "documented" functionality?

I'm assuming (based on the poster and timing) that this TIP is a reaction to the ad_page_contract stuff, so within that context - if the rule is that changes to the documented behaviour of core APIs require a TIP then Jun's changes wouldn't have required one (since the behaviour when vars are set before ad_page_contract is called isn't specified in the documentation).

Conversely, if the rule is that any changes to behaviour of core APIs for a given set of inputs (changes to existing functionality rather than API additions) require a TIP, then people are in effect being encouraged to add flags to handle the quirks of their own requirements, which in my opinion is likely to lead to a *more* fragmented and harder to understand API.

I don't know what the solution here is, but I don't think it lies in setting formal rules that won't in practice be followed.

All I'm trying to encourage is that ideas be run by those who are ultimately responsible for the core, I thought that was the OCT. Maybe too much is included in the core, I don't know. Specifically I meant API procedures. And I meant actual behavior, because that is what folks rely on.

I want to be able to write a package and not have to change it for every release of OpenACS. If I have to do this, it would be nice to have advance notice of the change, maybe a summary of how to upgrade the api calls, etc.

I think this thread could be boiled down to:

DO NOT EVER BREAK EXISTING CODE

I know that it's more difficult to work this way because you can't just change the code when you want. But I've seen too many packages move from working to obsolete because they "don't have adequate maintenance". The point is, they should require *no* maintenance once they are accepted into the mainstream. If a great, new, slick API is appropriate and it is *not* compatible with existing code, it should go into a new namespace and existing core (or other package) code should be refactored to interface with the new API.

  • Nobody's code breaks
  • Packages only become obsolete when a newer, bigger, better package provides better service than the existing package
  • Developers don't have to refactor tested and QA'd production code every time a new OACS release comes out
  • Users can keep up with *every* new release if they want to
  • Contributions by developers to the community do not become a maintenance nightmare
Too difficult?
Randy, that sounds perfect to me.  However, I also agree with Russell that we shouldn't have a proliferation of special-case switches, either.

Ultimately, I think we have to rely on people to use common sense when making toolkit changes, and also to speak up when changes have been made that are causing other people problems.

janine

I guess it is pretty clear that you need a TIP if the new API is breaking any existing code. Do we need a TIP for API changes in packages? I don't think so. But we need to have a place to announce this.

What I'm suggesting is:

- API change that breaks the core: TIP
- Every other API change (including packages): Post in an API change forum.

If people are unhappy with a change, they can start the discussion around the announcement.

How do you know in advance if changing the behaviour of some procedure will break existing code? We don't have a full set of regression tests yet... and there are surely places where parts of the core depend on undocumented functionality of other parts of the core...

I guess my concern (not that I'm a commiter) is that any process that is put in place that can lead to people having commit rights revoked ought to be well defined and possible for *everyone* to live with *all the time*...

As soon as someone who has been smacked can turn around and say "well Lars broke X without issuing a TIP, why wasn't his access revoked" when the commit in question fixed an obvious bug whose behaviour happened to be relied on elsewhere, the process falls over. As others have said it's not feasible to require a TIP for every commit that could change the behaviour of a core API (this includes bugfixes), so making rules that require people to do so or face the most severe penalty possible in this sort of community will inevitably lead to inconsistent application of those rules.

I think my point is that you can't legislate common sense, and that looks like the aim of the rules suggested here so far. Common sense has so many corners and fuzzy edges that you'll never get it right, someone will feel wronged, and in practice you'll end up with a situation no better than the informal self-policing model that we have at the moment.

I don't think the TIP has the idea of ruling common sense, at least not to my knowledge. And if I'm not mistaken, the OCT has to agree to reverting the commit rights (not an automatic behaviour). I'm not even in favour of beeing very drastic here. But at least the awareness has to be risen, that you cannot just change the API, especially if you know it might break existing code. How do you know ?

- You remove a parameter
- You change the way the parameter is beeing used in a not obvious way (e.g. if you suddenly decide to user first_names as the container for the birthdate).
- You change the return values (number / order)

And this is why I said, if in doubt, just announce it. The commiter might not be aware of it, or too overworked to notice (we are humans. Humans are prone to make error). Therefore I absolutly second Dirks request for a CVS diff mailinglist. It has helped us internally tremendously so far and prevents serious mistakes to get into the code (and teaches a little bit of coding style and commit comments).

Actually, on one hand I understand Tom's anger - but then: we do have CVS so that we can track changes in the code and easily revert: removing an erroneous commit isn't really time-consuming.
As to Randy's "Do not ever break existing code" - that's more of a wish, right?

Nobody wants to do it, but once in a while it turns out that it is impossible. For the upcoming 5.0 release I18N doesn't break the past, the templating system change does. There simply was no other way to fix this behaviour.

We need to balance stability with being able to move forward - let's see how OCT governance works out.

I think communicating such changes is important and not changing things without a consensus is also quite important but I think the current machanisms for oversight are reasonable (longish beta periods, nightly builds, commit mails, the cvs log browser's RSS feed, and the existing forums and irc).

Formalizing this for the core API makes sense to me but for a proposal like this to be acceptible we would need to be more careful about having things labeled private and public (and would need to add an experimental attribute to make clear that certain APIs were still open to change w/o the onus of writing a TIP). In addition I think "changing an api" would have to mean causing existing unit tests to fail (and of course such unit tests would need to be written...). As it stands, generally there are no unit tests to define current behavior and so the expected behavior is implicit in the way the APIs are used (and sometimes explicit in the documentation -- although there are enough conflicts between the code and documentation that even that is not really a good way to define expected behavior).

In any case, I find the penalty overly harsh and prone to capricious or contentious revocation of commit. Revoking commit for someone should be carefully considered, be the consensus of the OCT, and should never be up to one person (you need only look at times it's happened in other open source projects to see the potential repercussions). I would strongly object to any element of project governance which would allow something like that to happen.

Well there were lots of good ideas and different perspectives which I hadn't considered. For one, I didn't realize that the core API was changing so rapidly. Most of the packages I have written have worked unchanged for the last several years, maybe with the exception of the context bar.

So first off, I think my idea of what the core APIs include isn't as broad as what is actually in the core.

Second, my proposal wasn't to stop _any_ change or coding from taking place. The main ideas, maybe poorly communicated, were:

  • that there should be a central place where the reasons for the changes are recorded. Maybe explaining the changes so that folks know what to expect, without looking at a cvs log and piecing togeather that with the original file.
  • that the OCT take active oversight of these changes. Maybe that means something as simple as reading about the changes. Since cvs is always there to revert changes, my main concern is that a change, which is not good, goes in to support a package, unintentionally breaking other packages. Once another package needs this change, it is hard to use cvs to fix the matter.
  • Someone reviews the code for core changes. I think the rules and responsibilities TIP had some ideas about that. The package maintainer needed to review patches before they were used on the core.
  • The TIP process is finite. Discussions on what changes should take place that happen on the regular forums are not. If someone gets tired of talking about what they are going to change, and they have commit rights, they can just stop and make the change anyway. A TIP makes the decision process understandable to everyone, and it gets multiple views into the discussion. Regular forum discussions usually only involve a few voices, and maybe not the most knowledgeable ones on the subject. I know that after reading this TIP, my opinions have changed and my understanding of the issues have expanded. My opinion: this is the point of the TIP process. One or two, or even three dissenting voices isn't going to stop a change.

Third: revoking commit rights should always be possible. It should be as easy as getting them in the process. But this would probably be rarely applied. I didn't see it as harsh because I didn't imagine that it would happen very often. However Jeff's points are good. Maybe this is too heavy a stick and it could be used as a punishment. Removing commit rights is something that can be dealt with completely separately from the main issue of documenting core API changes.

What shall we make with this TIP? I support Tom's motion, especially as voiced in the last posting. But i'm not sure I'd consider it a TIP anymore, for lack of clear guidelines.

How about reopening the TIP process for this one, with Tom's suggestions but beeing a little bit more specific (central place, oversight (is reading enough), who is going to review the changes and in what timeframe).

I this should be either marked rejected or else written up to reflect the feedback and the whatever consensus there is and resubmitted.