You are viewing a plain text version of this content. The canonical link for it is here.
Posted to general@gump.apache.org by Leo Simons <ma...@leosimons.com> on 2005/05/26 12:33:16 UTC
Re: svn commit: r170825 - in /gump/branches/Gump3
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" <aj...@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
Re: svn commit: r170825 - in /gump/branches/Gump3
Posted by Leo Simons <ma...@leosimons.com>.
On 26-05-2005 17:00, "Adam R. B. Jack" <aj...@apache.org> wrote:
>>> 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.
>>
> It is a hostname I've used for a couple of years. It is just like
> giraffe.xml, I assume, although I suspected that was some new Python module
> (like cheetah ;-) for a while. :-)
Hehehe. Giraffe is my desktop, lsd is my server, paddo is my laptop :-). Is
there a checked-in giraffe.xml? Naughty me!
>> I can see what you worked /on/ from easily enough. But what's the work
> that
>> you /did/?
>
> Sorry, I thoguht that was what the code diff was for, so I summarized.
Heck, most developers I know are /terrible/ with their commit messages. I
was too for the longest time. Read
http://svn.collab.net/viewcvs/svn/trunk/HACKING?view=markup
For some "anal" policies which are a Very Good Idea to try and follow.
> Maybe [classpaths] is something we can discuss on
> irc://irc.freenode.net/#asfgump
Added that to my channel list. We'll see :-)
>>> + self.method(project, command)
>>> + except Exception:
> [...]
>>> + self.log.exception("Command %s Failed..." % (command))
>>> + 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 :-)
>
> I was trying to follow the "protocol" at the highest point I could do it.
> This was the only place that was (1) build related (2) able to catch
> exceptions. If you've pushed this into some algorythm, then great, but I
> guess that assumes that all exceptions thrown from project visits are build
> related, right?
Protocol? Uh. I guess this warrants some more thinking. This is why I love
pair programming :-). The above is in the BuilderPlugin isn't it? We have
this
gump.main
gump.engine.Engine
gump.engine.walker.Walker
gump.engine.algorithm.SomeAlgorithm
gump.engine.ConcretePlugin < BuilderPlugin < AbstractPlugin
I suggest we catch exceptions as follows:
1) ConcretePlugin swallows a very limited number of concrete and typed
exception (ie you write "except IOError:", not "except:"), logging it if
appropriate
2) BuilderPlugin doesn't log nor swallow any exception
3) AbstractPlugin doesn't log nor swallow any exception
4) SomeAlgorithm logs and swallows every exception after which it is
conceivable that continuing the gump run has some value (ie, most of them).
5) Walker doesn't log nor swallow any exception
6) Engine logs every exception and exits
So we have two exception boundaries:
gump.main
gump.engine.Engine <-- catch all -->
gump.engine.walker.Walker
gump.engine.algorithm.SomeAlgorithm <-- catch most -->
gump.engine.ConcretePlugin < BuilderPlugin < AbstractPlugin
>> 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.
>
> Does this hark back to your "small classes, many functions" e-mail?
Maybe :-). It's a hunch.
>>> + # 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
>
> How is this different from DynaGumper holding a log variable, or a db
> variable, or whatever.
Ah, that's the ServiceManager vs. Context debate :-). So many hidden design
practices in my code, its astounding!
The "db" is not a variable, nor is the "log". Both of these are "services"
that "do" stuff. They're not created by the Dynagumper, but created by the
config.py file. They're pretty much designed to be around for the lifetime
of the gump run. "tmp_env" doesn't "do" stuff and is created locally. Hence
it is some kind of "data".
The basic unspoken rule I've been applying is "don't store data on a plugin,
but you can keep around references to services inside them".
> Plugins re-use things over and over again, when not
> store them?
When they create those things themselves.
> Sicne we don't ahve a 'reaper' it could be cleaned up either (1)
> on exit [like most of our stuff] or (2) in _finalize.
For the example of the tmp_env its of course true that it doesn't really
matter all that much where you create it. It's just 2k of text (or whatever)
you create once and is needed for something like a 1/3 of the program
execution time (if the 3 stages each consist of a 1/3 of execution time,
which is of course false, but what the heck).
The reason I trip over it is that applying and keeping very simple rules is
easier to do than making, and more importantly, communicating, the
exceptions for those rules.
> Thanks for taking time to review this. I do appreciate the input/different
> perspective, even if the volume of it is hard to accept at times. ;-)
Hehehe. Welcome to open source :-). Code review is why our software is so
much better than that in the rest of the world. I hope you feel free to do
the same. The open source java community is less accustomed to it than we
should be.
Cheers!
LSD
---------------------------------------------------------------------
To unsubscribe, e-mail: general-unsubscribe@gump.apache.org
For additional commands, e-mail: general-help@gump.apache.org
Re: svn commit: r170825 - in /gump/branches/Gump3
Posted by "Adam R. B. Jack" <aj...@apache.org>.
> > 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.
>
It is a hostname I've used for a couple of years. It is just like
giraffe.xml, I assume, although I suspected that was some new Python module
(like cheetah ;-) for a while. :-)
> > 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/?
Sorry, I thoguht that was what the code diff was for, so I summarized. I
made the Ant commandline/environment match what was needed. I added
CLASSPATH and an environment variable, I added -X bootclasspath/p for boot
classpath, I call java (searching on the $PATH, not fixing it), I
added -buildfile and
The biggest work was to make classpath entries relative to the <home/>
element's nested or parent path, releative to the project directory. This
mean resolving all such entries at time of use, since the model was "pure"
of the root "workdir". Trust me, it wasn't fun, but I'm not 100% certain it
is all wrong.
Maybe this is something we can discuss on irc://irc.freenode.net/#asfgump
some time. I'd love for Stefan to be there, 'cos he knows the metadata (use
cases) the best. Other than that, I'm open to ideas on how to resolve
things.
> > + self.method(project, command)
> > + except Exception:
[...]
> > + self.log.exception("Command %s Failed..." % (command))
> > + 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 :-)
I was trying to follow the "protocol" at the highest point I could do it.
This was the only place that was (1) build related (2) able to catch
exceptions. If you've pushed this into some algorythm, then great, but I
guess that assumes that all exceptions thrown from project visits are build
related, right? That seems off.
> ...
> > Modified: gump/branches/Gump3/pygump/python/gump/plugins/java/builder.py
> ...
> > -class BuilderPlugin(AbstractPlugin):
> ...
>
> Hey, where'd that go?
We had two, with little/no specializations, I returned them one.
> 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.
Does this hark back to your "small classes, many functions" e-mail? If so,
I'll re-read that.
> > + # 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
How is this different from DynaGumper holding a log variable, or a db
variable, or whatever. Plugins re-use things over and over again, when not
store them? Sicne we don't ahve a 'reaper' it could be cleaned up either (1)
on exit [like most of our stuff] or (2) in _finalize.
Thanks for taking time to review this. I do appreciate the input/different
perspective, even if the volume of it is hard to accept at times. ;-)
regards,
Adam
---------------------------------------------------------------------
To unsubscribe, e-mail: general-unsubscribe@gump.apache.org
For additional commands, e-mail: general-help@gump.apache.org