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

Introduce public interfaces for per-project and per-evaluation disk I/O callbacks #6728

Merged
merged 23 commits into from
Oct 21, 2021
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
b6fd14c
Introduce IDirectoryCache & IDirectoryCacheFactory
ladipro Aug 3, 2021
7dae629
PR feedback: Pass Project instead of project path
ladipro Aug 4, 2021
9dcb52a
Don't depend on System.IO.Enumeration
ladipro Aug 5, 2021
521d042
Make EngineFileUtilities static
ladipro Aug 5, 2021
e235b15
Implement DirectoryCacheOverFileSystem
ladipro Aug 9, 2021
62737b6
Basic wiring of EvaluationContext-specific FileMatcher
ladipro Aug 9, 2021
10fff0f
Make FileMatcher consume IDirectoryCache instead of IFileSystem
ladipro Aug 10, 2021
200e037
Revert "Make FileMatcher consume IDirectoryCache instead of IFileSystem"
ladipro Aug 17, 2021
1004b54
Introduce DirectoryCacheFileSystemWrapper
ladipro Aug 17, 2021
faeda3e
Avoid string allocations when calling IsMatch
ladipro Aug 17, 2021
1d0a741
Pass IDirectoryCacheFactory to Project ctor (reverts most changes to …
ladipro Aug 18, 2021
65c4942
Fix bugs
ladipro Aug 18, 2021
e0340b7
Use directory cache in all of evaluation
ladipro Aug 19, 2021
77ed3ff
Add unit test
ladipro Aug 19, 2021
90a6bde
Tweaks and bug fixes
ladipro Aug 19, 2021
73d7636
More internal IFileSystem deprecation and tweaks
ladipro Aug 19, 2021
f916592
Add pattern parameter to EnumerateFiles and EnumerateDirectories
ladipro Sep 1, 2021
43f1323
Revert "More internal IFileSystem deprecation and tweaks"
ladipro Oct 15, 2021
89f7cc6
Redo some of the minor tweaks reverted in previous commit
ladipro Oct 15, 2021
e800fa8
Optimize path manipulations (Microsoft.IO.Redist)
ladipro Oct 15, 2021
231a869
Tweaks and comments
ladipro Oct 15, 2021
2348cd8
Remove unused parameter
ladipro Oct 20, 2021
e30d239
Add and update comments
ladipro Oct 21, 2021
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
14 changes: 14 additions & 0 deletions ref/Microsoft.Build/net/Microsoft.Build.cs
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,7 @@ namespace Microsoft.Build.Definition
public partial class ProjectOptions
{
public ProjectOptions() { }
public Microsoft.Build.FileSystem.IDirectoryCacheFactory DirectoryCacheFactory { get { throw null; } set { } }
public Microsoft.Build.Evaluation.Context.EvaluationContext EvaluationContext { get { throw null; } set { } }
public System.Collections.Generic.IDictionary<string, string> GlobalProperties { get { throw null; } set { } }
public Microsoft.Build.Evaluation.ProjectLoadSettings LoadSettings { get { throw null; } set { } }
Expand Down Expand Up @@ -1504,6 +1505,19 @@ public ProxyTargets(System.Collections.Generic.IReadOnlyDictionary<string, strin
}
namespace Microsoft.Build.FileSystem
{
public delegate bool FindPredicate(ref System.ReadOnlySpan<char> fileName);
public delegate TResult FindTransform<TResult>(ref System.ReadOnlySpan<char> fileName);
public partial interface IDirectoryCache
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to augment MSBuildFileSystemBase with these new methods instead of creating a new interface?. With a new interface, the APIs will get harded to use. Users will have to think whether to implement one or the other, or both, Maybe a new generic MSBuildFileSystemBase that extends MSBuildFileSystemBase.

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed an update where the interface is now provided via Project and not EvaluationContext so having a separate one should be easier to justify. I would argue that having multiple methods providing the same functionality in MSBuildFileSystemBase would also be confusing for users because it wouldn't be clear which one is used when.

{
bool DirectoryExists(string path);
System.Collections.Generic.IEnumerable<TResult> EnumerateDirectories<TResult>(string path, string pattern, Microsoft.Build.FileSystem.FindPredicate predicate, Microsoft.Build.FileSystem.FindTransform<TResult> transform);
System.Collections.Generic.IEnumerable<TResult> EnumerateFiles<TResult>(string path, string pattern, Microsoft.Build.FileSystem.FindPredicate predicate, Microsoft.Build.FileSystem.FindTransform<TResult> transform);
bool FileExists(string path);
}
public partial interface IDirectoryCacheFactory
{
Microsoft.Build.FileSystem.IDirectoryCache GetDirectoryCacheForEvaluation(int evaluationId);
}
public abstract partial class MSBuildFileSystemBase
{
protected MSBuildFileSystemBase() { }
Expand Down
14 changes: 14 additions & 0 deletions ref/Microsoft.Build/netstandard/Microsoft.Build.cs
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,7 @@ namespace Microsoft.Build.Definition
public partial class ProjectOptions
{
public ProjectOptions() { }
public Microsoft.Build.FileSystem.IDirectoryCacheFactory DirectoryCacheFactory { get { throw null; } set { } }
public Microsoft.Build.Evaluation.Context.EvaluationContext EvaluationContext { get { throw null; } set { } }
public System.Collections.Generic.IDictionary<string, string> GlobalProperties { get { throw null; } set { } }
public Microsoft.Build.Evaluation.ProjectLoadSettings LoadSettings { get { throw null; } set { } }
Expand Down Expand Up @@ -1498,6 +1499,19 @@ public ProxyTargets(System.Collections.Generic.IReadOnlyDictionary<string, strin
}
namespace Microsoft.Build.FileSystem
{
public delegate bool FindPredicate(ref System.ReadOnlySpan<char> fileName);
public delegate TResult FindTransform<TResult>(ref System.ReadOnlySpan<char> fileName);
public partial interface IDirectoryCache
{
bool DirectoryExists(string path);
System.Collections.Generic.IEnumerable<TResult> EnumerateDirectories<TResult>(string path, string pattern, Microsoft.Build.FileSystem.FindPredicate predicate, Microsoft.Build.FileSystem.FindTransform<TResult> transform);
System.Collections.Generic.IEnumerable<TResult> EnumerateFiles<TResult>(string path, string pattern, Microsoft.Build.FileSystem.FindPredicate predicate, Microsoft.Build.FileSystem.FindTransform<TResult> transform);
bool FileExists(string path);
}
public partial interface IDirectoryCacheFactory
{
Microsoft.Build.FileSystem.IDirectoryCache GetDirectoryCacheForEvaluation(int evaluationId);
}
public abstract partial class MSBuildFileSystemBase
{
protected MSBuildFileSystemBase() { }
Expand Down
35 changes: 35 additions & 0 deletions src/Build.UnitTests/Definition/ProjectEvaluationContext_Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,41 @@ public void IsolatedContextShouldNotSupportBeingPassedAFileSystem()
Should.Throw<ArgumentException>(() => EvaluationContext.Create(EvaluationContext.SharingPolicy.Isolated, fileSystem));
}

[Fact]
public void EvaluationShouldUseDirectoryCache()
{
var projectFile = _env.CreateFile("1.proj", @"<Project> <ItemGroup Condition=`Exists('1.file')`> <Compile Include='*.cs'/> </ItemGroup> </Project>".Cleanup()).Path;

var projectCollection = _env.CreateProjectCollection().Collection;
var directoryCacheFactory = new Helpers.LoggingDirectoryCacheFactory();

var project = Project.FromFile(
projectFile,
new ProjectOptions
{
ProjectCollection = projectCollection,
DirectoryCacheFactory = directoryCacheFactory,
}
);

directoryCacheFactory.DirectoryCaches.Count.ShouldBe(1);
var directoryCache = directoryCacheFactory.DirectoryCaches[0];

directoryCache.EvaluationId.ShouldBe(project.LastEvaluationId);

directoryCache.ExistenceChecks.OrderBy(kvp => kvp.Key).ShouldBe(
new Dictionary<string, int>
{
{ _env.DefaultTestDirectory.Path, 1},
{ Path.Combine(_env.DefaultTestDirectory.Path, "1.file"), 2 }
}.OrderBy(kvp => kvp.Key));
directoryCache.Enumerations.ShouldBe(
new Dictionary<string, int>
{
{ _env.DefaultTestDirectory.Path, 1 }
});
}

[Theory]
[InlineData(EvaluationContext.SharingPolicy.Shared)]
[InlineData(EvaluationContext.SharingPolicy.Isolated)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@ internal class ItemGroupIntrinsicTask : IntrinsicTask
/// </summary>
private ProjectItemGroupTaskInstance _taskInstance;

private EngineFileUtilities _engineFileUtilities;

/// <summary>
/// Instantiates an ItemGroup task
/// </summary>
Expand All @@ -41,7 +39,6 @@ public ItemGroupIntrinsicTask(ProjectItemGroupTaskInstance taskInstance, TargetL
: base(loggingContext, projectInstance, logTaskInputs)
{
_taskInstance = taskInstance;
_engineFileUtilities = EngineFileUtilities.Default;
}

/// <summary>
Expand Down Expand Up @@ -431,7 +428,7 @@ ISet<string> removeMetadata
// The expression is not of the form "@(X)". Treat as string

// Pass the non wildcard expanded excludes here to fix https://github.com/Microsoft/msbuild/issues/2621
string[] includeSplitFiles = _engineFileUtilities.GetFileListEscaped(
string[] includeSplitFiles = EngineFileUtilities.GetFileListEscaped(
Project.Directory,
includeSplit,
excludes);
Expand All @@ -455,7 +452,7 @@ ISet<string> removeMetadata

foreach (string excludeSplit in excludes)
{
string[] excludeSplitFiles = _engineFileUtilities.GetFileListUnescaped(Project.Directory, excludeSplit);
string[] excludeSplitFiles = EngineFileUtilities.GetFileListUnescaped(Project.Directory, excludeSplit);

foreach (string excludeSplitFile in excludeSplitFiles)
{
Expand Down Expand Up @@ -540,7 +537,7 @@ Expander<ProjectPropertyInstance, ProjectItemInstance> expander
// Don't unescape wildcards just yet - if there were any escaped, the caller wants to treat them
// as literals. Everything else is safe to unescape at this point, since we're only matching
// against the file system.
string[] fileList = _engineFileUtilities.GetFileListEscaped(Project.Directory, piece);
string[] fileList = EngineFileUtilities.GetFileListEscaped(Project.Directory, piece);

foreach (string file in fileList)
{
Expand Down
51 changes: 40 additions & 11 deletions src/Build/Definition/Project.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
using EvaluationItemSpec = Microsoft.Build.Evaluation.ItemSpec<Microsoft.Build.Evaluation.ProjectProperty, Microsoft.Build.Evaluation.ProjectItem>;
using EvaluationItemExpressionFragment = Microsoft.Build.Evaluation.ItemSpec<Microsoft.Build.Evaluation.ProjectProperty, Microsoft.Build.Evaluation.ProjectItem>.ItemExpressionFragment;
using SdkResult = Microsoft.Build.BackEnd.SdkResolution.SdkResult;
using Microsoft.Build.FileSystem;

namespace Microsoft.Build.Evaluation
{
Expand Down Expand Up @@ -68,6 +69,11 @@ public class Project : ILinkableObject
internal ProjectLink Link => implementation;
object ILinkableObject.Link => IsLinked ? Link : null;

/// <summary>
/// Host-provided factory for <see cref="IDirectoryCache"/> interfaces to be used during evaluation.
/// </summary>
private readonly IDirectoryCacheFactory _directoryCacheFactory;

/// <summary>
/// Default project template options (include all features).
/// </summary>
Expand Down Expand Up @@ -250,11 +256,12 @@ public Project(ProjectRootElement xml, IDictionary<string, string> globalPropert
/// <param name="projectCollection">The <see cref="ProjectCollection"/> the project is added to.</param>
/// <param name="loadSettings">The <see cref="ProjectLoadSettings"/> to use for evaluation.</param>
public Project(ProjectRootElement xml, IDictionary<string, string> globalProperties, string toolsVersion, string subToolsetVersion, ProjectCollection projectCollection, ProjectLoadSettings loadSettings)
: this(xml, globalProperties, toolsVersion, subToolsetVersion, projectCollection, loadSettings, null)
: this(xml, globalProperties, toolsVersion, subToolsetVersion, projectCollection, loadSettings, null, null)
{
}

private Project(ProjectRootElement xml, IDictionary<string, string> globalProperties, string toolsVersion, string subToolsetVersion, ProjectCollection projectCollection, ProjectLoadSettings loadSettings, EvaluationContext evaluationContext)
private Project(ProjectRootElement xml, IDictionary<string, string> globalProperties, string toolsVersion, string subToolsetVersion, ProjectCollection projectCollection, ProjectLoadSettings loadSettings,
EvaluationContext evaluationContext, IDirectoryCacheFactory directoryCacheFactory)
{
ErrorUtilities.VerifyThrowArgumentNull(xml, nameof(xml));
ErrorUtilities.VerifyThrowArgumentLengthIfNotNull(toolsVersion, nameof(toolsVersion));
Expand All @@ -264,6 +271,7 @@ private Project(ProjectRootElement xml, IDictionary<string, string> globalProper
implementationInternal = (IProjectLinkInternal)defaultImplementation;
implementation = defaultImplementation;

_directoryCacheFactory = directoryCacheFactory;
defaultImplementation.Initialize(globalProperties, toolsVersion, subToolsetVersion, loadSettings, evaluationContext);
}

Expand Down Expand Up @@ -342,11 +350,12 @@ public Project(XmlReader xmlReader, IDictionary<string, string> globalProperties
/// <param name="projectCollection">The collection with which this project should be associated. May not be null.</param>
/// <param name="loadSettings">The load settings for this project.</param>
public Project(XmlReader xmlReader, IDictionary<string, string> globalProperties, string toolsVersion, string subToolsetVersion, ProjectCollection projectCollection, ProjectLoadSettings loadSettings)
: this(xmlReader, globalProperties, toolsVersion, subToolsetVersion, projectCollection, loadSettings, null)
: this(xmlReader, globalProperties, toolsVersion, subToolsetVersion, projectCollection, loadSettings, null, null)
{
}

private Project(XmlReader xmlReader, IDictionary<string, string> globalProperties, string toolsVersion, string subToolsetVersion, ProjectCollection projectCollection, ProjectLoadSettings loadSettings, EvaluationContext evaluationContext)
private Project(XmlReader xmlReader, IDictionary<string, string> globalProperties, string toolsVersion, string subToolsetVersion, ProjectCollection projectCollection, ProjectLoadSettings loadSettings,
EvaluationContext evaluationContext, IDirectoryCacheFactory directoryCacheFactory)
{
ErrorUtilities.VerifyThrowArgumentNull(xmlReader, nameof(xmlReader));
ErrorUtilities.VerifyThrowArgumentLengthIfNotNull(toolsVersion, nameof(toolsVersion));
Expand All @@ -356,6 +365,7 @@ private Project(XmlReader xmlReader, IDictionary<string, string> globalPropertie
implementationInternal = (IProjectLinkInternal)defaultImplementation;
implementation = defaultImplementation;

_directoryCacheFactory = directoryCacheFactory;
defaultImplementation.Initialize(globalProperties, toolsVersion, subToolsetVersion, loadSettings, evaluationContext);
}

Expand Down Expand Up @@ -436,11 +446,12 @@ public Project(string projectFile, IDictionary<string, string> globalProperties,
/// <param name="projectCollection">The collection with which this project should be associated. May not be null.</param>
/// <param name="loadSettings">The load settings for this project.</param>
public Project(string projectFile, IDictionary<string, string> globalProperties, string toolsVersion, string subToolsetVersion, ProjectCollection projectCollection, ProjectLoadSettings loadSettings)
: this(projectFile, globalProperties, toolsVersion, subToolsetVersion, projectCollection, loadSettings, null)
: this(projectFile, globalProperties, toolsVersion, subToolsetVersion, projectCollection, loadSettings, null, null)
{
}

private Project(string projectFile, IDictionary<string, string> globalProperties, string toolsVersion, string subToolsetVersion, ProjectCollection projectCollection, ProjectLoadSettings loadSettings, EvaluationContext evaluationContext)
private Project(string projectFile, IDictionary<string, string> globalProperties, string toolsVersion, string subToolsetVersion, ProjectCollection projectCollection, ProjectLoadSettings loadSettings,
EvaluationContext evaluationContext, IDirectoryCacheFactory directoryCacheFactory)
{
ErrorUtilities.VerifyThrowArgumentNull(projectFile, nameof(projectFile));
ErrorUtilities.VerifyThrowArgumentLengthIfNotNull(toolsVersion, nameof(toolsVersion));
Expand All @@ -451,6 +462,8 @@ private Project(string projectFile, IDictionary<string, string> globalProperties
implementationInternal = (IProjectLinkInternal)defaultImplementation;
implementation = defaultImplementation;

_directoryCacheFactory = directoryCacheFactory;

// Note: not sure why only this ctor flavor do TryUnloadProject
// seems the XmlReader based one should also clean the same way.
try
Expand Down Expand Up @@ -488,7 +501,8 @@ public static Project FromFile(string file, ProjectOptions options)
options.SubToolsetVersion,
options.ProjectCollection ?? ProjectCollection.GlobalProjectCollection,
options.LoadSettings,
options.EvaluationContext);
options.EvaluationContext,
options.DirectoryCacheFactory);
}

/// <summary>
Expand All @@ -505,7 +519,8 @@ public static Project FromProjectRootElement(ProjectRootElement rootElement, Pro
options.SubToolsetVersion,
options.ProjectCollection ?? ProjectCollection.GlobalProjectCollection,
options.LoadSettings,
options.EvaluationContext);
options.EvaluationContext,
options.DirectoryCacheFactory);
}

/// <summary>
Expand All @@ -522,7 +537,8 @@ public static Project FromXmlReader(XmlReader reader, ProjectOptions options)
options.SubToolsetVersion,
options.ProjectCollection ?? ProjectCollection.GlobalProjectCollection,
options.LoadSettings,
options.EvaluationContext);
options.EvaluationContext,
options.DirectoryCacheFactory);
}

/// <summary>
Expand Down Expand Up @@ -1767,6 +1783,18 @@ internal void VerifyThrowInvalidOperationNotImported(ProjectRootElement otherXml
ErrorUtilities.VerifyThrowInvalidOperation(ReferenceEquals(Xml, otherXml), "OM_CannotModifyEvaluatedObjectInImportedFile", otherXml.Location.File);
}

/// <summary>
/// Returns <see cref="IDirectoryCache"/> as provided by the <see cref="IDirectoryCacheFactory"/> passed when creating the
/// project, specific for a given evaluation ID.
/// </summary>
/// <param name="evaluationId">The evaluation ID for which the cache is requested.</param>
/// <returns>An <see cref="IDirectoryCache"/> implementation, or null if this project has no <see cref="IDirectoryCacheFactory"/>
/// associated with it or it returned null.</returns>
internal IDirectoryCache GetDirectoryCacheForEvaluation(int evaluationId)
{
return _directoryCacheFactory?.GetDirectoryCacheForEvaluation(evaluationId);
}

/// <summary>
/// Internal project evaluation implementation.
/// </summary>
Expand Down Expand Up @@ -3623,6 +3651,7 @@ private void Reevaluate(

Evaluator<ProjectProperty, ProjectItem, ProjectMetadata, ProjectItemDefinition>.Evaluate(
_data,
Owner,
Xml,
loadSettings,
ProjectCollection.MaxNodeCount,
Expand Down Expand Up @@ -4164,15 +4193,15 @@ internal Data(Project project, PropertyDictionary<ProjectPropertyInstance> globa
/// <summary>
/// Prepares the data object for evaluation.
/// </summary>
public void InitializeForEvaluation(IToolsetProvider toolsetProvider, IFileSystem fileSystem)
public void InitializeForEvaluation(IToolsetProvider toolsetProvider, EvaluationContext evaluationContext)
{
DefaultTargets = null;
Properties = new PropertyDictionary<ProjectProperty>();
ConditionedProperties = new Dictionary<string, List<string>>(MSBuildNameIgnoreCaseComparer.Default);
Items = new ItemDictionary<ProjectItem>();
ItemsIgnoringCondition = new ItemDictionary<ProjectItem>();
ItemsByEvaluatedIncludeCache = new MultiDictionary<string, ProjectItem>(StringComparer.OrdinalIgnoreCase);
Expander = new Expander<ProjectProperty, ProjectItem>(Properties, Items, fileSystem);
Expander = new Expander<ProjectProperty, ProjectItem>(Properties, Items, evaluationContext);
ItemDefinitions = new RetrievableEntryHashSet<ProjectItemDefinition>(MSBuildNameIgnoreCaseComparer.Default);
Targets = new RetrievableEntryHashSet<ProjectTargetInstance>(StringComparer.OrdinalIgnoreCase);
ImportClosure = new List<ResolvedImport>();
Expand Down
6 changes: 6 additions & 0 deletions src/Build/Definition/ProjectOptions.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System.Collections.Generic;
using Microsoft.Build.Evaluation;
using Microsoft.Build.Evaluation.Context;
using Microsoft.Build.FileSystem;

namespace Microsoft.Build.Definition
{
Expand Down Expand Up @@ -38,5 +39,10 @@ public class ProjectOptions
/// The <see cref="EvaluationContext"/> to use for evaluation.
/// </summary>
public EvaluationContext EvaluationContext { get; set; }

/// <summary>
/// Provides <see cref="IDirectoryCache"/> to be used for evaluation.
/// </summary>
public IDirectoryCacheFactory DirectoryCacheFactory { get; set; }
}
}
Loading