Issue 3320 - Plugin registration phase #5776

maraf · 2018-11-14T19:23:31Z

This is WIP on #3320.
I need plugins to be registered even when GitExtensions are started with arguments to start other dialog than browse.

If this approach is ok, I will extend this PR also for other dialogs.

Yet I'm considering moving original plugin registration from FormBrowse into GitUICommands and let only menu items binding there. Thoughts?

codecov · 2018-11-15T04:07:00Z

Codecov Report

Merging #5776 into master will increase coverage by 0.75%.
The diff coverage is 62.5%.

@@            Coverage Diff             @@
##           master    #5776      +/-   ##
==========================================
+ Coverage   43.14%   43.89%   +0.75%     
==========================================
  Files         638      638              
  Lines       48501    48522      +21     
  Branches     6487     6490       +3     
==========================================
+ Hits        20924    21301     +377     
+ Misses      26427    25999     -428     
- Partials     1150     1222      +72

RussKie · 2018-11-15T02:39:33Z

GitUI/CommandsDialogs/FormBrowse.cs

@@ -657,6 +658,8 @@ private void UnregisterPlugins()
            {
                plugin.Unregister(UICommands);
            }
+
+            PluginRegistry.ArePluginsRegistered = false;


At this point the app is closed - FormBrowse is the main form, when it is closed the app is done.

RussKie · 2018-11-15T04:14:20Z

I pushed an alternative proposal, please have a look. The idea is to streamline the API.

@sharwell / @jbialobr could you please cast your eyes on the async changes.

jbialobr · 2018-11-15T08:32:42Z

I think we have to solve first the technical debt we have regarding to plugins. I don't think it is a good idea to scan for plugins each time the commit dialog is created (on its own as a separate process). The scanning is taking too long.
Perhaps we could extend the plugins metadata to let a plugin declare when it should be loaded (define some kind of scopes). We could then store in settings what types of plugins we have installed and where to seek them (in which dlls they are). This would let us load only the plugins we need if not the whole app is created.
Other problem to solve is that an individual instance of plugins should be created for each IGitUICommands instance. We can have multiple instances of IGitUICommands but plugins are registered only for the latest chosen repo. This causes the confusion to which I attribute assigning the introduced ArePluginsRegistered field inside of the InitializeAsync. It does not seem right.

sharwell · 2018-11-15T15:04:29Z

The scanning is taking too long.

vs-mef currently isn't caching the component catalog. Once it is, scanning is just a matter of loading a small file with the pre-scanned results.

RussKie · 2018-11-15T23:35:45Z

@jbialobr thank you for your thoughts.

The first part re: re-scanning - whilst it is certainly an performance issue, I personally wouldn't consider it as a release blocker. We can address it in subsequent maintenance releases, especially if we could enable the vs-mef caching.
The above is my personal view, I'm open to discussion.

The second part is interesting.
In case of the Commit dialog launches as own process - this should not be a problem, as the app is closed when the dialog is.
In event of the dialog being opened from the main form, we are levering the existing plugin bindings, so the behaviour stay as is now.
Am I overlooking something?

vs-mef currently isn't caching the component catalog.

Is it easy to enable?

jbialobr · 2018-11-16T07:52:23Z

I personally wouldn't consider it as a release blocker.

You made me to measure it. Running gitextensions commit takes:
Version 2.51.04: ~3secs
This version: ~6secs

For me it is a substantial decrease in performance (read: a blocker).

Am I overlooking something?

Of course you are not. But the current design is bad. I am against adding more mess here. Spreading plugin registration logic through all the app is a code smell. From one hand we have a global variable PluginsRegistered and a while after we register plugins for the currently being used instance of IGitUICommands inside of a public StartCommitDialog method. We call that method for a submodule from the Commit dialog. Would it be a surprise that we end up with a Commit dialog wired with a plugin that is registered for the superproject? I understand that @maraf is a victim of the existing bad design and may not be eager to clean it up. But as I said, we should clean it up first instead of adding more mess.

maraf · 2018-11-16T08:25:27Z

@jbialobr Is there a way I can help?

jbialobr · 2018-11-16T08:47:05Z

@jbialobr Is there a way I can help?

Sure. Could you try to replace MEF with https://github.com/Microsoft/vs-mef and report us how the load time has changed?

sharwell · 2018-11-16T14:17:27Z

Is it easy to enable?

Yes. The hard part is knowing when to delete the cache and scan again.

maraf · 2018-11-16T14:29:00Z

Ok, I'm going to try and let you know.

sharwell · 2018-11-16T15:51:29Z

@jbialobr Sorry, I didn't realize I never submitted the change to use vs-mef. I'll send a PR.

jbialobr · 2018-11-16T16:32:35Z

@jbialobr Sorry, I didn't realize I never submitted the change to use vs-mef. I'll send a PR.

😄 It happened to me recently as well. We were talking about performance and I realized that I had never merged one optimization into the production. I redirect your apology to @maraf - I hope that you have at least learned something new if you have started working on this.

maraf · 2018-11-16T17:32:50Z

It's ok, I haven't started yet.

sharwell · 2018-11-16T19:03:15Z

📝 I submitted #5788

jbialobr · 2018-11-19T09:56:30Z

Thank you @sharwell. I have merged your PR into this PR and I have noticed ~1 second improvement in the load time. It is still 2 seconds worse than running gitextensions.exe commit in the 2.51.04 version.
I investigated it further and observed that the increase in the load time is caused by a need to load all the additional dlls - containing plugins and their dependencies. When the dlls are loaded it takes about 150 ms to load plugins.

sharwell · 2018-12-21T14:05:10Z

GitUI/Plugin/PluginRegistry.cs

+        /// Initialises all available plugins on the background thread.
+        /// </summary>
+        /// <param name="postInitialiseAsync">A function to execute once plugins are loaded.</param>
+        public static Task InitializeAsync(Func<Task> postInitialiseAsync)


💡 Passing continuations via a callback argument reminds me of the days before async/await. It would be better to reorganize this code to avoid the need for this form.

sharwell · 2018-12-21T14:06:04Z

GitUI/Plugin/PluginRegistry.cs

                }
+
+#pragma warning disable VSTHRD105 // Avoid method overloads that assume TaskScheduler.Current


❗️ This should be fixed rather than suppressed. You can make the containing method asynchronous, and then simply put the flag initialization in a finally block.

sharwell · 2018-12-21T14:07:11Z

GitUI/GitUICommands.cs

                    {
-                        form.ShowDialog(owner);
+                        PluginRegistry.Plugins.ForEach(p => p.Unregister(this));


💡 It seems like PluginRegistry would be responsible for this action.

sharwell · 2018-12-21T14:07:40Z

GitUI/GitUICommands.cs

+                        await PluginRegistry.InitializeAsync(() =>
+                        {
+                            // this will only execute, if start without the main form
+                            PluginRegistry.Plugins.ForEach(p => p.Register(this));


💡 It seems like PluginRegistry would be responsible for this action.

maraf · 2019-01-10T16:47:00Z

I have updated code based on the code review.

About performance, is there a way to cache whether exists plugins interested in commit dialog?

I'm not sure whats wrong with Appveyor, any thoughts?
Could rebase help here?

ghost · 2019-01-10T16:51:52Z

Looks like a rebase on master will be needed, because of the conflicting file.

Conflicting files
GitUI/Plugin/PluginRegistry.cs

…meter.

sharwell · 2019-01-11T11:58:52Z

GitUI/Plugin/PluginRegistry.cs

+                }
+                catch
+                {
+                    // no-op


This try-catch was originally in FormBrowse.cs.
@RussKie moved it here in 7d30a22.

If a plugin fails to load we don't want to crash the app because of it.

sharwell · 2019-01-11T12:02:13Z

GitUI/CommandsDialogs/FormBrowse.cs

-                }
+                PluginRegistry.Initialize();
+                await ThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync();
+                RegisterPlugins();


💭 It seems we have a race condition, where the dialog can be closed before plugin registration completes. What are we expecting to happen in this case?

FormBrowse is the main form, if it closed the process is finished, so it probably doesn't matter at this point...

RussKie · 2019-01-29T13:15:06Z

I'd like to resurrect this PR and move on with it.

@sharwell I see a number of your comments are now outdated, it looks like your feedback has been addressed.
Is there anything else you feel needs to be done before this can be merged?

RussKie · 2019-01-29T13:17:41Z

@maraf in mean time please send a separate PR with your sign off (similar to #6166), that will help reduce contentions with the contributors.txt.
Thank you

vbjay · 2019-01-29T13:30:05Z

So there is going to be an issue depending on where plugins are stored and what is loaded and how. The issue is going to be assembly binding issues. I have been fighting such an issue in the main problem and getting it squared away. Example couple of our plugins used a different package that provided a different version of a tfs library. It was the same file path. That one was easier because that nuget package was unlisted so I found the official package. With a system like this, it is going to be quite easy for a plugin to require newtosoft json 11 or some other dependency. So we need to use app domains or some other means to make sure plugin dependencies don't combat with our dependencies.

Why I am putting this here. The plugin registry might keep track of the needed dependencies or help in this regard.

RussKie · 2019-01-29T13:34:59Z

👍

However what are the chances that a user would need to have all 3 tfs plugins at the same time?

And I fully appreciate that various plugins may use different versions of same assembly (i.e. newtonsoft).

@maraf is it possible to install plugins in own directories to avoid version conflicts?

sharwell · 2019-01-29T13:37:05Z

So there is going to be an issue depending on where plugins are stored and what is loaded and how. The issue is going to be assembly binding issues.

This is a massive understatement. I learned some hard lessons on this in the past. The safest assumption in the current workflow is that all external extensions will require recompilation for every build of Git Extensions they want to work with (i.e. patch updates to the host will break all extensions until they are updated). Plugins that want to support multiple host versions will need one assembly for each supported version.

So we need to use app domains or some other means to make sure plugin dependencies don't combat with our dependencies.

This is going to be difficult, since IIUC it won't be a working solution when we execute Git Extensions as a .NET Core 3.0 app.

RussKie · 2019-01-29T13:41:43Z

So is it similar to the VS extension love-hate relationships - a new version of an extension must be released for a new version of VS?

If so, I am not totally opposed to this model.

RussKie · 2019-01-29T13:42:33Z

Do we have any other alternatives? I'd like to remove plugins from the main codebase.

vbjay · 2019-01-29T13:49:43Z

The tfs was a way to showcase the issue. Since they were different packages, automatic binding didn't help. The same issue could happen. Don't focus on multiple versions of tfs plugins, which is POSSIBLE to do. Focus on the issue I am pointing out.

vbjay · 2019-01-29T13:57:21Z

@drewnoakes @sharwell Since you seem to be in at least the right area and could put forth some feedback in the right spot possibly. Nuget needs a file output verification.

Package X adds xyz.dll
Package Y adds xyz.dll DIFFERENT
These are different packages. Automatic binding doesn't solve the issue and only one xyz.dll can be in output.

maraf · 2019-01-29T16:21:00Z

@RussKie Plugins can totally be installed in its own directories, its minor change in plugin manager.
We have also briefly discussed moving them out of GE installation folder.
About compatibility, in maraf/GitExtensions.PluginManager#50 we have discussed that each version of plugin can be bound to concrete version of GE.

vbjay · 2019-01-29T16:47:00Z

@maraf great. So it would be

Plugins root
|---plugin x folder
    |---plugin dll
    |---plugin dependency...

sharwell · 2019-01-29T19:42:15Z

So is it similar to the VS extension love-hate relationships - a new version of an extension must be released for a new version of VS?

For VS extensions, things don't break as often:

VS doesn't break on minor version updates (e.g. 15.0 → 15.1). Git Extensions will potentially break on every commit/build, so extensions compiled for 3.0.0 would not be compatible with 3.0.1 and vice versa.
VS applies strict versioning policies and leverages binding redirects to minimize the need to recompile code, so assemblies that work in 12.0 often still work in 13.0, 15.0, and 16.0 provided the manifest gets updated. Git Extensions does not provide this ability at all, so different assemblies will be needed for each build.

RussKie · 2019-01-29T21:06:52Z

Would the version issue be (largely) addressed if we do something like the following?

extract and publish a contract (e.g. an assembly with interfaces) as a separate consumable package
make the app dependant on the package
install each package in individual folder

sharwell · 2019-01-29T21:44:30Z

@RussKie Yes, provided a strict versioning policy is applied to the interfaces assembly (no binary breaking changes) and the host application provides a binding redirect for it.

RussKie · 2019-02-09T03:29:02Z

I'm merging this as is, however the discussion above is duly noted and we assume a certain tech debt which needs to be addressed going forward.

Items to work through include the following:

define a public contract for plugins and enforce strict versioning policy
plugins to be installed in own folders to reduce risks of overwriting assemblies used by other plugins

Until then external plugins may have to be recompiled for every new version of GE.

Blocking feedback was addressed

RussKie · 2019-02-10T10:39:32Z

Thank you gentlemen, your work and input are highly appreciated.

RussKie · 2019-02-10T10:58:13Z

@maraf do we need to re-enable the PM bundling?

maraf · 2019-02-10T13:34:15Z

@RussKie I don't think so. This PR was not related to PM, AFAIK. I needed it for one of my plugins.

ghost added the 👓 status: needs review label Nov 14, 2018

ghost assigned RussKie Nov 15, 2018

ghost added status: ready and removed 👓 status: needs review labels Nov 15, 2018

RussKie reviewed Nov 15, 2018

View reviewed changes

RussKie added this to the 3.0.1 milestone Nov 18, 2018

gerhardol modified the milestones: 3.00.01+3.01.00, 3.01.00 Dec 9, 2018

RussKie mentioned this pull request Dec 21, 2018

Update ManagedExtensibility to vs-mef #5788

Merged

sharwell previously requested changes Dec 21, 2018

View reviewed changes

maraf and others added 3 commits January 10, 2019 20:50

WIP - Registering plugin when opening commit dialog from startup para…

3b51b28

…meter.

Tweak API

7d30a22

Refactor PluginRegistry according to the code review.

33d11c7

sharwell reviewed Jan 11, 2019

View reviewed changes

sharwell added the 📜 status: needs cla The author needs to update contributors.txt before reviewing or merging can proceed label Jan 23, 2019

RussKie added the status: more info requested label Jan 29, 2019

mast-eu mentioned this pull request Jan 30, 2019

Remove SVN support #4592

Closed

gerhardol added 🖊️ status: cla signed and removed 📜 status: needs cla The author needs to update contributors.txt before reviewing or merging can proceed labels Jan 30, 2019

RussKie removed the status: more info requested label Feb 9, 2019

RussKie merged commit 1273ff8 into gitextensions:master Feb 10, 2019

ghost removed the status: ready label Feb 10, 2019

maraf mentioned this pull request Mar 28, 2019

How to solve shared files/dependencies across different plugins? maraf/PluginManager#21

Closed

		}

		#pragma warning disable VSTHRD105 // Avoid method overloads that assume TaskScheduler.Current

Issue 3320 - Plugin registration phase #5776

Issue 3320 - Plugin registration phase #5776

Conversation

maraf commented Nov 14, 2018

codecov bot commented Nov 15, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

RussKie commented Nov 15, 2018

jbialobr commented Nov 15, 2018

sharwell commented Nov 15, 2018

RussKie commented Nov 15, 2018

jbialobr commented Nov 16, 2018

maraf commented Nov 16, 2018

jbialobr commented Nov 16, 2018

sharwell commented Nov 16, 2018

maraf commented Nov 16, 2018

sharwell commented Nov 16, 2018

jbialobr commented Nov 16, 2018

maraf commented Nov 16, 2018

sharwell commented Nov 16, 2018

jbialobr commented Nov 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maraf commented Jan 10, 2019 • edited

ghost commented Jan 10, 2019 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussKie Jan 26, 2019 • edited

Choose a reason for hiding this comment

RussKie commented Jan 29, 2019

RussKie commented Jan 29, 2019

vbjay commented Jan 29, 2019

RussKie commented Jan 29, 2019

sharwell commented Jan 29, 2019

RussKie commented Jan 29, 2019 • edited

RussKie commented Jan 29, 2019

vbjay commented Jan 29, 2019

vbjay commented Jan 29, 2019 • edited

maraf commented Jan 29, 2019

vbjay commented Jan 29, 2019

sharwell commented Jan 29, 2019

RussKie commented Jan 29, 2019

sharwell commented Jan 29, 2019

RussKie commented Feb 9, 2019

RussKie commented Feb 10, 2019

RussKie commented Feb 10, 2019

maraf commented Feb 10, 2019

codecov bot commented Nov 15, 2018 •

edited

jbialobr commented Nov 19, 2018 •

edited

maraf commented Jan 10, 2019 •

edited

ghost commented Jan 10, 2019 •

edited by ghost

RussKie Jan 26, 2019 •

edited

RussKie commented Jan 29, 2019 •

edited

vbjay commented Jan 29, 2019 •

edited