[
Date Prev][
Date Next][
Thread Prev][
Thread Next][
Date Index][
Thread Index]
[
List Home]
|
RE: [stellation-res] Code review process
|
> -----Original Message-----
> From: stellation-res-admin@xxxxxxxxxxxxxxx
> [mailto:stellation-res-admin@xxxxxxxxxxxxxxx]On Behalf Of Jim Wright -
> IBM Research
> Sent: Wednesday, March 26, 2003 5:33 PM
> To: stellation-res@xxxxxxxxxxxxxxx
> Subject: 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.
I agree completely on this one. While I have work in progress in this area I
can see no need for it until after we have got self-hosting operational and
stabilized.
>
> 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.
>
Firebird changes have had no known impact on PostgreSQL support but there
were some changes in core stuff to fix problems.
> 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 think we shook out most Windows specific stuff in earlier passes. The
changes this time were mostly Firebird related. It would require fairly
substantial refactoring to create platform specific Jars and I am not sure
that the platform dependencies that do exist, except possibly for file
attribute handling are such that it makes sense to try and factor them out.
What I have found is that most platform dependencies can be handled by
careful use of standard Java facilities.
>
> 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.
It seems to me that a critical point is that patches must be applied rapidly
or they can go stale. The Windows patches for example were unavoidably
delayed for a week because of conferences. When I created them I had
synchronized with the repository immediately prior to creating the patches,
but it seems that even that may not be good enough. Given that the core
functionality and command line functionality are in good state, I am
wondering whether we should not consider moving to private command line
based self hosting even before we complete the Eclipse client support. The
sooner we start hammering the core support the better from the long term
view.
>
> 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....
>
Is there anything I can take on to help in getting the Eclipse client
support ready? Aside from bug fixes, the Windows port appears to be complete
so I should be able to help if you can identify tasks that can be worked on
independently.
>
>
>
>
> (I'm about to check in a slightly-modified version of the source-drop.xml
> patch set, since Jonathan has verified it).
>
> - Jim
>
>
Regards
Jonathan
