[xen-tools] Re: More changes

[C.J. Adams-Collier]

On 9/5/07, Steve Kemp <steve at steve.org.uk> wrote:
> On Wed Sep 05, 2007 at 07:03:55 -0700, C.J. Adams-Collier wrote:
>
> > I personally find it a useful thing to do.  apt-get install xen-tools
> > is nice, but it would like to be able to use this module from other
> > distributions.
>
>   And that's one of reasons why I'm not keen on the Moose overhead.
>   Right now I use the minimum of modules required...

Yes.  I agree.  Dependencies are often difficult to deal with.
However, based on my review of your code, using Moose could reduce
your codebase size by nearly 40% and will make it much easier to
extend.  The pain of this particular dependency is mitigated by the
fact that Moose is the most popular Perl5 object model.  I expect that
it will be requried by many other tools in the near future.

> > I don't want to cause fragmentation or overhead.  Let me know where
> > the problem is and I'll try to address it.  The new Build.PL and your
> > Makefile are not mutually exclusive.  They should both be able to
> > function separately with no problem.
>
>   Really the problem is that people don't read documentation, so
>  seeing lots of things in the top-level directory will confuse them.
>  (Seriously!)

I see where you're coming from.  However, as a rule, people who read
Perl code expect to see a Makefile.PL or Build.PL, a MANIFEST file and
a META.yml in the root of the distribution.  The same thing is done in
all modern CPAN code.  I argue that providing the standard
distribution layout increases, not decreases readability.

>   Given that I don't believe the general audience  would benefit
>  from more than "make install" I think they clutter things unduly,
>  moving them into ./misc/ for people who wanted them would satisfy
>  me - as I could then pretend it didn't exist.

The general audiance isn't going to be installing from the
distribution.  That's what 'apt-get install xen-tools' and "perl
-MCPAN -e 'install Xen::Tools'" will do.  For the most part, these
targets benefit the author and contributors.  Having a simple system
to verify that the unit tests cover all use cases, that new code meets
the distribution's coding standards, and provides hooks for build
automation tools seems like enough reason to include them.

But as I mentioned before, I am happy to maintain an external patch
that brings the distribution into conformance.

> > Well, Moose provides an object model that is becoming standard as well
> > as some fancy features like an argument validation system, object
> > attribute constructors and class creation helpers.  It reduces the
> > KLOCs substantially and IMHO, makes the code much cleaner.
>
>   I'm not seeing a clear benefit there.  Standard perl OO doesn't
>  seem too bad, given the small number of classes and loose inheritence
>  we'll be having.

Yes.  Standard perl OO is pretty terrible :)  It's a hacky hack with a
bit of hack peppered on top.  Moose does a good job of providing a
standard object model in a way that doesn't push the module author
into some strange contortion.  The perl code continues to be perl.

> > Another benefit is that I expect to write the C binding using the
> > Moose object model.  The Moose and glib object models are similar
> > enough that it should be easy to write a c -> Moose caller library
> > relatively easily.  This would open all classes that inherit from
> > Moose to being called from C.  Python, Perl, C++, C# and basically
> > everything else can call C, which means that they would be able to
> > call Moose object methods without the overhead of process context
> > creation.  Rolling our own object model (or worse, doing it ad hoc)
> > would make it much more difficult to write xen-tools apps in different
> > languages than Perl.
>
>   I guess that is a valid point, but in practise is it a huge problem?
>   With things like swig I think that the problem shouldn't be difficult
>  either way.

Swig makes it possible to call perl from C, but it doesn't make it
easy to define classes in Perl.  Historically, there has been a lot of
hand waving and behind-the-curtain hiding involved with OO perl.
Moose cleans a lot of this up.

>    I was picturing a much simpler system with standard constructors
>  something like this:
>
> =begin doc
>
>   Create a new instance of this object.
>
> =end doc
>
> =cut
>
> sub new
> {
>     my ( $proto, %supplied ) = (@_);
>     my $class = ref($proto) || $proto;
>
>     my $self  = {};
>
>     #
>     #  Allow user supplied values to override our defaults
>     #
>     foreach my $key ( keys %supplied )
>     {
>     $self->{ lc $key } = $supplied{ $key };
>     }
>
>     bless ($self, $class);
>     return $self;
>
> }

Yipe!  Your example has no type checking and provides no
encapsulation.  I prefer the following to declare class attributes and
define accessor / mutators:

=head1 FUNCTIONS

=head2 logpath

 Attribute which indicates the log directory.  Defaults to /var/log/xen-tools

=cut

has 'logpath'  => ( is      => 'ro',
                               isa     => 'Str',
                               default => '/var/log/xen-tools'
                               );

EOF

note that the value of the 'isa' field is a type constraint that can
be defined using Moose::Util::TypeConstraints

$ sudo apt-get install libmoose-perl && perldoc Moose::Util::TypeConstraints

>    I'm assuming that we'll have test cases for all the new classes
>  we're putting in, so we'd easily be able to catch bugs, or problems.

Yep.  We should also use Devel::Cover to ensure that all use cases are
accounted for.

>   I've commited everything you've supplied so far *except* the build
>  stuff.  Tonight I'll add a 'Xen::Tools::Configuration' class, or
>  similar to centralise the use of the configuration file parsing,
>  and I'll do that in the standard way so you can compare and contrast.
>  If you think you could live with the "old" way I'd be much pleased :)

Okay.  I'll review and provide a patch so you can see whether the
convenience, simplicity and maintainability outweigh the cost of
having another external dependency.

> Steve

Cheers,

C.J.

-- 
moo.