Skip to content
This repository has been archived by the owner on Oct 4, 2021. It is now read-only.

[Ide] Consider MSBuild item conditions in Solution pad #9407

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@
// THE SOFTWARE.
using System;
using System.Collections.Generic;
using System.Xml.Linq;
using System.Linq;
using MonoDevelop.Core;

namespace MonoDevelop.Projects.MSBuild
{
class MSBuildPropertyGroupEvaluated: MSBuildNode, IMSBuildPropertyGroupEvaluated, IMSBuildProjectObject
{
protected Dictionary<string,IMSBuildPropertyEvaluated> properties = new Dictionary<string, IMSBuildPropertyEvaluated> (StringComparer.OrdinalIgnoreCase);
protected Dictionary<string, IMSBuildPropertyEvaluated> properties = new Dictionary<string, IMSBuildPropertyEvaluated> (StringComparer.OrdinalIgnoreCase);
MSBuildEngine engine;

internal MSBuildPropertyGroupEvaluated (MSBuildProject parent)
Expand All @@ -42,30 +42,41 @@ internal MSBuildPropertyGroupEvaluated (MSBuildProject parent)

internal void Sync (MSBuildEngine engine, object item, bool clearProperties = true)
{
if (clearProperties)
properties.Clear ();
if (clearProperties) {
lock (properties) {
properties.Clear ();
}
}
this.engine = engine;
foreach (var propName in engine.GetItemMetadataNames (item)) {
var prop = new MSBuildPropertyEvaluated (ParentProject, propName, engine.GetItemMetadata (item, propName), engine.GetEvaluatedItemMetadata (item, propName));
properties [propName] = prop;
lock (properties) {
properties [propName] = prop;
}
}
}

public bool HasProperty (string name)
{
return properties.ContainsKey (name);
lock (properties) {
return properties.ContainsKey (name);
}
}

public IMSBuildPropertyEvaluated GetProperty (string name)
{
IMSBuildPropertyEvaluated prop;
properties.TryGetValue (name, out prop);
lock (properties) {
properties.TryGetValue (name, out prop);
}
return prop;
}

internal void SetProperty (string key, IMSBuildPropertyEvaluated value)
{
properties [key] = value;
lock (properties) {
properties [key] = value;
}
}

internal void SetProperties (Dictionary<string,IMSBuildPropertyEvaluated> properties)
Expand All @@ -75,12 +86,16 @@ internal void SetProperties (Dictionary<string,IMSBuildPropertyEvaluated> proper

public IEnumerable<IMSBuildPropertyEvaluated> GetProperties ()
{
return properties.Values;
lock (properties) {
return properties.Values.ToArray ();
}
}

internal bool RemoveProperty (string name)
{
return properties.Remove (name);
lock (properties) {
return properties.Remove (name);
}
}

public string GetValue (string name, string defaultValue = null)
Expand Down Expand Up @@ -152,11 +167,15 @@ public MSBuildEvaluatedPropertyCollection (MSBuildProject parent): base (parent)

internal void SyncCollection (MSBuildEngine e, object project)
{
properties.Clear ();
lock (properties) {
properties.Clear ();
}
foreach (var p in e.GetEvaluatedProperties (project)) {
string name, value, finalValue; bool definedMultipleTimes;
e.GetPropertyInfo (p, out name, out value, out finalValue, out definedMultipleTimes);
properties [name] = new MSBuildPropertyEvaluated (ParentProject, name, value, finalValue, definedMultipleTimes);
lock (properties) {
properties [name] = new MSBuildPropertyEvaluated (ParentProject, name, value, finalValue, definedMultipleTimes);
}
}
}

Expand Down Expand Up @@ -243,7 +262,9 @@ MSBuildPropertyEvaluated AddProperty (string name)
{
var p = new MSBuildPropertyEvaluated (ParentProject, name, null, null);
p.IsNew = true;
properties [name] = p;
lock (properties) {
properties [name] = p;
}
return p;
}

Expand Down Expand Up @@ -280,13 +301,19 @@ void IPropertyGroupListener.PropertyRemoved (MSBuildProperty prop)
// that property group property.
if (ep.IsNew || !prop.IsNew) {
ep.IsNew = false;
properties.Remove (ep.Name);
lock (properties) {
properties.Remove (ep.Name);
}
}
}
}

public IEnumerable<IMSBuildPropertyEvaluated> Properties {
get { return properties.Values; }
get {
lock (properties) {
return properties.Values.ToArray ();
Copy link
Contributor

Choose a reason for hiding this comment

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

:(

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess there's no way to avoid this.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this collection to allow concurrency?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

I'm concerned about doing this change in 8.4, since it might have an important impact in performance.

}
}
}
}

Expand Down
31 changes: 31 additions & 0 deletions main/src/core/MonoDevelop.Core/MonoDevelop.Projects/Project.cs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
using MonoDevelop.Projects;
using System.Threading.Tasks;
using MonoDevelop.Projects.MSBuild;
using MonoDevelop.Projects.MSBuild.Conditions;
using System.Xml;
using MonoDevelop.Core.Instrumentation;
using MonoDevelop.Core.Assemblies;
Expand Down Expand Up @@ -1216,6 +1217,36 @@ public bool IsFileInProject (string fileName)
return files.GetFile (fileName) != null;
}

/// <summary>
/// Return non-hidden files based on the configuration.
/// </summary>
/// <param name="configuration">Configuration.</param>
/// <returns>Files that should be displayed in the Solution window for a project.</returns>
public IEnumerable<ProjectFile> GetVisibleFiles (ConfigurationSelector configuration)
{
MSBuildEvaluationContext ctx = null;

foreach (ProjectFile file in Files) {
if (!file.Visible || file.IsHidden) {
continue;
} else if (string.IsNullOrEmpty (file.Condition)) {
yield return file;
continue;
}

if (ctx == null) {
ctx = new MSBuildEvaluationContext ();
ctx.InitEvaluation (MSBuildProject);
var config = (ProjectConfiguration)GetConfiguration (configuration);
foreach (var prop in config.Properties.GetProperties ())
ctx.SetPropertyValue (prop.Name, prop.Value);
}

if (ConditionParser.ParseAndEvaluate (file.Condition, ctx))
yield return file;
}
}

/// <summary>
/// Gets a list of build actions supported by this project
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,13 +84,9 @@ void GetFolderContent (Project project, string folder, out List<ProjectFile> fil
files = new List<ProjectFile> ();
folders = new List<string> ();

foreach (ProjectFile file in project.Files)
{
foreach (ProjectFile file in project.GetVisibleFiles (IdeApp.Workspace.ActiveConfiguration)) {
string dir;

if (!file.Visible || file.Flags.HasFlag (ProjectItemFlags.Hidden))
continue;

if (file.Subtype != Subtype.Directory) {
// If file depends on something other than a directory, continue
if ((file.DependsOnFile != null && file.DependsOnFile.Subtype != Subtype.Directory) || FileNestingService.HasParent (file))
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
//
// ProjectNodeBuilderTests.cs
//
// Author:
// Matt Ward <[email protected]>
//
// Copyright (c) 2019 Microsoft Corporation
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
// in the Software without restriction, including without limitation the rights
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
// copies of the Software, and to permit persons to whom the Software is
// furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in
// all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.

using System.Linq;
using System.Threading.Tasks;
using MonoDevelop.Core;
using MonoDevelop.Ide.Gui.Pads.ProjectPad;
using MonoDevelop.Projects;
using NUnit.Framework;
using UnitTests;

namespace MonoDevelop.Ide.Projects
{
[TestFixture]
[RequireService (typeof (RootWorkspace))]
class ProjectNodeBuilderTests : IdeTestBase
{
[Test]
public async Task ConditionalFiles ()
{
FilePath solutionFile = Util.GetSampleProject ("ConditionalFiles", "ConditionalFiles.sln");

using (var solution = (Solution)await Services.ProjectService.ReadWorkspaceItem (Util.GetMonitor (), solutionFile)) {
var p = solution.GetAllProjects ().OfType<DotNetProject> ().Single ();

// Debug configuration.
IdeApp.Workspace.ActiveConfigurationId = "Debug";

var treeBuilder = new TestTreeBuilder ();
treeBuilder.ParentDataItem [typeof (Project)] = p;

var nodeBuilder = new ProjectNodeBuilder ();
nodeBuilder.BuildChildNodes (treeBuilder, p);

var debugFiles = treeBuilder.ChildNodes.OfType<ProjectFile> ().ToList ();
var debugFileNames = debugFiles.Select (f => f.FilePath.FileName).ToList ();
Assert.That (debugFileNames, Has.Member ("MyClass.cs"));
Assert.That (debugFileNames, Has.Member ("MyClass-Debug.cs"));
Assert.That (debugFileNames, Has.No.Member ("MyClass-Release.cs"));
Assert.AreEqual (2, debugFiles.Count);

// Release configuration.
IdeApp.Workspace.ActiveConfigurationId = "Release";

treeBuilder.ChildNodes.Clear ();
nodeBuilder.BuildChildNodes (treeBuilder, p);
var releaseFiles = treeBuilder.ChildNodes.OfType<ProjectFile> ().ToList ();
var releaseFileNames = releaseFiles.Select (f => f.FilePath.FileName).ToList ();
Assert.That (releaseFileNames, Has.Member ("MyClass.cs"));
Assert.That (releaseFileNames, Has.Member ("MyClass-Release.cs"));
Assert.That (releaseFileNames, Has.No.Member ("MyClass-Debug.cs"));
Assert.AreEqual (2, releaseFiles.Count);
}
}
}

}
Loading