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

Fix for: 3596 --- [.NET Framework Projects] IDE is confused about the last file to compile #3603

Merged
merged 1 commit into from
Sep 19, 2017

Conversation

KevinRansom
Copy link
Member

Okay the fix here is to check to see if the project supports CPS before invoking from Workstation events.

I don't think this is the long term solution ... but right now setupproject is needed to eagerly fetch everything needed to compile the project. I expect that we should evolve to the circumstance where we rely on the events.

Anyway this should solve the problem.

Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

This does not fix #3596 in a hive for me. The supplied repro project exhibits the following:

3596-not-fixed

@KevinRansom
Copy link
Member Author

@cartermp this works for me. So what are the specifics of the repro for the failure. If you are in the office tomorrow can you drop by my office and show me the failure.

Thanks

@majocha
Copy link
Contributor

majocha commented Sep 19, 2017

Works for me, both issues don't show up anymore. Fixes #3545 and #3596. 👍

@cartermp
Copy link
Contributor

@KevinRansom What I saw was from f5 into a new hive off of this branch. Though given that @majocha sees it fixed, I suspect it may be something on my machine.

@@ -273,9 +273,13 @@ type internal FSharpProjectOptionsManager
}

member this.UpdateProjectInfoWithProjectId(projectId:ProjectId, userOpName) =
Copy link
Contributor

@dsyme dsyme Sep 19, 2017

Choose a reason for hiding this comment

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

I wonder if we should systematically clarify between "LegacyProject" and "ModernProject" throughout the naming of method like this, so

UpdateProjectInfoWithProjectId --> UpdateModernProjectInfoWithProjectId 
SetupProject --> SetupLegacyProject

etc. That may help us reason correctly about the two separate paths

@KevinRansom
Copy link
Member Author

@dsyme, I don't really see it as legacy projects and CPS projects. I think the SetupProject path needs to go away ... after all everything is Roslyn really not CPS and MPS. However ... before that happens we need to do some work to ensure that the notifications don't swamp the FCS with invalidates ... which is what happens right now.

@KevinRansom KevinRansom merged commit 194fa62 into dotnet:master Sep 19, 2017
@KevinRansom KevinRansom deleted the Fixregularprojects branch September 19, 2017 17:52
@dsyme
Copy link
Contributor

dsyme commented Sep 19, 2017

@dsyme, I don't really see it as legacy projects and CPS projects. I think the SetupProject path needs to go away ...

That might be true, though the current code definitely has these two modalities, I don't think I can think through it without distinguishing between these paths.

@KevinRansom
Copy link
Member Author

@dsyme

So there aren't really two code paths .... there are just different preparations prior to calling updateprojectinfo.

  • The legacy project stuff goes through SetUpProjects/SyncProjects.
  • The project-system projects just go through OnProjectAdded/OnProjectUpdated events. And for the current project creates a siteprovider wrapper. Of course the project-system projects don't do project-project references yet, and so they may have a more involved path before long.

Perhaps Skype when I get into work, and we can talk it through ... because I honestly am a bit embarrassed how little churn this change involved, considering how long it took and how confused I was.

Kevin

@dsyme
Copy link
Contributor

dsyme commented Sep 20, 2017

@KevinRansom OK, thanks. I'll add some comments next time I visit the code

nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
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.

7 participants