Forum OpenACS Improvement Proposals (TIPs): TIP #61: (Approved) Guidelines for CVS committers

Proposal

Formal rules governing CVS commits to the openacs.org code repository, including .LRN and contributed modules.

CVS committers are trusted to make direct changes to the code base for OpenACS and its packages.

  1. Which branch to use
    1. For core packages, new features should always be committed on HEAD, not to release branches.
    2. For core packages, bug fixes should be committed on the current release branch whenever applicable.
    3. For non-core packages, developers should work on a checkout of the release branch of the lastest release. For example, if OpenACS 5.1.0 is released, developers should work on the oacs-5-1 branch. When oacs-5-2 is branched, developers should continue working on oacs-5-1 until OpenACS 5.2.0 is actually released.
    4. The current release branch is merged back to HEAD after each dot release.
  2. New packages should be created in the /packages directory and the maturity flag should be set to zero. (This is a change from previous policy, where new packages went to /contrib/packages)
  3. Code
    1. Only GPL code and material should be committed to the openacs.org CVS repository
    2. Code should only be reformatted when functionality is changed, e.g. when you change control flow and reindent to reflect it.
    3. Database upgrade scripts should only span one release increment, and should follow Naming Database Upgrade Scripts.
    4. Database upgrade scripts should never go to the release version, e.g., should always have a letter suffix such as d1 or b1.
    5. CVS commit messages should be intelligible in the context of Changelogs. They should not refer to the files or versions.
    6. CVS commit messages and code comments should refer to bug, tip, or patch number if appropriate, in the format "resolves bug 11", "resolves bugs 11, resolves bug 22". "implements tip 42", "implements tip 42, implements tip 50", "applies patch 456 by User Name", "applies patch 456 by User Name, applies patch 523 by ...".
  4. When to TIP
    • A proposed change must be approved by TIP if
      • It changes the core data model, or
      • It will change the behavior of any core package in a way that affects existing code (typically, by changing public API), or
      • It is a non-backwards-compatible change to any core or standard package.
    • A proposed change need not be TIPped if
      • it adds a new function to a core package in a way that:
        • does not change the backwards-compatibility of public API functions.
        • does not change the data model
        • has no negative impact on performance
      • or it changes private API.
      • or it is a change to a non-core, non-standard package
  5. Tags
    1. When a package is released in final form, the developer shall tag it "packagename-x-y-z-final" and "oacs-x-y-compat". x-y should correspond to the current branch. If the package is compatible with several different core versions, several compat tags should be applied.
    2. When OpenACS core is released, the openacs-x-y-z-final tag shall be applied to all compat packages.

For example, adding a new API function wouldn't require a TIP. Changing an existing API function by adding an optional new flag which defaults to no-effect wouldn't require a TIP. Added a new mandatory flag to an existing function would require a TIP.

Reasons

We don't currently have clear standards for committing code.

Rule 1.3: First, this ensures that developers are working against stable core code. Second, it ensures that new package releases are available to OpenACS users immediately.

Rule 3.4: If an upgrade script ends with the final release number, then if a problem is found in a release candidate it cannot be addressed with another upgrade script. E.g., the last planned upgrade script for a package previously in dev 1 would be upgrade-2.0.0d1-2.0.0b1.sql, not upgrade-2.0.0d1-2.0.0.sql. Note that using rc1 instead of b1 would be nice, because that's the convention with release codes in cvs, but the package manager doesn't support rc tags.

Rule 5.1: Reason 1: The packagename tag is a permanent, static tag that allows for future comparison. The compat tag is a floating tag which is used by the repository generator to determine the most recent released version of each package for each core version. This allows package developers to publish their releases to all users of automatic upgrade without any intervention from the OpenACS release team.
Reason 2: The compat tags allows CVS users to identify packages which have been released since the last core release.
Reason 3: The compat tag or something similar is required to make Rule 6 possible.

Rule 5.2: This allows OpenACS developers who are creating extensively customized sites to branch from a tag which is stable, corresponds to released code instead of development code, and applies to all packages. This tag can be used to fork packages as needed, and provides a common ancestor between the fork and the OpenACS code so that patches can be generated.

Disadvantages

Having guidelines implies enforcement. Hopefully the committers will be self-enforcing.

Implementation

The rules take effect upon approval. All technical support required already exists except for Package Maturity, which will be in OpenACS 5.2.

Edited after initial publication to reflect Jeff's clarifications of comment formatting; I also made the formatting a bit more verbose for more reliable parsing

Edited to reflect changes from TIP #70.

Collapse
Posted by Jeff Davis on
Approved (with some possible clarifications):

On 4, should renaming or moving a file also require a tip? (an example of this: I would like to move the includes out of /www to /lib).

plsql is currently fuzzy, should we consider plsql API "public".

On code, commit messages for bug/tip/patch should be in the format "bug 11", "bugs 11,22,44". "tip 42", "tips 42,50", "patch 456 by User Name", "patch 456 by User Name, patch 523 by ...". I was thinking we should maybe start using "resolves bug #" so we could automatically construct lists of bugs resolved per release as well (and potentially feed to bug tracker)"

Collapse
Posted by Malte Sussdorff on
approved and good clarifications
Collapse
Posted by Joel Aufrecht on
"On 4, should renaming or moving a file also require a tip? (an example of this: I would like to move the includes out of /www to /lib)."

So the guideline is it should be tipped if ... "it will change the behavior of any core package in a way that affects existing code (typically, by changing public API)"

I would opine that since there was no defined interface for includelets before /lib, includelets in /www are not part of a public interface.  So you could rename them freely in www - I guess it's like a private API - but renaming or changing in  /lib is TIPable.

On the other hand, we've made a bunch of changes to master template etc, just since 5.0, that it seems like overkill to TIP for.  Maybe the rule for public includelets should be that changes operate on the "beg for forgiveness" model.  That is, non-bugfix changes can happen without TIP, but only on HEAD, and authors must pay attention to the response.

Collapse
Posted by Caroline Meeks on
Approved.  Note this is consistant with Tip 58. OACS allows non-GPL code but it is not hosted in the OpenACS.org repository.

Thanks Joel!

Collapse
Posted by Dave Bauer on
Approved
Collapse
Posted by Jade Rubick on
Thanks for writing up these guidelines Joel. They look excellent to me.

One question: does this mean we're going to move a lot of the current /contrib/ packages into the /packages directory? Or is this only for new packages?

Collapse
Posted by Ola Hansson on
This looks very good, but only packages which support PostgreSQL AND Oracle will be candidates for packages/, or is this criterion changing?
Collapse
Posted by Joel Aufrecht on
By these guidelines plus TIP #47: Package Maturity Standards, this criterion is changing. All packages go in /packages, even if they only support one database. A package cannot be maturity level 3, Mature and Standard, unless it supports both databases. All Core and Standard and .LRN packages should be at Maturity Level 3.

Jade: yes, we should roll contrib over after we ship 5.2 with working maturity level.

Collapse
Posted by Roberto Mello on
Approved.

Jeff, perhaps we should use the Debian packaging system way of closing bugs. In the changelog one can write: closes: #12345. The regular expression used by their system is the following:

/closes:\s*(?:bug)?\#?\s?\d+(?:,\s*(?:bug)?\#?\s?\d+)*/i

Debian has an archive maintenance script (katie) that takes care of closing bugs in the bug tracking system when a developer uploads a package with a closes: field in the changelog. We could write a similar script to do that for our bug-tracker. I would be happy to do that.

-Roberto

Collapse
Posted by Jeff Davis on
I think we should use "resolves" rather than "closes" since we have adopted (for better or worse) the convention that bugs are marked resolved and the fix must be confirmed for it to be closed.

"close" is shorter though :)

Collapse
Posted by Lars Pind on
Approved, except for this, which I'd like to have clarified:

"Code should only be reformatted when functionality is changed"

I think it's actually superior to have reformatting done separately from any other change, since then it's easier to figure out what changed functionally in the commmit.

I'd like to see some code formatting standards as well, like indents, style for declaring procs in namespaces, etc. This is something that makes reading code and fixing bugs a bit easier. And reformatting to follow the standards should be okay in my opinion.

Collapse
Posted by Jeff Davis on
"Code should only be reformatted when functionality is changed"

The intent is to avoid mixing reformatting with changes and by "functionality" the meaning was control flow so if you went from:

 
blah
bleh
foo
to
blah 
if {![zim]} { 
    bleh
    foo
}
you would indent bleh and foo properly.

I agree that you want to seperate commits to bring code into conformance with formatting conventions from actual code changes and it's something we should add as part of the guidelines. On the other hand, recognize that reformatting comes with quite a cost to people maintaining seperate archives for their sites, as it can mean tremendous numbers of conflicts.

I did a little checking to confirm what we already knew which was that tcl is overwhelmingly indented 4 spaces (something like 15:1 versus 2).

I think if we wanted to reformat code to bring it into compliance with standards (I think we need to be clearer about what those are) there are good times to it. Best near term would be when 5.1 is pretty much stable and 5.2 is not yet branched. That should be pretty soon (probably after the dotlrn bug bash would be a good time).

Collapse
Posted by Joel Aufrecht on
Marking approved
Collapse
Posted by Eduardo Pérez on
I still think these two points cause many problems to other developers:
<blockquote> For core packages, bug fixes should be committed on the current release branch whenever applicable.
</blockquote>
The problem with this one is that some developers have to develop with old code in the stable branch because HEAD contains bugs. I see many people on the lists that have these problems.

<blockquote> The current release branch is merged back to HEAD after each dot release.
</blockquote>
This doesn't work as well as expected as there are usually conflicts and they don't get resolved very well.

Anyway this is just my humble opinion. You are the ones that approve this proposals.

Collapse
Posted by Jeff Davis on
Eduardo, what would you propose then? That people have to commit on both the release and HEAD themselves?

This doesn't work as well as expected as there are usually conflicts and they don't get resolved very well.
I am not sure this is really accurate; there have been some problems with merge conflicts, but the vast majority of the cases get handled properly and we have not lost any bug fixes on the release branches.

If your proposal is that people are responsible for merging when they commit bug fixes, I think the short answer is that it's not administratively tenable to do it that way unless someone steps forward to take responsiblity for auditing commits to make sure that bug fixes do get rolled forward to HEAD.

The way we do it now may not be ideal but I think it's the best solution with the resources we have now.

Collapse
Posted by Eduardo Pérez on
<blockquote> what would you propose then? That people have to commit on both the release and HEAD themselves?
</blockquote>

Yes, that's what I'm proposing. Since the bugfix commiter has the knowledge of the bug and the bugfix he is the best one to commit on both branches instead someone that doesn't know very well how the bugfix work at release commit from the branch to HEAD.

<blockquote> I am not sure this is really accurate; there have been some problems with merge conflicts,
</blockquote>

These are the conflicts I'm referring to.

<blockquote> If your proposal is that people are responsible for merging when they commit bug fixes,
</blockquote>

What's the problem with committing the bugfix in both branches?

<blockquote> I think the short answer is that it's not administratively tenable to do it that way unless someone steps forward to take responsiblity for auditing commits to make sure that bug fixes do get rolled forward to HEAD.
</blockquote>

You can still merge bugfixes to HEAD.
But people should commit on both branches.

<blockquote> The way we do it now may not be ideal but I think it's the best solution with the resources we have now.
</blockquote>

What additional resources are needed in my proposal?

At least with my proposal people can develop with HEAD.

What do other people think?
Should this be posted to OpenACS Development? (for other developers to know)

Collapse
Posted by Jeff Davis on
Here are the problems I see with your approach:
  • People have to have both a 5.1 and HEAD checkout (a lot of people probably don't keep both around).
  • People have to know how to merge with CVS (a lot don't know how to do it cleanly).
  • It's non-trivial to do the merge for multiple files and it's easy to miss one or two.
  • If you are merging just your changes it can have conflicts that arise from past changes on HEAD or the release which were unmerged (which if we continue to do bulk merges is not as much of an issue).
  • If someone misses merging a change how do we know?

With the current rules we are very confident that nothing gets lost with the primary risk being that a conflict gets resolved the wrong way, at the expense of having HEAD be somewhat out of date with respect to the release branch bug fixes.

Under your suggested methodology, the risk would be code gets lost, individuals merging make the wrong decision merging conflicts.

I would dispute the assertion that the overall error rate would decline if people resolved their own conflicts -- I have been merging roughly monthly for about a year, with a typical merge affecting ~800 files and there have been about 10 conflicts that I am aware of being done the wrong way of 1270 commit sets, affecting 7500 files). Depending on how you calculate it that error rate is somewhere between 1% and .1%; I would claim that if you had the 30 people who had done those commits also responsible for the merges on each commit set, that the error rate would be much higher (and I guarantee we would have lost code/bugfixes along the way).

In order for your approach to work better someone would have to take responsibility for making sure that peoples commits were merged properly and if not remind them to do the merge (and teach people how to properly do merges). And also periodically make sure that no code had been lost.

I am happy to do the merges with the commit guidelines we have now and I am confident in claiming we are not losing code or bug fixes. Under your proposed solution I would not be willing to take that QA role since a) it would be a lot more work, and b) I would not be at all confident that code would not be slipping through the cracks. When I said it would take more resources that's what I meant.

This has come up before and I still don't think there are that many instances of things fixed in 5.1 which have not been fixed on HEAD. If you want to merge to HEAD more frequently then I am only to happy to have you carry out a merge (and tell you how I have been doing the merges to date so it's all done consistently).

If you want to propose changes to the commit guidelines then submit a TIP and lobby the OCT members to vote for it. I would say that unless you found the resources to insure your approach delivered better overall quality that what we do now I personally would vote no.

Collapse
Posted by Roberto Mello on
I agree with Jeff. Our current approach is not perfect, but it's the best we've identified so far.

Jeff has been doing a great job with CVS maintenance and merging, and has proven that it works well.

The problem that you are really trying to address is the disparity between the stable and the HEAD branches. The compromise I can see would be to do more frequent merges. But as Jeff has pointed out, this is more work. If you are willing to take on that work, and can convince Jeff that you can do it well, I'm sure other OCT members would follow.

-Roberto

Collapse
Posted by Joel Aufrecht on
Eduardo, can you explain this a bit more for me:
> For core packages, bug fixes should be committed on the 
> current release branch whenever applicable.

The problem with this one is that some developers have to 
develop with old code in the stable branch because HEAD 
contains bugs. I see many people on the lists that have 
these problems.
I think most developers should be developing against 5.1, not HEAD. The rules about bug fixes vs new features only apply to core, not to the other packages. (I know all this could be clearer - it's taken me about two years to understand even as much as I do now, which is still only a subset of the problem space and a smaller subset of the solution space - and I'm going to work on some docs this week.) The only time you should be working on HEAD is if you are doing development directly on core packages. This approach should get you all of the benefits - you can still do whatever you want to your packages. If you are working on the standard packages (file-storage, forums, etc), you can still do minor or even major releases, and you know your releases will be available for most other users much sooner than if they depended on HEAD core.
Collapse
Posted by Joel Aufrecht on
I've updated TIP #61 with the amendments from TIP #70.  I took the liberty of relabelling the new rules 5 and 6 as 5.1 and 5.2, and moved some of the reasons out to the Reasons section for easier reading.