[Ide] Consider MSBuild item conditions in Solution pad #9407
Conversation
mrward
force-pushed
the
conditional-project-files
branch
from
ee72118
to
aa7148c
Compare
|
@monojenkins backport release-8.4 |
| get { return properties.Values; } | ||
| get { | ||
| lock (properties) { | ||
| return properties.Values.ToArray (); |
There was a problem hiding this comment.
I guess there's no way to avoid this.
There was a problem hiding this comment.
The ToArray part? I did also try ConcurrentDictionary but that broke a test. There is some code that relies on the order of the items in the dictionary which works when using Dictionary. Using the lock was a way to make sure the test did not fail.
There was a problem hiding this comment.
Why do we need this collection to allow concurrency?
There was a problem hiding this comment.
When testing this change I ran into an InvalidOperationException Collection was modified being thrown when getting the properties for a project configuration (ProjectConfiguration.Properties.GetProperties ()).
Adding a new file to the project saves the project and the project configuration properties are modified. Values are added and then removed (if they are to be merged up to the parent group) when OnWriteProjectHeader is called. If that save happens at the same time as another thread accesses the project configuration properties then the InvalidOperationException can occur.
There was a problem hiding this comment.
So this InvalidOperationException is not really related to the changes you are doing, isn't it? If that's the case, it would be better to do them in a separate PR.
There was a problem hiding this comment.
It looks to be related as far as I can see. The change made here, to look at the MSBuild condition, makes use of the project configuration properties on the UI thread and the first time I tested adding a file it triggered the InvalidOperationException resulting in no files being displayed in a folder in the Solution pad window. The comment on the second commit has the exception callstack I ran into - 78345ea
There was a problem hiding this comment.
I'm concerned about doing this change in 8.4, since it might have an important impact in performance.
|
@monojenkins rebase |
Creating an ASP.NET Core project when .NET Core 3.1 SDK was installed
would result in .json files being displayed twice in the Solution pad.
.NET Core 3.1 SDK defines .json files twice.
<Content Include="**\*.json" ... Condition="'$(ExcludeConfigFilesFromBuildOutput)'!='true'" />
<Content Include="**\*.json" ... Condition="'$(ExcludeConfigFilesFromBuildOutput)'=='true'" />
Older .NET Core SDKs did not define the Content items more than once.
The Condition was not considered when showing files in the Solution
pad.
To support conditional files the Solution pad asks the project for
its visible files. The project uses the MSBuildEvaluationContext
to evaluate the condition to see if the file is visible or not.
Note that conditions on parent ItemGroups are currently not taken into
account. Also that visible files are not updated if the active config
is changed.
Fixes VSTS #1005277 Create ASP.NET Core project, open Properties
folder, there are two launchSettings.json files.
Adding a new .json file to a ASP.NET Core 3.1 project resulted in the files not being displayed in the Properties folder due to the project configuration's Properties being updated on a background thread on saving and the UI thread reading these properties to determine if a file is visible in the Solution pad. Switch to using a concurrent dictionary in MSBuildPropertyGroupEvaluated. System.InvalidOperationException: Collection was modified; enumeration operation may not execute. at System.Collections.Generic.Dictionary`2+ValueCollection+Enumerator[TKey,TValue].MoveNext () [0x00085] in mono-x64/external/corefx/src/Common/src/CoreLib/System/Collections/Generic/Dictionary.cs:1628 at MonoDevelop.Projects.MSBuild.MSBuildEvaluatedPropertyCollection+<MonoDevelop-Projects-IPropertySet-GetProperties>d__11.MoveNext () [0x0008e] in monodevelop/main/src/core/MonoDevelop.Core/MonoDevelop.Projects.MSBuild/MSBuildPropertyGroupEvaluated.cs:207 at MonoDevelop.Projects.Project+<GetVisibleFiles>d__143.MoveNext () [0x0017d] in monodevelop/main/src/core/MonoDevelop.Core/MonoDevelop.Projects/Project.cs:1231 at MonoDevelop.Ide.Gui.Pads.ProjectPad.FolderNodeBuilder.GetFolderContent (MonoDevelop.Projects.Project project, System.String folder, System.Collections.Generic.List`1[MonoDevelop.Projects.ProjectFile]& files, System.Collections.Generic.List`1[System.String]& folders) [0x00149] in monodevelop/main/src/core/MonoDevelop.Ide/MonoDevelop.Ide.Gui.Pads.ProjectPad/FolderNodeBuilder.cs:87 at MonoDevelop.Ide.Gui.Pads.ProjectPad.FolderNodeBuilder.BuildChildNodes (MonoDevelop.Ide.Gui.Components.ITreeBuilder builder, System.Object dataObject) [0x00048] in monodevelop/main/src/core/MonoDevelop.Ide/MonoDevelop.Ide.Gui.Pads.ProjectPad/FolderNodeBuilder.cs:74 at MonoDevelop.Ide.Gui.Components.ExtensibleTreeView+TransactedTreeBuilder.FillNode () [0x00082] in monodevelop/main/src/core/MonoDevelop.Ide/MonoDevelop.Ide.Gui.Components/TransactedTreeBuilder.cs:522
Using a ConcurrentDictionary broke the tests that rely on the items being ordered in the Dictionary. The item definition test ItemDefinitionGroup_AddFilesWithoutMetadata_MetadataUsesEmptyElements failed due to the item definition properties being re-ordered. For now using a lock around the Dictionary instead of a ConcurrentDictionary.
monojenkins
force-pushed
the
conditional-project-files
branch
from
aa7148c
to
f076c06
Compare
Creating an ASP.NET Core project when .NET Core 3.1 SDK was installed
would result in .json files being displayed twice in the Solution pad.
.NET Core 3.1 SDK defines .json files twice.
Older .NET Core SDKs did not define the Content items more than once.
The Condition was not considered when showing files in the Solution
pad.
To support conditional files the Solution pad asks the project for
its visible files. The project uses the MSBuildEvaluationContext
to evaluate the condition to see if the file is visible or not.
Note that conditions on parent ItemGroups are currently not taken into
account. Also that visible files are not updated if the active config
is changed.
Out of scope for this change (to minimize changes for 8.4):
Fixes VSTS #1005277 Create ASP.NET Core project, open Properties
folder, there are two launchSettings.json files.