Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plugin updating does not respect subfolders #52

Closed
oleg-shilo opened this issue Mar 25, 2017 · 28 comments
Closed

Plugin updating does not respect subfolders #52

oleg-shilo opened this issue Mar 25, 2017 · 28 comments

Comments

@oleg-shilo
Copy link

Recently something has changed in the plugin update algorithm (at least in v1.4.3.0). It doesn't longer preserve sub-folders from the plugin packages and collapses all the files in the single root directory.

While the problem itself is simple it has very severe effect on plugins that do rely on file structure. Thus CS-Script plugin was completely broken because of this problem.

Test-case

  • Do clean install of CS-Script (C# Intellisense) plugin.
    Ignore prompt to install plugin internal update that tries to provide a work around for this problem.
  • Observe plugin flat file structure:
    image

And this is the actual file structure of the plugin package:
image

@chcg
Copy link
Collaborator

chcg commented Mar 26, 2017

What was the last PM version which you tested successfully?
N++ default is still 1.3.5.

@oleg-shilo
Copy link
Author

For this report I tested it with v1.4.3.0.

BTW I had a lower version just before the test (it also exhibited the problem) and when I started installing the test plugin it advised me to update the Plugin Manager before installing any other plugins. So I did and the latest Plugin Manager (v1.4.3) showed that problematic behavior.

I cannot tell what was the last version of the Plugin Manager that behaved correctly as this problem affects only some of my plugin functionality (a specific runtime mode). Thus I detected it not at the time it has appeared.

I guess it will be difficult for you to trace the change in the source code and you will need to rely on debugging of the latest released version.

@CollinChaffin
Copy link

I'll bump this issue as well because this is a major issue that has been destroying my NPP install particularly the above mentioned CS-Script but also other critical subfolders. Really hard to keep using an editor that literally requires 2 hrs of reinstalling and reconfiguring plugins from scratch and tracing just what files got smashed - all from a broken plugin manager that for some reason worked great until a couple versions ago.

Perhaps you need to roll back the codebase because something is REALLY screwed up when I get THIS popup now (on multiple WIN7x64 PCs running NPP7.3.3x86) every time I try to use NPP and install/uninstall/update ANY plugins despite the plugmgr being up to date (think APPDATA plugin folder vs progfiles is somehow now totally broken so installs to one place and never sees the updated dll because it then looks at the version in the other):

http://i.imgur.com/rKVsG9O.png

@bruderstein
Copy link
Owner

I'll take a look. I don't remember much changing in this area, but something must have.

@bruderstein
Copy link
Owner

APPDATA plugins is a difficult topic, because different people / organisations expect different defaults. (Should a plugin in APPDATA with the same name override a plugin in notepad++\plugins?)

Can you detail your setup

  • where is plugin manager installed?
  • if APPDATA, is there also a plugin manager in the notepad++\plugins directory (this doesn't work well - notepad++ itself doesn't cope with this particularly well I don't think)
  • where are you installing plugins to? (The setting in plugin manager)

@CollinChaffin
Copy link

Actually, herein lies the problem with this development because NPP is what actually has not only created the secondary - user specific version of it's plugins, but also has fully supported this for a very long time. In fact, it's the only supported setup in a shared user environment where plugins cannot (and should not) ever be installed PER MACHINE to progfiles, but instead by simply creating the fully supported XML trigger file named "allowAppDataPlugins.xml" as a blank/zero byte file in the progfiles/npp main directory then NPP switches itself to a fully supported multi-user environment where all plugins and all subfolders normally under progfiles\npp instead get installed to "C:\Users\USERNAME\AppData\Roaming\Notepad++\plugins", and "C:\Users\USERNAME\AppData\Roaming\Notepad++" is where NPP itself (and pluginmgr today and always in the past) has then installed not only itself, but all plugins and subsequent updates. Again - this is fully documented, supported, and has been forever.

It simply appears that new development on both the plugin manager, but also some of these big plugins like CS-Script (I just confirmed that there is also an issue specific to that plugin also just being 100% unaware of this but also the reverse of the above issue here). Meaning, when my subfolder structure is in the new prerelease "fixed" state to avoid this very issue here, that plugin throws exceptions blows up because it cannot find it's own distributed assemblies. I can take a video cap to show that in my testing, only when I literally did the exact opposite of what they even added to their ABOUT box "fix the folder structure" does and by hand moved ALL the files in their CSScriptNpp folder into the Rosylyn subfolder did all the broken intelisense work and low and behold, all the fatal exceptions disappeared.

In a nutshell, there are three major issues here and I just proved in my testing two of them are actually working AGAINST each other so one needs to stop. :) CS-Script 1.7.1 prerelease is broken and because they began jumping through hoops because pluginmgr was dumping it all into a flat folder they also borked something and now the prerelease ONLY works when I do exactly what it was supposed to avoid, which is to manually dump it all into a flat folder eliminate the normal subfolders - which is just totally wrong.

However, all of that they can fix by reverting much (or all) of those recactive changes if/when the plugin manager folks here can address whatever recently has began borking plugin installs folder structures.

Then, totally separate but still impacted is the issue of both plugin manager and CS-script verifying in both code-bases that they are not only aware, but do in fact run in and support the currently fully-supported scenario of a machine implementing the "allowAppDataPlugins.xml" file switch and running NPP in a non-admin, standard shared user configuration where all plugin binaries and data instead reside in and must properly perform all their operations from the users' "AppData\Roaming\Notepad++" location as detailed in the NPP docs here:

http://docs.notepad-plus-plus.org/index.php/Configuration_Files

This, is the cause of the 3rd major issue - not only operating in the "other" fully supported location everyone has forgotten about (or never known was supported), but also all the little other nuances like once pluginmgr is installed there, it then looks in the old location, sees an "old" outdated ABANDONED copy of the binaries from before NPP executed that first time with the XML flag and began using the APPDATA, which now causes an outdated upgrade prompt at every launch, every other plugin install (despite them apparently actually installing properly to appdata) - I'd say it's 85% okay running in APPDATA but the 15% is causing a lot of annoying little issues. So that's #3 and whether it could also be impacting the first two for those of us running in that config, still remains to be seen and confirmed but I suspect once #1 and #2 are addressed, most of #3's annoyances will still remain. :(

Do note, however, that the section in the above doc detailing "allowAppDataPlugins.xml" behavior is not completely accurate and in my experience, it does NOT in fact load BOTH the users' appdata plugins and "local" (normal progfiles) but instead one or the other when you enable to use the APPDATA location using the xml - that is what you get and ANYTHING installed subsequently in program files is actually totally ignored - which I believe to be the root cause of many issues exactly like this one we are commenting in, going back years to others like this (2012):

https://superuser.com/questions/379339/notepad-plugin-manager-shows-no-available-plugins/1145634

and this one from 2015:

https://superuser.com/questions/945096/why-am-i-unable-to-install-the-nppexec-plugin-for-notepad

There are many and all of them wind up mentioning this XML flag file and the lack of understanding in many cases of how it affects NPP's operation, particularly with plugins.

IMO, the appdata for plugins per user has always made sense, and also removes all requirements for the user to be a local admin to install plugins, so I've always thought the flag file should be reversed years ago and APPDATA should be the default unless it's overridden for some reason by some single admin user who knows he/she will be the only one using their machine to which they know they are a full admin, etc......but that's just me. Note after all these years, that file/setting isn't even one you can set during an fully silent automated install, nor later via any group policy...you literally have to open an editor or copy con the blank file by hand if you forget to check the one checkbox which is the only opportunity during a fully interactive setup in which case the nullsoft installer is nice enough to create the blank XML for you.

@bruderstein
Copy link
Owner

I've quickly read through this - barely have time to fix this, let alone read through all this.

I know how the AppData thing works, thanks, I wrote it, at least the PM support for it. And it's not been there "forever", since about 5.something.

Installing plugins to AppData is something some organisations actively don't want to allow, which is why it's a explicit opt in.

So the TL;DR is:

  1. Recursive copies seem to lose the folder structure
  2. If a plugin is installed in both AppData and the default location, then Plugin Manager picks the wrong one (or at least, possibly different from the one N++ picks).

PS. This is a great article http://www.catb.org/esr/faqs/smart-questions.html

@CollinChaffin
Copy link

LOL if you wrote it then I guess you're right I probably don't need to take all the time to try to help and you already know it's broken and how to fix it - I just did not get that out of the prior comments above. Sorry but after being in IT/DEV approaching my 30yr mark myself, I guess I like it when folks give me more info rather than less and from reading above the info until mine didn't offer much nor did your comments indicate you really understood the issues plaguing what you've written. I know I certainly am not above someone having to point me to errors I may have made in my code over the years, but everyone is different.

Yes if you want to keep it short:

  1. Correctly detect both self and child plugin versions (and install location) for proper updating
  2. Honor plugins' required folder structures and copy/unarchive their files properly

Now since we're joking and giving each other a hard time, I'd say perhaps review regression and changelog since your code a few months ago was still handling both of these things properly. And if you really want to razz and break this down, #1 is 50% of what any basic plugin mgr needs to perform, and #2 is the other 50%, and in this case both #1 and #2 are broken hence why I was trying to help you but frankly between my career and being a full-time single dad of two young babies you're right I probably don't need to spend so much time away from them trying to write up something in such detail to help you pinpoint why such a basic plugin manager seems to be having such a hard time unzipping and dumping zip file IO streams to disk. and locating it's own plugin files and simply reading their internal version numbers. :)

@oleg-shilo
Copy link
Author

Thank you Collin for such comprehensive investigation.

CS-Script 1.7.1 prerelease is broken and because they began jumping through hoops because pluginmgr...

Of course CS-Script needs to stop all these attempts to fix the PM mess.

Don't get me wrong, from the start it was clear that "fix the folder structure" feature is a desperate emergency feature that would only exist until PM is fixed. I had to resort to these unorthodox measures because my previous experience with PM support requests made me believe that the problem is here to stay for quite some time. Sorry Dave but it took 13 days for you to at least start really talking about the problem, which has such devastating effect on other plugins. I am sure you have your reasons...

Anyway, I am happy to remove "fix the folder structure" as it seems like now the problem has got a proper attention :)

I will roll back the pre-release and push the update within a few days. Looking forward the proper PM fix.

@bruderstein
Copy link
Owner

I apologise if my answer came across as aggressive - I appreciate the effort in reporting and detailing this issue, and I'll look into the issue with the file structure.

The issue with the appData plugins I'll look into separately - I think there's some agreement needed on what should happen when a plugin is in both places, and I suspect plugin manager is doing something different than Notepad++.

There's sadly a third issue with gpup.exe for non-admin installs, in that it assumes it needs admin to copy files, which of course isn't the case for appData installs, but is also not the case in a number of other situations (the portable install, for instance). This is actually a harder problem to solve, so will probably take longer before we have a proper fix in place.

Thanks again,
Dave

@oleg-shilo
Copy link
Author

oleg-shilo commented Apr 7, 2017

Dave, I definitely agree that appData and file structure should issues be considered separately. Thank you for looking at it.

Collin, I have created, on your behalf, the issue for CS-Script.Npp faulty file structure recovery algorithm: oleg-shilo/cs-script.npp#1. I want to not only remove the work around but also understand the reasons for its failure. Can you please do a simple experiment for me and post your findings to that new issue page.

When the plugin fails locating Roslyn and disables C# 7 support try to just restart Npp and after restart enable C#7 support again. I expect that there is a chance that it will help but possibly only for the next Npp restart. At least this is what I see on my test system.

@CollinChaffin
Copy link

Hey Oleg!

FYI, I just opened issue #62 and found why there have been lingering Roslyn support issues above and beyond the subfolder flattening that was causing my remaining issues specific to CS-Script in NPP. You won't believe it, but it boils down to a mis-spelling mismatch in the wiki docs, the settings xml, and source code where two different assembly spellings are being referenced. Details here:

oleg-shilo/cs-script#62

Going for lunch with half my hair left. :) As you'll see, I did additional research and testing for you to hopefully make it quick for you to see and address but once you look at the files/references I think you'll agree it is virtually impossible that I am the only one having issues specifically enabling the C#6+ support.

-Collin

@bruderstein
Copy link
Owner

bruderstein commented Apr 7, 2017

I've found the issue with the file structure - it's actually the instructions to gpup.exe that are not reflecting the structure. This actually explains why I didn't catch it on my normal tests (and highlights a hole in my procedure), as I use a testing N++ installation so it doesn't need gpup.exe to copy the files for the installations. Funnily enough it also means a new installation to AppData also works :)

Fix on the way.

@CollinChaffin
Copy link

@bruderstein Nice! Glad you found it! Did you see my other finding regarding the CS-Script syntax issue? We should all know how much that darn single bracket or semicolon ruined entire days......in this case it's a single darn period that, when it finally hit the darn config/source files what a PITA! I don't mind spending a ton of time when it's a really hard debug deserving.....but when it comes down to a friggin single char even if I wasn't the coder I've been there so it still makes me want to scream especially when you can tell it's one that's been lingering for a while just waiting to kick us all in the kneecaps and waiting until another issue arose to really do so!

Good news is, between what you've found on the folder structure, and what I've found in the syntax issues, I'm confident not only will all the NPP plugin functions return to normal, but specifically the CS-Script plugin I know as DEVs we all really love and rely on should now return to being stable once @oleg-shilo can commit those few changes.

@oleg-shilo
Copy link
Author

oleg-shilo commented Apr 7, 2017

@CollinChaffin I am working on it right now :) Will be ready soon.
That misspell on wiki is nasty. Corrected now. I apologize for that. The faulty doco was affecting users who manually edited config file. For Sublime Text 3 I do it automatically. Will need to consider the same for Npp.

As for the file structure, v1.7.0 change was to do the conditional probing for both root and Roslyn directory. Needed because there is no warranty that even the latest plugin is serviced by the latest (fixed) PM. This one I am afraid have to stay.

v1.7.1 has also introduced "fix file structure". This feature will be removed. It's not reliable and only adds the confusion.

And, I will be contacting you soon regarding that reoccurring C# 7 / Roslyn loading failure (not related to this post).

Thank you for such an excellent investigation.

@oleg-shilo
Copy link
Author

The unreliable plugin's work around has been revoked and the new release has been published: https://github.com/oleg-shilo/cs-script.npp/releases/tag/1.7.3. I will start pushing it to the users in a few days after processing initial feedback if any.

But the real fix to the problem will only come when the Plugin Manager is fixed and released.
@bruderstein any at least rough idea when we can expect the fixed Plugin Manager?

@CollinChaffin
Copy link

@oleg-shilo I updated my open issue #62 that I just realized I opened under the main CS-Script and not the NPP plugin repo, so I apologize. The good news is I think regarding this subfolder issue, the prerelease is dealing with that well. However, after now being so frustrated that I went as far as to just temporarily abandon the APPDATA and just move the plugins back to default location - Roslyn support still fails but the behavior has changed with this new release. The hard crashing with exceptions even via cmdprompt is resolved, but despite that it does now maintain the "enabled" status despite it being broken, so as you can see from from the info I added to #62 something is just still not quite right.

I only comment here on this because despite believing the prerelease might in fact now fully address this pluginmgr issue's symptoms until it is fixed, hopefully Dave doesn't mind but it might be good to leave an indication here for anyone having issues fully enabling the plugin's features that began with this pluginmgr issue, to continue to the CS-Script repo issue for updates where we can hopefully verify with a bit more debugging if/what issues remain.

Continued here for Roslyn enabling issues: oleg-shilo/cs-script#62

@oleg-shilo
Copy link
Author

Cool. Thank you.
As far as pure CS-Script related issues I have created the dedicated issue on the project page (oleg-shilo/cs-script.npp#1). @CollinChaffin please visit it as I will post today a few question for you there.

Let's keep this thread clean a let Dave to deal with the problem and us to monitor the progress and detect the definitive moment it's fixed.

@chcg
Copy link
Collaborator

chcg commented Apr 10, 2017

@oleg-shilo v1.4.5 is available for manual download. Installation seems to be ok regarding the folder stucture. Could you please check that also the expected functionality is ok.

Removal just deletes the plugin dll, but the additional folder CSScriptNpp stays in place. Maybe you wan't to extend your configuration at the server for a delete action.

@oleg-shilo
Copy link
Author

Thank you @chcg. Will test it tomorrow.

Maybe you wan't to extend your configuration at the server for a delete action.

Can you please elaborate on this one?

@oleg-shilo
Copy link
Author

OK I have tested the PM and indeed the file structure is created correctly. However, there are a few problems still unresolved:

  • re-instlalling the plugin does not replace the subfolders (e.g. CSScriptNpp ) but merges them.
  • upgrading the plugin does not replace the subfolders (e.g. CSScriptNpp ) but merges them.
  • uninstalling the plugin does no removes subfolders (e.g. CSScriptNpp). Youe it is what you mentioned.
    There is a chance these problem have the same cause.

I think closing this defect was premature. Can you please reopen it? Let's keep it open until the all issues are solved and verified.

@bruderstein
Copy link
Owner

  • Reinstalling a plugin will never replace any subfolders - (if it did it would break many installations!). If you want to do this, add a delete step to your install (it will add additional time, i think, as it will fail if it's not there, but will mean that your folder is definitely removed)
  • Upgrading is supposed to merge folders (otherwise things like installing plugin manager would remove the updater folder, which is shared)
  • to remove the folder on uninstallation, you need to add a step to delete the folder to the uninstall steps (the only default install step is to remove your DLL, nothing else is done automatically)

@chcg
Copy link
Collaborator

chcg commented Apr 10, 2017

@oleg-shilo Example of uninstall see e.g. https://npppm.bruderste.in/plugins/view/compare

@oleg-shilo
Copy link
Author

Excellent. Thank you.
I also noticed that the PM doesn't take the latest copy of the plugin (CSScriptNpp v1.7.5.0) but some previous one. I assume that it is just a matter of the PM getting the latest plugins XML. Am I right?

And one more question. With uninstall steps (Copy, Download, Delete, Run), in what order they are executed?

@CollinChaffin
Copy link

Hey @bruderstein so sorry I've had sick little ones been offline for a bit - It looks like @oleg-shilo has updated on his end and from my testing, 100% of the CS-Script issues I was seeing in conjunction with (and affected by) this original plugmgr subfolders is resolved other than what was noted above with the uninstall/reinstall and I'm a bit out of loop because I just got an OTA update of plugmgr pushed of 1.4.6, yet looking here in repo I actually see 1.4.8 so I wonder why that version didn't push?

Also, to mirror @oleg-shilo's questions above, ultimately I'll pull away because it's his plugin but will say from all my testing, I actually has already come up with his exact list of remaining questions above to ask myself prior to logging back on - so as I pull away and just help test/debug as needed I would just mirror that the results from the upgrade/reinstall/uninstall just need to perhaps be explained and perhaps a bit more focus so that as we all develop NPP plugins we have a really good handle on how the plugmgr will be handling those events.

I don't see any changelog for 1.4.7 or 1.4.8 I can dig through the commits myself but if you happen back into this issue and it's short maybe just update as to whether those last 2 builds were specific to oleg's list of remaining issues above (I'm guessing they might).

I'm happy to confirm that my initial testing looks great for the changes you made related to users running from APPDATA, so awesome work there and thanks!

@oleg-shilo
Copy link
Author

oleg-shilo commented Apr 15, 2017

Hi @CollinChaffin, I have not found a single trace of any documentation on "Run" and "Uninstall" steps. Anything that I found indicated that only Copy and Download steps are supported but nothing else. I am guessing that the new API just not reflected in any documentation. Dave, please correct me if I am wrong.

The answer for the execution order of the uninstall steps was vital for uninstalling CS-Script plugin. This is because Roslyn compiler (VBCSCompiler.exe) has this "brilliant" work around for addressing its enormous loading overhead - it stays in memory after the calling app exits (e.g. NPP). This in turn blocks PM if I configure it to delete sub-folders on uninstall/update.

I wanted to run on uninstall my own process that would kill (VBCSCompiler.exe) and prevent PM blocking. But... I gave up on this. Edited this bit to reflect the Easter spirit. It was too much frustration in it. :)

Collin, the latest CS-Script plugin v1.7.6 has an internal hard coded "kill VBCSCompiler" action on NPP exit. This should tale care about not preventing PM from removing plugin subfolders. The only tricky part is that you need to ensure that you are running the latest PM and it pulls the latest plugin.

@bruderstein
Copy link
Owner

Uninstall steps are run in the order they're listed in, as are the install steps. I'd not considered that this isn't obvious - I'll make sure it's clear in the new admin system.

I'll add docs for the XML for run and delete - that documentation has almost been forgotten about since the admin system came along. For run, just add a run step and fill in the path. For delete, just fill in the path, same as the other steps.

@bruderstein
Copy link
Owner

1.4.8 should go out today - I was hoping to get it out earlier, but I had some issues whilst testing it and I wanted to make sure we weren't introducing any further issues.

Thanks for confirming your issues are fixed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants