Forum OpenACS Q&A: Quality of OpenACS

Collapse
Posted by Malte Sussdorff on
Currently the OCT has started a discussion on the maturity of the OpenACS code base and the expectations of CVS HEAD. Here is my take on this (and hopefully non OCT members will give their comments as well).

On using code from CVS:

Use CVS at you own risk. Always. Do not assume CVS code works. If you use code from CVS, it will most likely fail. If it fails, file a bug report. Wait for the bug report to be dealt with, then try again.

On releases (read: -compat flag) and maturity level:

We have releases of OpenACS Core and packages. These are clearly marked by the openacs-5-2-compat flag and show up in the OpenACS Repository. If a developer makes the effort to mark a package compatible to a certain version, he knows that it will work and apparently has tested it.

The maturity level is for a package *at a given time*. You should always be able to commit a package in a lower maturity level than it was before, as long as you do not release it.

This method has a couple of advantages:

A) You do not have to fork to /contrib if you have new ideas and they might not adhere to the standards of the current code maturity immediately.

B) We have package releases. Only a released package is supposed to work. Though this reduces the number of packages available, it increases the quality of the code and the user can be sure that someone is looking over the package.

C) It reduces the need to merge from contrib to release.

D) Developers are able to commit code early on and exchange code. I can only say, if stricter rules with regards to HEAD commits had been into place, contacts and project-manager would never have been committed from our side as they are still under heavy development and we only have time every few months to test a proper release. Which would have made the exchange with Matthew and Alex hard to non existent.

For the future I suggest:

A) Post clear guidelines what to expect from OpenACS CVS and from the released packages. This is the following:

  • If a package shows up in the OpenACS Repository (not CVS) then a developer has decided that it is save to use in the maturity level given to it. No released package should have a maturity level < 1.
  • If a package resides in the CVS repository of the currently released branch of OpenACS, then the chances are high, that the package works in the maturity level given to it. This has not been proved by a developer yet though.
  • If a package resides on CVS HEAD, then chances are high that it breaks. OpenACS core package will be cleaned up and tested before the release of the next version. Before that, using OpenACS Core from HEAD is for testing purposes only and at your own risk. We encourage you to do this though in your development areas once the previous branch has been declared final.
  • Any package without a maturity level is not supposed to work, regardless where it resides.

For CVS commits we should encourage / enforce to our abilities the following rules of conduct:

  • If you work on a package and think your changes reduce the maturity, decrease the maturity level. DO NOT RELEASE.
  • Only release packages if their maturity level matches the level of the version before and you have tested that it works.
  • Only release a package if you are willing to fix bugs that did not exists in the release before (read: that have been most likely introduced by you).
  • Do everyone a favour and try to follow the release guidelines for OpenACS Core before releasing a package on your own. After all, if critical or serious bugs exist, how are you able to use the package.
  • Before committing your code to the CVS repository, make sure the package you commit to has someone else (preferably from the OCT) marked as "code reviewer". A code reviewer will take a look at your CVS commits and make sure no accidental commits sneak in. The code reviewer should be stated at the Package page in XoWIKI.
  • Before releasing a package, make sure that the XoWIKI page for the page is filled with sensible data (e.g. the name of the reviewer). Furthermore, detail out (and if only brief) what you changed for this release. If you can't remember, feel free to state "and much more...", but at least make others aware that a lot has changed.
  • Do not release OpenACS Core packages, unless you talked to the OCT before it.
  • Make use of dependencies and clean them up before a release.

On code reviewer:

A code reviewer is a person, who is willing to take a glance at the CVS commits of a package and make sure no custom code sneaks in by accident or the new code is not documented enough to make sense. The code reviewer is not supposed to test the package or even use the package himself. A code reviewer can always drop his reviewing obligation at any time. It is in the responsibility of developers who want to commit a package, to find a code reviewer first. The OCT should be helpful in finding a code reviewer if initial tries by the developer fail. The OCT is not obliged to become code reviewers for all packages, though Code Reviewers for Core packages have to be members of the OCT.

Collapse
2: Re: Quality of OpenACS (response to 1)
Posted by Torben Brosten on
Regarding complaints about quality, I think there is greater understanding when version numbers more accurately reflect the ongoing evolution of OpenACS.

What is the difference between a minor release and a major release?

4.7d became 5.0 due to the number of significant changes to the core. See https://openacs.org/doc/openacs-5-0/release-notes.html

5.1 seemed to be a maturing of 5.0 with no new core features. See https://openacs.org/doc/openacs-5-1/release-notes.html

5.2 introduced callbacks and the kernel now requires acs-lang. AFAICT, these changes have resulted in significant coding change requirements to most all packages. Considering the time it has taken to get to release stage etc. why was this not changed to version 6.0? The release notes do not even mention callbacks: https://openacs.org/doc/openacs-5-2/release-notes.html

5.3 is still CVS Head, right? This version requires tcllib, and introduces xotcl, and a portals method right? Aren't these features creating yet another paradigm shift in programming packages for OpenACS? Shouldn't this be versioned to 7.0.. or at least 6.0?

When I tell people 5.3 is really version 7, they understand why OpenACS (as code, website and community) is exhibiting some of the symptoms that they see.

---
Malte, I think you've stated pretty well how things work (or are supposed to). Yet I have a couple of questions.

Can't package maintainers review other package code, and the OpenACS community review code? I do where I can.

Doesn't requiring OCT members to review code create a new technical/skills requirement on the team role which has not been there?

Collapse
3: Re: Quality of OpenACS (response to 1)
Posted by Dave Bauer on
Malte

I absolutely do no agree with checking in lower quality code than what is existing.

It should be reasonable to expect that once a package is working, new development will improve or add new features etc, without breaking existing functionality.

It seems there are 3 types of pacakges, 1) acs core packages, 2) the "important" optional pakcages, basically those that make up dotlrn, and 3) other optional packages not in wide use.

Now, there are always bugs, etc, so I don't expect perfection, but an effort to always check in working code would help.

This requires a little more effort on the developer's part, to design new features in smaller chunks that more a package forward a little at a time. The other option is to wait to commit until a package is tested, especially for 1) new install of openacs and 2) upgrade.

Another key is to add automated tests is you change existing behavior. The tests can confirm that the original behavior works, and the new feature also works.

These are goals, and guidelines, not requirements, but if the community can work towards these goals, the quality will improve.

Regarding releases 1) only the release manager should release OpenACS Core packages. Right now, we are calling Don the release manager, he took over when I didn't have enough time to manage the 5.2.3 release.

Thanks for stressing following the release process, I think it does work, and is helpful to remember to test a new install bfore you release.

Collapse
Posted by Malte Sussdorff on
Dave, in the szenario you describe we will not need a release mechanism any more. Anything that is committed should be automatically released under the maturity level, as the conditions for the commit are exactly the ones I propose for releases.

So the question remains do we want strict commit or strict release rules (as we agree on the goals/guidelines/rules/bylaws/...)? What are the pros and cons of each approach. And what are the consequences for the development style and sharing of code of individual committers.

On a related note, I know that a lot of code, patches, new functionalities exist out there in the open which no one is committing, not because the functionality is not good, but because they do not have the time to test it in a proper way that will make it release worthy. My take was always to have these kind of features in HEAD and jointly figure out how to test them before release. If my take does not fit with the community at large, I would like to know how and where to exchange code that I deem interesting but did not have to time to write automated tests or test a new install with.

Collapse
5: Re: Quality of OpenACS (response to 1)
Posted by Janine Ohmer on
As I have said before, I don't agree with committing code that the developer knows is not finished. That doesn't mean that everything that goes into HEAD has to be perfect, it may not be fully tested, but it shouldn't be completely non-functional either. IMHO checking out from HEAD means running on the bleeding edge of the latest code, not tripping and stumbling through a minefield.

With the current level of testing resources we have available, I'm fairly confident that what would happen under the system Malte proposes is that most of the non-core packages would drift backwards in maturity until they were all broken, and then those who needed them would end up having to fix them. This places a huge burden on other developers. I can say this with some authority as it happened to me some months ago; I spent many hours of my client's time making a package work again, and in the process I made it incompatible with the other develoeper's work, so if/when he commits again the same thing is going to happen. It just makes for a big mess.

Perhaps we need a separate branch, or even a separate repository, for people to place their work in progress? I'm all for sharing unfinished code, but would prefer to not pollute the main repository with it.

Collapse
Posted by Alex Kroman on
I know that there is a lot of code sitting out there that won't be committed because when developers try to install a package from HEAD they get the message "Installation Failed".

Commits to HEAD should be working code that are unstable not because they don't work but because they've only been tested on the developers own machine with a vanilla OpenACS installation.

Collapse
Posted by Torben Brosten on
Janine, are you saying that the community development of the re-write of ecommerce should happen elsewhere?

https://openacs.org/xowiki/pages/en/ecommerce-g2

Collapse
Posted by Janine Ohmer on
Well, I'm not the one who gets to decide such things, but IMHO a major refactoring should not be committed to HEAD until it's at least at alpha stage. Until then it should go into either a branch or it's own repository.

Really the most important thing is that everyone agree on the guidelines. The biggest problem is having HEAD be seriously broken when the expectation is that it will be mostly functional. As long as expectations and reality match, we'll be ok.

Collapse
Posted by Stan Kaufman on
Maybe I'm just thicker than a whale omelet, but I remain puzzled by why/when development goes into HEAD vs a branch. For instance, while working now with file-storage, I see that the version in HEAD is 5.1.0a17 while the version in oacs-5-2 is 5.2.2. Clearly work is going on in the branch that may or may not end up in HEAD, instead of vice versa. These aren't just bug fixes; these are enhancements or otherwise nontrivial developments.

This is unlike what happens in other settings -- eg Debian. There the versions of packages in "unstable" are always later than those in "testing", which are later than those in "stable". Seems to me that HEAD should be "unstable", the tip of oacs-5-2 should be "testing" and then a named release would be "stable".

Package developers with Debian do all their work in unstable and then push their work down into the more stable versions once bug fixes and dependencies, etc, are settled. WIth OpenACS the development occurs lower down (because that's what can be installed and run) and then eventually (hopefully) gets pushed up to HEAD. Is this the sensible way to do it?

Collapse
Posted by Janine Ohmer on
If I was the Queen, it would work like this:

Ongoing development would be committed to private repositories. Only when the work neared completion would it go into HEAD, which would, as you said, be considered unstable. Ongoing bugfixes and small changes would be made directly in HEAD.

When it was time for a release a new branch would be opened up, and last minute fixes would be done there. After the release was done, the branch would be merged back into HEAD and there would be no other branches until the next release.

However, I'm not the Queen and we have an even more complicated setup than most projects because core and packages are released separately, so the above probably wouldn't work.

I have never been exactly sure what the current process is; I don't commit all that often so when I do need to I just find out from Dave where it should go. That way I won't screw it up.

Collapse
Posted by Stan Kaufman on
I have never been exactly sure what the current process is; I don't commit all that often...

Seems to me that that is the undesirable result of all this: multiple people solve the same problems on their own without getting the solutions back to the common code base, so we all duplicate each others' work and still wind up with unfixed problems.

Collapse
Posted by Janine Ohmer on
While I agree with Stan's point, I should clarify that the cause and effect in my case are not as they appear. I don't commit much anymore because the sites I work on have not released their code, and because some of what I do is too specialized to be of use to anyone else. So I am unfamiliar with the current processes because I don't have a need to commit very often, not the other way around.
Collapse
Posted by Malte Sussdorff on
Thanks a lot for your explanation and the connection to Debian. This is exactly the way I see it.

HEAD : Unstable
Tip of released branch: Testing
Released versions: Stable

If we could agree on this denomination, we should then clarify the expectations for each of the trees.

As for where does development happen, here is the take I have been following so far when a project starts:

- Get a checkout from the current release
- Get a checkout from HEAD for the packages that I know will change heavily.
- Add new packages to HEAD
- Merge this together in the development and production server for the client, along with custom packages in our own repository.

And here is my way of committing:

- When I fix change functionality in released packages make sure they install on other environments (read: make a quick cvs update on other servers under my maintenance and do a quick check).
- HEAD package changes are committed if they work on the development server and then moved into production. No further testing on other machines.
- Once in a while I sit down and test the installation (in contrast to only upgrades) of a couple of these packages on a blank core release, using CVS. If it works I release the packages using the compat flag.
- Today I learned, that I should also release the required packages, otherwise the release does not make any sense.

Collapse
Posted by Tracy Adams on
>Janine, are you saying that the community development of the re-write of ecommerce should happen elsewhere?

Or you could develop it in small enough pieces that the basic integrity of the package would be maintained.

It would be quite unsettling if we got a checkin that fundamentally broke the ecommerce package, esp. if the fixes required were more than a few hours of work. It would pretty much render the rest of us helpless or unable to work on the HEAD.

Collapse
Posted by Torben Brosten on
Adding code in small unbroken components is essentially how development is being done. Some packages are new, others are modifications to existing packages where changes are not meant to break anything. No work is being done to the ecommerce package directly.

On another note, were I the queen or a surf for that matter, I would want her surfs to tend to the land (think code) outside the castle yet within the kingdom's domain, lest her people starve trying to fend for themselves.

Collapse
16: Re: Quality of OpenACS (response to 1)
Posted by Nick Carroll on
I agree with Janine. Only working code should be committed to the CVS repository. Even code in HEAD should work, and not be completely broken.

I personally don't think the code reviewer suggestion will work. It works well within an office environment, where you can sit next to a person and review their code as they walk you through it. Not having the developer help walk you through the changes they've made will make it difficult to detect by the reviewer. Code reviews won't work in a distributed environment, unless we all have remote screen sharing software that we can use for this.

Collapse
17: Re: Quality of OpenACS (response to 1)
Posted by Caroline Meeks on
I have a minor social engineering suggestion. I'm not saying this is the solution to all of these problems., but I think its an easy way to increase code reuse.

I think many of us find ourselves doing something interesting and saying to ourself...I should really clean this up, generalize it and commit it to OpenACS. But of course you can't do it right then because you are in the middle of a client project.

Please when you have that thought goto the wiki page per package and write a brief note. Then at least someone has knows and has the option of asking you for a tarball to look at before they do something similar.

Here are two examples I did today:

https://openacs.org/xowiki/pages/en/calendar
https://openacs.org/xowiki/pages/en/photo-album

Please no one ask for details this week, we are in the middle of a client project. We will deliver the project to the client middle of next week then we'll try to get demos up and commit the code.

Al, Don, can you guys put pointers to your portal plans on the portals page?

Thanks
Caroline