Mailing-List: contact general-help@gump.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Gump code and data" <general@gump.apache.org>
Received-SPF: neutral (hermes.apache.org: local policy)
User-Agent: Microsoft-Entourage/11.1.0.040913
Date: Thu, 26 May 2005 12:33:16 +0200
Subject: Re: svn commit: r170825 - in /gump/branches/Gump3
From: Leo Simons <mail@leosimons.com>
To: <general@gump.apache.org>
Message-ID: <BEBB728C.3D48E%mail@leosimons.com>
In-Reply-To: <20050518204302.25711.qmail@minotaur.apache.org>
Mime-version: 1.0
Content-type: text/plain;
	charset="US-ASCII"
Content-transfer-encoding: 7bit

Adam,

Since you asked about some code review...I'll fire off my random thoughts as
I read your commits. I hope they help :)

On 18-05-2005 22:43, "ajack@apache.org" <ajack@apache.org> wrote:
> Author: ajack
> Date: Wed May 18 13:43:01 2005
> New Revision: 170825
> 
> URL: http://svn.apache.org/viewcvs?rev=170825&view=rev
> Log:
> 1) Added to the tsws1.xml workspace:

Which is? That info should not be in the commit but rather in the xml
comments for the workspace.

> 1.1) Moved test (bogus) projects onto module gump-test.
> 1.2) Created gump-test (partly to insert an 'env' call, to debug bootstrap-ant
> problems).
> 1.3) Preparing for adding 'bash gump test'.
> 
> 2) Worked on the AntBuilder. Seems closer to providing the needs of the Ant
> commandline. Still need to add <work> items to the Gump3 model complete the
> CLASSPATH. 

I can see what you worked /on/ from easily enough. But what's the work that
you /did/? There's also a "TODO" in there, which probably should be a "TODO"
in the code or a jira issue.

Is there a jira issue for this? Maybe. If so, you could consider mentioning
it in the commit. Jira will soon have functionality to autolink svn commits
into the tracker. Real cool :)

...
> Modified: gump/branches/Gump3/pygump/python/gump/plugins/builder.py
...
>          assert isinstance(project, Project)
>          self.log.debug("Visit %s looking for %s" % (project,self.cmd_clazz))
>          for command in [command for command in project.commands if
> isinstance(command,self.cmd_clazz)]:
> -            self.log.debug("Perform %s on %s" % (command, project))
> -            self.method(project, command)
> -
> +            try:
> +                self.log.debug("Perform %s on %s" % (command, project))
> +                self.method(project, command)
> +            except Exception:
> +                self.log.exception("Command %s Failed..." % (command))
> +                #TODO Ought we create a BuilderErrorHandler for this?
> +                # Annotate failure
> +                # command.build_log = ':TODO: Serialize Exception trace into
> here?';
> +                command.build_exit_status=1
> +

You're "swallowing an exception" here. Don't ever swallow exceptions. I
think we had a thread on that already recently. Who knows, it might be
changed again already :-)

...
> Modified: gump/branches/Gump3/pygump/python/gump/plugins/java/builder.py
...
> -class BuilderPlugin(AbstractPlugin):
...

Hey, where'd that go? Having trouble reading the diff here! One trick I try
and do before committing is to run 'svn diff' for every file that's changed
(ie after an 'svn status') and make sure I comment on every change that
might deserve a comment.

...
>  class ArtifactPath(object):
> @@ -76,6 +59,13 @@
>          self.parts = []
>          self.state='unknown'
>      
> +    def __nonzero__(self) :
> +        if self.parts: return True
> +        return False
> +    
> +    def __len__(self):
> +        return len(self.parts)
> +    
>      def __add__(self,other):
>          if not isinstance(other,ArtifactPath):
>               other=ArtifactPath("Unknown",other,"Unspecified")
> @@ -84,8 +74,11 @@
>          return self
>      
>      def __str__(self):
> +        return self.join(os.pathsep)
> +    
> +    def join(self,sep):
>          import string
> -        return string.join([ part.path for part in self.parts ], os.pathsep)
> +        return string.join([ part.path for part in self.parts ], sep)
>      
>  class ClasspathPlugin(BuilderPlugin):
>      """Generate build attributes (e.g. CLASSAPATH) for a builder."""
> @@ -95,8 +88,8 @@
>      def _forge_classpaths(self, project, ant):
>  
>          # Stub them out...
> -        ant.classpath=Classpath("Standard")
> -        ant.boot_classpath=Classpath("Boot")
> +        ant.classpath=Classpath('Standard')
> +        ant.boot_classpath=Classpath('Boot')
...

I'm guessing that what makes this so painful (besides the classpath problems
being painful in general) is the use of "intelligent objects". You'll note
that so far the objects I've put into the gump model are very "dumb". I.e.
The "javabeans pattern" is avoided. I don't know who thought of javabeans,
but they were wrong.

I'm guessing that refactoring your code to have totally passive objects
along with "global" logic will simplify it considerably, especially as some
bits of the classpath intelligence need to be shared by the AntPlugin,
ClasspathPlugin, MavenPlugin, ...

>          # Flesh them out...
>          self._calculateClasspaths(project,ant)

For example, I'm guessing that this could be

 gump.model.util.get_classpath(project,command)

...
> +        # Clone the environment, so we can squirt CLASSPATH into it.
> +        self.tmp_env = dict(os.environ)
...

I don't get it. Why is this done here? The problem with the above is that
the plugin suddenly has a "tmp_env" variable, and when and how will that be
cleaned up? I believe you could ultimately even consider

  cmd = Popen(args, shell=False,c wd=projectpath, stdout=PIPE,
stderr=STDOUT, env=self._get_customized_env(ant))

A bit further below, and do more "lazy evaluation". The way I try to get to
that kind of code is by stubbing out the logic first, ie

      def _do_ant(self, project, ant):
          # set up environment
          projectpath = get_project_directory(self.workdir,project)
          classpath = get_ant_classpath(project, ant)
          boot_classpath = get_ant_boot_classpath(project, ant)
          env = get_ant_build_environment(project, ant)

          # set up command line
          args = get_ant_command_line(project, ant, projectpath,
            boot_classpath)

          # execute command
          cmd =Popen(args, shell=False, cwd=projectpath, stdout=PIPE,
stderr=STDOUT, env=env)

          # save results
          ant.build_log = cmd.communicate()[0]
          ant.build_exit_status = cmd.wait()

Not that I did that properly myself here! For example, "save results" could
obviously be

  def save_build_results(model_element, cmd):
    model_element.build_log = cmd.communicate()[0]
    model_element.build_exit_status = cmd.wait()

Which would of course show the eventual need for

  def get_build_log
  def get_build_exit_status

> Added: gump/branches/Gump3/tsws1-settings.sh

I'm guessing "tsws1" is "test workspace 1" or something like that? Are you
setting GUMP_HOSTNAME before running? Does that work well?

Cheers,

LSD


---------------------------------------------------------------------
To unsubscribe, e-mail: general-unsubscribe@gump.apache.org
For additional commands, e-mail: general-help@gump.apache.org