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

Fixes for project file removal support #1694

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

savpek
Copy link
Contributor

@savpek savpek commented Feb 2, 2020

  • Implemented routines to handle removed project files on project system.
  • Fixed issues where removal of files (documents/projecfiles) caused state not to update to clients (for example vscode) about removed diagnostics.

@SirIntruder
Copy link
Contributor

Removal of directories is messed up because:

  1. file watcher in vscode extension has glob patter that handles only files, and not directory changes
  2. there is no code on omnisharp side that would handle "directory was deleted" event, if one was sent - guess a proper way to handle one would be to delete all currently loaded Document/Project objects included in the deleted path.

@savpek
Copy link
Contributor Author

savpek commented Mar 3, 2020

@SirIntruder i noticed 😄 microsoft/vscode#90746

I am planning to prototype make fix in omnisharp-vscode side so it will send deleted files 'properly'. It will be hacky and doesn't support direct filesystem changes (made outside of vscode context) but i feel its correct place to fix it since problem is on vscode side instead of making complex workarounds at backend side which is working as intended at the end. In depth theres event that occurs just before move and remove folder events which can be probably be used to write workaround for those cases. However that exist only on newest vscode apis and current omnisharp-vscode has bit obsolete one so there is some refactoring to do.

If that ends up as dead end next option is to handle removal of directories so that option at omnisharp-roslyn side where directory removal is handled. But it's bit problematic since there really seem not to be centralized place that understands file structure current state so it requires changes all around the code base probably where all parts handle them properly 🤔 Or some kind of cache between that keeps track of structure but it has risks to go to invalid state that introduce very strange bugs.

But yeah i think this problem is one of main reasons why omnisharp requires restarts so often which is quite anonying so even less perfect solution will be step forward.

@savpek
Copy link
Contributor Author

savpek commented Mar 4, 2020

But when worked around at vscode side then problems like breaking up during switches between branches persist, so it might be better to do in o-roslyn side at the end, lets see 🤔

@dnfclas
Copy link

dnfclas commented Mar 11, 2020

CLA assistant check
All CLA requirements met.

@savpek savpek marked this pull request as ready for review March 30, 2020 04:55
@savpek
Copy link
Contributor Author

savpek commented Mar 30, 2020

Ready for review @david-driscoll @filipw @JoeRobich

Build pipelines had bad day yesterday 🤔

Will figure out folder removals at another PR, its tricky case because of limitations of VScode side. Another follow up is to support solution file updates properly project added, removed etc (which isn't currently supported).

@JoeRobich
Copy link
Member

@savpek I like the sound of this PR and will make time to review it this week.

Will figure out folder removals at another PR, its tricky case because of limitations of VScode side.

Can you walk all the folders and watch each path inidividually.

@savpek
Copy link
Contributor Author

savpek commented Jun 5, 2020

Can you walk all the folders and watch each path inidividually.

Any idea is it possible performance wise? If it doesn't anhilate vscode plugin perf then it could simple and elegant solution for problem 🤔

@savpek savpek changed the title Fixes for file removal support Fixes for project file removal support Jun 5, 2020
});
if (projectFileInfo.RuleSet?.FilePath != null)
{
_fileSystemWatcher.Unwatch(projectFileInfo.RuleSet.FilePath);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this be problematic if multiple projects use the same ruleset file? project.AnalyzerConfigDocuments are another group of files that could be unwatched, but they are likely shared with other projects so it may require reference counting.

@CGNonofr
Copy link

Can I do anything to help getting this PR merged?

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

Successfully merging this pull request may close these issues.

5 participants