We have a bit of a code organization issue for plugins in Snapcraft that I’d like to discuss. Built-in plugins are located in snapcraft/plugins/. Each module in there contains a single plugin defined to be a class that inherits from snapcraft.BasePlugin. The assumption that things are organized this way exists throughout the codebase. For example:
The plugin loading code expects the module to be named the same as the plugin specified in the YAML, and loads the first snapcraft.BasePlugin child from that module it sees
snapcraft list-plugins literally prints out the list of modules found in snapcraft/plugins/
This convention encourages each plugin to be self-contained, as it doesn’t define a place to put helper classes, etc. It results in plugins like the Catkin plugin, which is over 1000 lines and contains multiple classes. It’s getting difficult to maintain, and raises the barrier to entry when trying to contribute.
Now we’re starting to discuss adding support for ROS 2, which uses a new build system (ament), but re-uses some of the same components. There is no inherent relationship between catkin and ament, but both use rosdep, for example. I’d like to be able to maintain those helper classes independent of the plugins using them. To that end, I’d like to discuss an acceptable convention for a better-organized plugin, whether it be because the plugin is complex or because components need to be shared and maintained in isolation.
Whatever we decide, it’d be nice to provide the same ability to local plugins as well. That pretty much is the case today as the local plugin path is added to sys.path, we just need to make it more official and cover it with test cases. Here are a few options to kick-off the discussion for the built-in ones:
Continue assuming modules in the root of the plugin path are plugins, and skip over packages. This allows plugins to organize helpers/shared components in topic-specific packages (e.g. snapcraft.plugins.ros would contain Rosdep)
Continue assuming modules in the root of the plugin path are plugins, but create a single special package, perhaps snapcraft.plugins.helpers to contain the various helper classes plugins might need. For example, there could be a snapcraft.plugins.helpers.ros containing Rosdep. This would require skipping over the helpers package for plugin enumeration.
Don’t change anything at all in the plugin parsing, but create a new package elsewhere in the tree for plugins to put this type of code. Perhaps snapcraft.plugin_helpers or some such thing.
The first suggestion, organizing code into topic-specific packages, seems strongly preferable to me because classes can be independent yet right next to where they’re used.
After talking to @sergiusens offline about this, it sounds like we want to go with a slightly modified version of (1). I’m going to try and summarize our path forward (please jump in if I do so incorrectly):
Rather than having plugin-specific packages in snapcraft/plugins/, collect helper classes into categories and create packages for each of them in snapcraft/plugins/.
Since these are helper classes to be shared among internal plugins, the API is not guaranteed to be stable. If they’re being imported from outside of snapcraft (e.g. in a local plugin) a warning should be shown.
A follow-up question I have here: why not just name them with a known private pattern, i.e. prepended with an underscore, and forgo the warning? Developers should know to avoid them.
The various categories of helpers used at least in the Catkin plugin fall into two broad categories: pullers and builders.
Pullers (dealing with discovering dependencies and/or sources and fetching them)
wstool
rosdep
Builders (dealing with building the pulled sources)
compilers
catkin
I’m going to go forward using exactly that taxonomy for refactoring the Catkin plugin. Changing names is an easy matter, please let me know your thoughts here.
So in this categorization, is pip a puller or a builder? I think most fall into this bag so it might just be one category as I trust you gave it some though.
The only constraint is, don’t call it helpers or handlers where we can end up with a handlerhelper
Yeah pip is a little special as it can kinda do both. In the typical sense though, where pypi has wheels, I’d call pip a puller. I’d say its primary purpose is to parse the setup.py/requirements.txt and manage dependencies, which puts it pretty solidly into the puller camp, even though accomplishing that goal might involve building wheels.
I find the name “puller” rather weird… that said I completely agree with the semantic reasoning. Also because pip actually relies on builders - it doesn’t generate any bytecode by itself.
After one last follow-up with @sergiusens, we’ve decided to forgo the warning and create a snapcraft.plugins.internal package that then contains pullers and builders. Then local plugin authors will then know not to use them.
