Merge lp://qastaging/~ted/snapcraft/ros-catkin-plugin into lp://qastaging/~snappy-dev/snapcraft/core

inline

It's probably not a good idea to use config={} in the arguments of the methods because {} is mutable and it can cause some unexpected behaviors. See more info here: http://effbot.org/zone/default-values.htm
As the article suggest, it's better to use config=None, and then if config is None: config = {}.

Also, your python files are not passing the mccabe test:

./snapcraft/plugins/catkin.py:73:1: C901 'CatkinPlugin.find_package_deps' is too complex (10)
./snapcraft/plugins/catkin.py:128:1: C901 'CatkinPlugin.build' is too complex (13)

You should extract methods for the loops. The more the merrier.

Hey good work on getting everything to this point. Seems like a really useful plugin. I do have some comments though.

I've been giving this a bit more of a look and I don't think we want config={} at all, parts should not have knowledge of config, it is what keeps them independent. We also don't want to have it write a services entry, that should be in the users control.

Last but not least, as elopio mentions, the code needs to be split out a bit.

Also merge lp:~sergiusens/snapcraft/catkin which adds the roscore binaries

On Fri, 2015-10-02 at 22:39 +0000, Sergio Schvezov wrote:

> I've been giving this a bit more of a look and I don't think we want
> config={} at all, parts should not have knowledge of config, it is
> what keeps them independent. We also don't want to have it write a
> services entry, that should be in the users control.

So, I actually think the opposite, we need it more or an actual step for
it.

I think that we do want the plugins to provide smarter generation of the
packages.yaml file, we can argue whether they should do services, but I
think we definitely want them to do frameworks and add capabilities on
individual binaries/services/etc. The case I'd mention is the QML
plugin. For QML to be useful it needs the Mir Framework and every binary
needs to have the mir_client capability. I don't think that we should
require the developer to understand those, just understand that he/she
needs to pull in QML. The QML plugin should add those onto the packages
file where it can.

Perhaps a solution for the services would be to provide a way to insert
a commented block into the packages.yaml file. So the plugin could see
if there was a ROS Master and if not throw some comments in saying "hey,
you might want these keys."

While for ROS I think this isn't as important, but as we start adding
things to the wiki you'd want to add postgres to your snap, not
configure postgres as a server in your snap. It seems that there needs
to be some way to have the service come along with the plugin.

So I think the complexity is taken care off and the other little comments.

I'm going to leave the *attr() ones because they match the other code, if we're going to change those I'm happy with that, but it should be one branch to change them all.

Wait for Sergio's comment/discussion on the config variable and how we want to handle that.

Oh, also note, this push breaks the Erle Spider codebase as the switch to sets exposes some more dependency declaration that is needed in their codebase.

I've done a first pass, the code looks really good, just a couple nits here and there.

One thing we haven't been doing and I started addressing was that all our methods our public and I'd make them private (prefix the method with _), this becomes more important now that we inherit from plugins instead of 'requiring' them.

On Thu, 2015-10-15 at 23:44 +0000, Sergio Schvezov wrote:

> One thing we haven't been doing and I started addressing was that all our methods our public and I'd make them private (prefix the method with _), this becomes more important now that we inherit from plugins instead of 'requiring' them.

Fixed.

> > +binaries:
> > + listener:
> > + exec: opt/ros/indigo/beginner_tutorials/listener
> > + talker:
> > + exec: opt/ros/indigo/beginner_tutorials/talker
> > +
> > +parts:
> > + roscore:
> > + plugin: roscore
> > + catkin-tutorials:
> > + plugin: catkin
> > + source: .
> > + catkin-packages:
> > + - beginner_tutorials
> > +
> > +services:
>
> just a minor thing, but maybe put this upstairs (above) together with binaries

Fixed

> > + rosmaster:
> > + start: bin/roscore-rosmaster-service
> > + description: ROS Master Service
> > + ports:
>
> this can be dropped, I think it is being killed and nothing uses it so it can just go away to not confuse people (ports and everything under it)

Fixed, we should probably mark it deprecated in the schema then, no?

> > + def __init__(self, name, options):
> > + super().__init__(name, options)
> > + self.rosversion = options.rosversion
>
> the base class already saves options as self.options from way back and I guess we never noticed that, but maybe all these 'options' attribs don't need specific assigns.

Fixed.

> > + except lxml.etree.ParseError:
> > + logger.warning("Unable to read packages.xml file for '" + pkg + "'")
>
> use .format instead of + or maybe ('string %r' % pkg)

Fixed.

> > + return
>
> what are the consequences of this, even if handled internally maybe raising a RuntimeError is good here and logging from whoever calls this.

My thoughts here is that we can let the developer decide, if they care
that we can't parse it they can fix the file. If they don't care and
they just want to add the dependencies via stage-packages manually we
should allow that too. I kinda see packages.xml as nicety instead of a
requirement for a build.

> > +
> > + for deptype in ['buildtool_depend', 'build_depend', 'run_depend']:
>
> use () instead of [] here.

Fixed. But I don't understand why that would be better, it seems like a
list would always be less memory?

Great work! Great contribution!