[
Date Prev][
Date Next][
Thread Prev][
Thread Next][
Date Index][
Thread Index]
[
List Home]
|
Re: [stellation-res] Code review process
|
At 11:44 AM 3/26/2003, Mark C. Chu-Carroll wrote:
I'm against this. My reasoning, roughly, is that this won't eliminate
any problems; it will merely make them harder to detect.
The problems with patches come about when the workspaces of the patch
creator, and the workspace of the patch verifier are not in synch. Doing
the patch creation manually by packaging changes files won't eliminate
that error.
If you're using an absolutely clean workspace as a starting point, and
making a single change in that clean workspace, and you generate patches
from the project itself, you're essentially doing exactly what's being
described below - only you're creating the patches by hand, by figuring
out what files you changed.
In theory, this will work for verifying patches as well:
a) create an absolutely clean workspace containing the current CVS head version
b) apply a single patch set
c) test and fix as needed until patch is verified
In practice, this may fail if the CVS head version has changed since the
time the
patch set was created. A bigger problem, for me, has been keeping things
straight
while multiple patches are pending review.
If you make a mistake doing this using patches, the patches will reveal
the mistake. If you make a mistake doing his manually, it will be easy
to miss. You lose the systems ability to help you view what has changed;
and you lose the systems ability to tell you when something is wrong.
If we are going to continue using patch-like approach, I'd also rather use
CVS patches as we have been. Manually juggling multiple pending changes in
my local development has been painful, and I'd expect similar pain to occur
with file deltas.
To be honest, I'm frustrated with the code review process as a whole. I
don't think it's working out.
In theory, it's a great idea. In practice, there just aren't enough
people involved, so patches don't get checked quickly enough, and we
constantly run into problems because we're not in sync, because we went
ahead and started our next task while waiting for the last patch to be
verified.
Yes.
I also think it's worth reassessing what level of process is helpful,
given where we are. The answer may be different for various parts
of Stellation:
* Does the change pose a risk to self-hosting?
* Is the affected component feature-complete for the current target
release?
* Can the change be isolated to a particular platform (e.g. Windows)?
I believe the core components (cli, core, remote) are essentially
feature-complete (at least for self-hosting). Since stability here
affects self-hosting, and all uses of Stellation, changes here probably
require the greatest level of care.
The Eclipse client is not yet feature-complete, and I would argue that
changes here (to scm.ui, scm.model) can be made more freely, until
the feature _are_ complete. Some Eclipse client changes may motivate
changes to the core components, or vice versa (e.g. the recent timestamp
changes); any client-motivated changes to core components should be
made with a more stringent process, per the preceding paragraph.
Changes related to the Windows port may require varying levels of
care:
* If Firebird-related changes don't affect PostgreSQL support or
core repository code, they could be made relatively freely.
* If Windows-related changes can be effectively isolated from the
core components, less care is needed than if such changes
directly impact how core components behave on Linux.
Eclipse uses different jars (selected at runtime) for platform-
specific SWT and resource components; we might consider a similar
approach, if that would facilitate the porting effort.
I'm not proposing that we abandon quality-control efforts, and we should
certainly continue to review each other's code. However, it's pretty clear
that the current approach is slowing us down.
In favor of the patch process, I've liked the way it helps me keep track
of what's going on: I can see things happening in bugzilla, and it's
easy for me to look at an incoming change in the patch viewer and see
exactly what was changed. That's been very helpful to me. But overall,
I think its negatives are outweighing its positives.
We could try a slightly modified approach:
1. Create an absolutely clean workspace and apply your changes to it.
(Only necessary if the change set needs to be isolated from other
changes in the same workspace.)
2. Run the regular test suite (command line) and an Eclipse client smoke
test (if the client was touched).
3. Create a patch set to document the change set, and attach it to the
corresponding Bugzilla item. (patch set should include the updated
ChangeLog)
4. Check in the change set to CVS.
5. Create a version tag for the change set (Bugzilla #)
6. Build a source drop of the local workspace (now more-or-less identical
to the CVS image) and post it to the website downloads area.
Building the source drop has several advantages:
* It's easy to regress to an earlier version if a given change set is bad;
even if the version tag (step 5) was omitted.
* Details of changes can be identified by:
- creating a workspace containing the previous version
- diffing against the head version (Eclipse CVS diff support works ok
for this).
* Developers without CVS access can still get the code.
This is just a quick sketch, but something like this might be worth
considering.
The source-drop.xml file I wrote recently should make it easier to create the
source backups (current date is encoded in the zip filename; it would be
easy to
include user.name or other markers if desirable). I'd be glad to refine the
script (and look into ways of automating the update to the downloads
index.html)
if that would be helpful.
Of course, the real solution will be to restart self-hosting, especially once I
get the Eclipse sync view to work with Stellation....
On Wed, 2003-03-26 at 05:10, Jonathan Gossage wrote:
> There seem to be some problems with the current patch based code review
> process which center on the patches themselves. I would like to propose an
> alternate possibility which would have the side effect of taking us closer
> to the process that we will likely adopt once we start self-hosting.
>
> The proposal relies on the use of multiple workspaces and on the use of
file
> deltas instead of patch files.
>
> It could work like this.
>
> 1. Create a base workspace. This workspace would be kept in sync with the
> repository but would never have local modifications made to it. This
> workspace would be used for quickly creating clones in which actual work
> would be done.
>
> 2. Create a separate workspace for each work element such as a bugzilla fix
> or enhancement.
>
> 2, When ready to submit a patch, create a zip file that contains copies of
> all files changed while developing the patch.
>
> 3. The reviewer creates a new workspace when the patch is received and
> unzips the patch into the workspace. The patch can then be reviewed and
> tested.
>
> 4. Once a patch has been verified it can be checked into CVS by either the
> reviewer or the originator of the patch.
>
> This process should get around the problems of creating patch files that
> cannot be applied.
At 03:35 PM 3/26/2003, Jonathan Gossage wrote:
Then we are going to have to decide how to proceed from here. There are a
couple items in the current pipeline including the Windows port. How would
you like to proceed on this?
I also have several items in the pipeline that I'd like to check in.
If the approach I proposed is agreeable, I can add before-and-after
source drops to the downloads page, and we can take it from there.
(I'm about to check in a slightly-modified version of the source-drop.xml
patch set, since Jonathan has verified it).
- Jim
--------------------------------------------------------------------
Jim Wright, IBM T.J. Watson Research Center
*** The Stellation project: Advanced SCM for Collaboration
*** http://www.eclipse.org/stellation
*** Work Email: jwright@xxxxxxxxxxxxxx ------- Personal Email:
jim.wright@xxxxxxx
