gump-general mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Leo Simons <m...@leosimons.com>
Subject Re: svn commit: r170825 - in /gump/branches/Gump3
Date Thu, 26 May 2005 20:25:08 GMT
On 26-05-2005 17:00, "Adam R. B. Jack" <ajack@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


Mime
View raw message