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, "[EMAIL PROTECTED]" <[EMAIL PROTECTED]> 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: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
