Reintegration of PluginManager. #6664
Conversation
ghost
assigned
Codecov Report
@@ Coverage Diff @@
## release/3.2 #6664 +/- ##
===============================================
- Coverage 47.34% 47.32% -0.02%
===============================================
Files 745 745
Lines 54524 54524
Branches 7162 7162
===============================================
- Hits 25815 25806 -9
- Misses 27298 27299 +1
- Partials 1411 1419 +8
|
|
@Johno-ACSLive any thoughts? |
I've had a quick look at this PR, roadmap item #6542 etc. If I understand this correctly, you wish to use a plugin manager for GE and the plugin manager will pull plugins using NuGet. This PR is to remove plugins from the installer / portable and only include the plugin manager. There's also changes between system install for all users vs per user installation of plugins. I saw this piece:
I agree in principle with the above approach. From a distribution mechanism, the installer / portable just needs to know where the plugins go and if they are per user or per machine based once we know the details of how the plugin manager will work etc. What is the decision around plugins, will they be installed for all users, per user or configurable for the installer? For portable, it doesn't make sense to have these options as it should be on a per instance (folder copy) basis. Portables are not expected to be copied to sensitive locations and so should have full access to it's local structure. For the installer this could be different based on the per machine / per user support, but it sounds like we simply need to create the appropriate directory based on a per machine or per user install. Uninstall just needs to know about the root Plugins folder for clean-up, we don't care what is below as we can tell the installer to recursively delete the Plugins directory. Do we want similar capability to VS where users can "Roam" their plugins? I know this question is out of scope of this PR and should be covered under the roadmap. If the Plugin Manager does some smart things, then the installer and portable will need to know how to handle these things. I may have gone into too much detail here 😄 not sure if this helps at all? |
Yes, you captured it nicely.
Not exactly. This PR will add the PluginManager, but not yet remove the plugins. They will be removed later, after they have been moved - one by one - into own repos and made available as NuGet packages.
There is a related discussion in #6415 / #6551. The conclusion was to go for plugins on a per user basis and without roaming.
This is mostly correct. Deleting the plugins directory is sufficient to remove the plugin itself, but not it's settings. |
As well as possibly in settings per repo |
|
OK understand now, I agree that the plugins themselves shouldn't roam (i.e. the binaries should be stored in %LOCALAPPDATA%. I think that roaming which plugin a user has enabled and potentially it's settings could be stored in %APPDATA%. What this means is that the plugin manager can recognise that a plugin has been downloaded and installed on device A and plugin manager can then see that has occured and download the associated plugin on device B (irrespective of GE / Plugin version, it just cares about the plugin being installed). This assumes the version of GE supports the plugin being migrated to PM otherwise it will have no effect. For individual plugins, it would depend on how breaking changes implement their configuration handling, perhaps initially %LOCALAPPDATA% is used and as the plugins mature their configuration management they can support %APPDATA% for roaming. From an installer perspective, cleaning up configuration from main GE settings and having a config file per plugin would depend on the migration strategy to the PM. If we move a plugin from Legacy to PM, something needs to migrate the data. So lets say we upgrade GE to a version where plugin A is removed from GE and is now available from PM. The installer will have the payload removed (the binaries will no longer be included and during upgrade, the installer will remove the previous version which will remove the exisiting binaries) but something in GE / PM will need to detect if the user was using the Plugin, install the package via PM and migrate the settings from GE main config to plugin config. Is this expected to be the installer or GE / PM / Plugin? I believe that GE / PM / Plugin should be responsible to perform this action as I am assuming that a plugin via PM will be self contained (it will contain base config of the plugin). I have looked at the TODO's and there are still some decisions to be made. However, what we can do in the installer is launch an executable (or script) with a flag to indicate a migration needs to be performed. The installer UI can indicate it is migrating plugins while the custom action is occuring. The executable or script (especially a PS script) will also need to be signed. Alternatively, the first load of GE can run through a Plugin migration wizard and we could detect what plugins we think a user is using and have them select which ones to preserve. Again, depends on the migration strategy for the plugins. In the case where GE is being uninstalled and not upgraded, we would remove all directories associated with GE. So if we create two user profile locations e.g. %LOCALAPPDATA%\gitextensions and %APPDATA%\gitextensions then uninstall just deletes those to ensure a clean removal. For upgrades these directories will of course be preserved (the installer just needs to ensure this is the case). Is the PM ready to have its binaries compiled / signed with GE ready for inclusion in the installer? At the moment I see a PS script to download the binaries to sub folder of the Plugins directory during installation. Ideally we would bundle the PM binaries in the installer. |
|
Actually I am thinking of a much simpler approach: leaving it entirely to users what plugins they want to install. Detecting which plugins a user might have used in the past would be very tricky and error-prone. For example, I often look at the results from AppVeyor (mostly the green and red dots in the revision graph) and occasionally open the Statistics. Both are plugins and for both I didn't even change the default settings. I cannot think of a way to reliably detect this kind of usage. Also if you use multiple devices, I would require you install the plugins multiple times. At least for the first release, since I think the majority of our users does not require roaming of plugins (or of the metadata of installed plugins). It would be nice to have, but it is not a mandatory feature.
Yes, almost. The PM code can be found in gitextensions/gitextensions.pluginmanager and the artifact can be downloaded from AppVeyor. It is not yet signed (see gitextensions/gitextensions.pluginmanager#17).
🤔 The script is downloading the PM while assembling the installer, so that it is already bundled with the MSI. There shouldn't be a separate download during installation on the target PC. |
|
The 3.2 release is imminent, so if you want to include the PM in the distribution - it is a good time to bring it back now. |
|
In my option only |
|
Let's aim for the following release then
…On Mon, 8 Jul 2019 at 05:51, Marek Fišera ***@***.***> wrote:
In my option only GitExtensions.Extensibility nuget package is a must
have from our todo list.
All other things are "nice to have".
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#6664?email_source=notifications&email_token=ABBTEXVTCB7P6Z375CKZ2XDP6JCMLA5CNFSM4HPSC2G2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZLR5AY#issuecomment-509025923>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABBTEXX4RBF5XPSKWDPMSXTP6JCMLANCNFSM4HPSC2GQ>
.
|
I deleted the file gitextensions/Setup/PluginManager.wxs Line 8 in 7a994d7 Thus I don't think this ID can change unexpectedly, as long as the path and filenames do not change.
Sure, the file structure of the PM artifact might change in the future, but in this case I think it would be acceptable that the installer needs to be updated, too. Furthermore, I don't think that changes will be frequent. |
|
That makes things a lost simpler, the heat stuff was very painful for me. I'm going to update the PR. |
|
The PM download URL still points to the original repository, as the official repository doesn't any release yet. |
I just created pre-release |
|
I have updated the URL to the gitextensions/gitextensions.pluginmanager repository. |
|
Any idea why the CI build is failing? I didn't look into details yet... Could you please rebase on current |
|
@mast-eu Oh, sorry. I completely missed that. Do it now.. |
|
Release 3.2 is technically ready, I'm happy to wait a little longer, if you think this PR is almost ready. |
RussKie
force-pushed
the
PluginManagerReintegration
branch
from
ce79e19
to
e5bc014
Compare
This comment has been minimized.
This comment has been minimized.
|
I fixed the build (by simply restarting it 😁 ) Seems like the hiccup was on AppVeyors servers, not in our code: https://ci.appveyor.com/project/gitextensions/gitextensions/builds/26923655#L286 |
|
@mast-eu I still don't see the installer. |
|
MSI artifacts are not available on GitExtensions' AppVeyor account, because installers are deliberately not build on pull requests. Therefore, I built it on my personal account. https://ci.appveyor.com/project/mast-eu/gitextensions/builds/26958591/artifacts |
|
You don't see an msi because this is a PR build, add a commit on top of
release/3.2 then you'll see it (you may not have permissions though, Martin
does).
…On Mon, Aug 26, 2019, 10:15 PM Martin Steinisch ***@***.***> wrote:
MSI artifacts are not available on GitExtensions' AppVeyor account,
because installers are deliberately not build on pull requests. Therefore,
I built it on my personal account.
https://ci.appveyor.com/project/mast-eu/gitextensions/builds/26958591/artifacts
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6664?email_source=notifications&email_token=ABBTEXVAO3637NREUEXFVODQGQTVBA5CNFSM4HPSC2G2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5FL43A#issuecomment-524992108>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABBTEXUXJWUIFNJZS2XU2WLQGQTVBANCNFSM4HPSC2GQ>
.
|
This comment has been minimized.
This comment has been minimized.
RussKie
force-pushed
the
release/3.2
branch
2 times, most recently
from
4df1705
to
4999140
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
The 3.2 branch appears to be in a good shape. |
mast-eu
force-pushed
the
PluginManagerReintegration
branch
from
1b82708
to
724014e
Compare
mast-eu
force-pushed
the
PluginManagerReintegration
branch
from
724014e
to
a1b879b
Compare
|
The change I pushed corrects the path where PluginManager is copied to. It was @maraf: I changed your original implementation a bit. Most importantly, I removed |
There was a problem hiding this comment.
I tested the MSI and the portable build. PluginManager now works correctly with both.
MSI artifacts are not directly available on GitExtensions' AppVeyor account, because installers are deliberately not build on pull requests. Therefore, I built it on my personal account (same SHA, no code changes): https://ci.appveyor.com/project/mast-eu/gitextensions/builds/27160008/artifacts
Maybe worth noting that the PM, although technically a plugin, is not de-selectible during installation:
IMHO that's ok, since it should be the only mandatory "plugin".
|
Great news!
Not showing / not allowing to deselect is fully acceptable
…On Wed, Sep 4, 2019, 1:12 AM Martin Steinisch ***@***.***> wrote:
***@***.**** approved this pull request.
I tested the MSI and the portable build. PluginManager now works correctly
with both.
MSI artifacts are not directly available on GitExtensions' AppVeyor
account, because installers are deliberately not build on pull requests.
Therefore, I built it on my personal account (same SHA, no code changes):
https://ci.appveyor.com/project/mast-eu/gitextensions/builds/27160008/artifacts
Maybe worth noting that the PM, although technically a plugin, is not
de-selectible during installation:
[image: image]
<https://user-images.githubusercontent.com/46861028/63694103-f615c100-c815-11e9-94a4-928401a84e14.png>
IMHO that's ok, since it should be the only mandatory "plugin".
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6664?email_source=notifications&email_token=ABBTEXWN5GYK2WXPXPHNPGDQH3OLJA5CNFSM4HPSC2G2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCDRKJFA#pullrequestreview-283288724>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABBTEXSANUDWH3MA3ZWOHPLQH3OLJANCNFSM4HPSC2GQ>
.
|
|
if it builds, merge at will
…On Wed, Sep 4, 2019, 9:51 AM Igor Velikorossov ***@***.***> wrote:
Great news!
Not showing / not allowing to deselect is fully acceptable
On Wed, Sep 4, 2019, 1:12 AM Martin Steinisch ***@***.***>
wrote:
> ***@***.**** approved this pull request.
>
> I tested the MSI and the portable build. PluginManager now works
> correctly with both.
> MSI artifacts are not directly available on GitExtensions' AppVeyor
> account, because installers are deliberately not build on pull requests.
> Therefore, I built it on my personal account (same SHA, no code changes):
> https://ci.appveyor.com/project/mast-eu/gitextensions/builds/27160008/artifacts
>
> Maybe worth noting that the PM, although technically a plugin, is not
> de-selectible during installation:
> [image: image]
> <https://user-images.githubusercontent.com/46861028/63694103-f615c100-c815-11e9-94a4-928401a84e14.png>
>
> IMHO that's ok, since it should be the only mandatory "plugin".
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#6664?email_source=notifications&email_token=ABBTEXWN5GYK2WXPXPHNPGDQH3OLJA5CNFSM4HPSC2G2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCDRKJFA#pullrequestreview-283288724>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABBTEXSANUDWH3MA3ZWOHPLQH3OLJANCNFSM4HPSC2GQ>
> .
>
|
This PR reintegrates PluginManager to the GitExtensions installer and portable zip.
To make PM self updateable I'm moving it to user plugins folder.
Here I'm facing a problem with WIX.
All folders in user profile must be uninstalled, so I have defined their removal. But on of the folders is generated by
<HeatDirectory />build task and has generated id (Setup/Product.wxs#L75). Which may change unexpectedly..The reason why PM WIX component is generated is that its file structure may change in the future, so I tried to make the installer universal.
Does anyone of you guys know how to make
<HeatDirectory />generate uninstall rules?The only solution that came to my mind is replacement of HeatDirectory with powershell script.