GNOME Bugzilla – Bug 642694
IAge checking
Last modified: 2018-05-22 12:10:31 UTC
Created attachment 181240 [details] [review] Added support for checking IAge to PeasEngine Add IAge checking to the engine
Created attachment 181241 [details] [review] Added support for IAges associated to specific interfaces
Created attachment 181242 [details] [review] Added IAges, for supporting plugins with multiple interfaces
Review of attachment 181240 [details] [review]: Minor comments inline. Fix them and push. ::: libpeas/peas-engine.c @@ +117,3 @@ + if (peas_iage_check (engine->priv->iage, + peas_plugin_info_get_iage (info))) + if (engine->priv->iage == NULL) let's not add so many returns, just do if (!.... @@ +1213,3 @@ + * Sets the engine's IAge to check for before loading plugins. + * + * Note: @iage is not taken by @engine, you still have a ref on it. like for other docs say the method they have to use to free: peas_iage_unref @@ +1229,3 @@ + engine->priv->iage = peas_iage_ref (iage); + + PeasIAge *iage) g_list_next ::: libpeas/peas-iage.c @@ +150,3 @@ + * @n_versions: The number of versions. + * @version: A version. + * @va_args: missing docs @@ +152,3 @@ + * @va_args: + * + * Creates a new #PeasIAge... ... ? @@ +180,3 @@ + * @n_versions: The number of versions. + * @version: A version. + * @Varargs: wrong parameter and missing docs @@ +182,3 @@ + * @Varargs: + * + * Creates a new #PeasIAge... ditto @@ +247,3 @@ + * @version: A version. + * + * Returns if ... better docs here
Thinking about it, I wonder if this is really the way IAge should work. Let's keep a simple use case in sight: Four versions of gedit, all hypothetically using libpeas: 2.30, 3.0, 3.2 and 3.4. We know that gedit 2.30 (let's imagine it's using libpeas) has plugins, but with a totally different set of hooks than the new gedit 3.0. Let's imagine again that gedit 3.2 introduces a new interface TextViewFrobnicator. gedit 3.4 otoh drops support for some API related to GeditApp. Obviously, you will want gedit 2.30 to propose a different IAge than gedit 3.0, but it's much harder to decide what to do about 3.2 and 3.4, because they share a great deal of API and ABI compatibility (especially when it comes to python plugins), so you will basically want to expose the same IAge. But a plugin developped with 3.2 in mind will likely fail in 3.0 *and* in 3.4 if it uses the wrong APIs. I think it would be worth looking at how others have dealt with this issue, most notably the Mozilla crowd. They actually did it the other way than what we are trying to achieve with this patch: each application exposes its own version, and the *plugin* chooses which versions it can run with (See https://developer.mozilla.org/en/Install_Manifests#targetApplication). So basically, if you're running the Foo plugin with gedit 3.2, here is what you'd do: peas_engine_set_iage (engine, VERSION); And in the foo.plugin file, you'd have: IAge: 3.0 to 3.4 It also means you would *not* allow plugins to support unknown versions, and that plugins would have to be updated once a new version is out.
Created attachment 195007 [details] [review] Added version handling This adds PeasPluginVersion which represents a version number and PeasPluginDependency which represents 3 different types of dependencies. The engine now checks that dependencies of plugins match a specific version and requirements can be added to the engine. Also remove "IAge=N" from all .plugin files.
Created attachment 197554 [details] [review] Added version handling v 2 This adds PeasPluginVersion which represents a version number and PeasPluginDependency which represents 3 different types of dependencies. The engine now checks that dependencies of plugins match a specific version and requirements can be added to the engine. Also remove "IAge=N" from all .plugin files.
Review of attachment 197554 [details] [review]: It looks really good. See some comment inline. ::: libpeas/peas-engine.c @@ +312,3 @@ + + engine->priv->requirements = g_hash_table_new_full (g_str_hash, + g_str_equal, wrong indentation? @@ +1363,3 @@ + * Gets the version of the requirement @name. + * + peas_plugin_version_new (version)); afaik there is no need for transfer full for gchar * ::: libpeas/peas-plugin-info-priv.h @@ +47,3 @@ gchar *version; gchar *help_uri; + gchar **requirements; is this really needed? ::: libpeas/peas-plugin-info.c @@ +58,3 @@ * Website=http://live.gnome.org/Libpeas * Help=http://library.gnome.org/devel/libpeas/unstable/ + * Interfaces=libpeas == 1 Requires or interfaces? if so let's change the var names as it would make it easier to understand. ::: libpeas/peas-plugin-info.h @@ +49,3 @@ * A dependency of the plugin was not found. * @PEAS_PLUGIN_INFO_ERROR_DEP_LOADING_FAILED: +<<<<<<< HEAD what about this conflict? @@ +57,3 @@ + * @PEAS_PLUGIN_INFO_ERROR_INVALID_VERSION: + * The plugin is for another version. +>>>>>>> Added version handling here too ::: peas-demo/plugins/helloworld/helloworld.plugin @@ +1,3 @@ [Plugin] Module=helloworld +Interfaces=libpeas == 1 Requires?
As discussed on IRC it would be nice to have the content of the "Interfaces" field above as additional items in the "Depends" field.
Created attachment 197828 [details] [review] Added version handling v3 This fixes everything commented about and it now uses the Depends field. Renames: peas_engine_set_requirement() -> peas_engine_add_prerequisite() peas_engine_get_requirement() -> peas_engine_get_prerequisite() And now the engine uses the term prerequisite instead of requirement. I also fixed a small bug in peas_plugin_info_has_dependency() which was checking the whole string and not the module name, i.e. "libpeas == 1" and not "libpeas".
Review of attachment 197828 [details] [review]: I know I'm annoying, but could you please split this patch in a few parts? - IAge removal in .plugin files - Actual adding of the dependency version stuff - The rest. I'm sure the .plugin files alone are more than half of the touched files... ::: libpeas-gtk/peas-gtk-plugin-manager-view.c @@ +601,3 @@ if (!is_row) return FALSE; + Please avoid whitespace fixes in 4000+ patches touching more than 20 files ::: loaders/c/peas-plugin-loader-c.c @@ +72,3 @@ if (!g_type_module_use (G_TYPE_MODULE (module))) { + g_warning ("Plugin '%s': Could not load module", module_name); This should go in another patch. ::: peas-demo/plugins/gjshello/gjshello.plugin @@ -2,3 @@ Module=gjshello Loader=gjs -IAge=2 IMHO the IAge removal should be in another patch, just because it's a dumb change and make this very patch more difficult to grasp.
Created attachment 200150 [details] [review] Added version handling v4 (In reply to comment #10) > Review of attachment 197828 [details] [review]: > > I know I'm annoying, but could you please split this patch in a few parts? > - IAge removal in .plugin files > - Actual adding of the dependency version stuff > - The rest. > > I'm sure the .plugin files alone are more than half of the touched files... Removed the IAge= from all .plugins in another patch, have no clue as to what "The rest" is referring to. > ::: libpeas-gtk/peas-gtk-plugin-manager-view.c > @@ +601,3 @@ > if (!is_row) > return FALSE; > + > > Please avoid whitespace fixes in 4000+ patches touching more than 20 files Done, but would be nice to finally be allowed in.
Ok, please commit these ones :-)
After talking a bit with Garret on IRC, I'll add some considerations. Keep in mind that I have not really thought about this in depth, so I'll be happy to admit that anything I say below is a bad idea :) 1) I have a bad gut feeling about dropping IAge and using Depends... I know they look and behave in a similar way, but I still think they are different concepts 2) I think we mostly need a way to express two concepts: - plugins saying "I need at least version X.Y" - main app saying "Do not even touch those old plugins, because we rewrote everything" The "depends" thing seem mostly suited for the first case. It can also handle the second case, but that would be left up to specific conventions, like bumping the major number when there is an incompatible change. In other words, should gedit 4 load a plugin that says "depends: gedit >= 3.2" ? It's not something you can say for sure, it depends on what gedit 4 will be... You could hardcode that saying >= 3.2 actually means 3.x with x >= 2 and if gedit 4 does not break the plugin interface it could claim to provide both 3.* and 4.*, but it seems to me that you would be hardcoding many assumptions about the application in libpeas 3) I am not sure taht saying "depends: gedit > 3.2" is a great way to express a specific need of a plugin... if a plugin needs to add an item to the gedit-frobnicator-sidepane, maybe it would be better to have a way for gedit to be able to say "I support the frobnicable interface" and for the plugin to require it... though it soon becomes a slippery slope
At first I would rather have had required to specify the latest supported version of an interface, as firefox does, but it's sort of lying, because either you specify the latest known version and then it's annoying for distributions and such because they need to patch or re-release plugins for each versions, or you'd put a version like 4.0.0 and then you're back to square one. Maybe we could just, when setting requirements, specify "the version when the API was last broken incompatibly", defaulting to NULL for "never have" ?
The logic to decide whether a plugin is compatible with an application could be kept in the application itself. Because apps can have many different versioning schemes. Or have two versions in the *.plugin file that libpeas can check: - API version: incremented on API breaks - App version. libpeas checks that: - the API version is the same - the App version in the *.plugin file is less than or equal to the App version So basically it's similar to the Libtool version.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/libpeas/issues/6.