From 8ebc5caa25ecb7e5fee9d2abc4175abbdc01a8a7 Mon Sep 17 00:00:00 2001 From: Jon Hynes Date: Mon, 12 Jun 2023 08:53:46 -0400 Subject: [PATCH 1/4] Backward-compatibility with KeyValuePair metadata items (#8870) Backport #8870 to vs17.7. --- .../BackEnd/TaskExecutionHost/TaskExecutionHost.cs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs b/src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs index 705ca12979..b2eee81b3f 100644 --- a/src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs +++ b/src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs @@ -1395,9 +1395,15 @@ namespace Microsoft.Build.BackEnd // Setting an item spec expects the escaped value, as does setting metadata. newItem = new ProjectItemInstance(_projectInstance, outputTargetName, EscapingUtilities.Escape(output.ItemSpec), parameterLocationEscaped); - newItem.SetMetadataOnTaskOutput(output.CloneCustomMetadata() - .Cast() - .Select(x => new KeyValuePair((string)x.Key, EscapingUtilities.Escape((string)x.Value)))); + newItem.SetMetadataOnTaskOutput(EnumerateMetadata(output.CloneCustomMetadata())); + + static IEnumerable> EnumerateMetadata(IDictionary customMetadata) + { + foreach (DictionaryEntry de in customMetadata) + { + yield return new KeyValuePair((string)de.Key, EscapingUtilities.Escape((string)de.Value)); + } + } } } From 4cf11b497f1b2e3a4c504e8b525dbb5529764941 Mon Sep 17 00:00:00 2001 From: Rainer Sigwald Date: Wed, 12 Jul 2023 13:39:43 -0500 Subject: [PATCH 2/4] Fix binlog OOM embedding files Fixes #8595 by storing the embedded-file zip in a temporary directory (instead of memory or binlog target directory) to avoid problems with file watchers. --- .../Logging/BinaryLogger/BinaryLogger.cs | 18 ++++- .../BinaryLogger/BuildEventArgsWriter.cs | 16 ++++ .../BinaryLogger/ProjectImportsCollector.cs | 76 +++++++++---------- src/MSBuild/XMake.cs | 2 +- 4 files changed, 68 insertions(+), 44 deletions(-) diff --git a/src/Build/Logging/BinaryLogger/BinaryLogger.cs b/src/Build/Logging/BinaryLogger/BinaryLogger.cs index 29b259d2be..28a16df7c9 100644 --- a/src/Build/Logging/BinaryLogger/BinaryLogger.cs +++ b/src/Build/Logging/BinaryLogger/BinaryLogger.cs @@ -6,6 +6,7 @@ using System.IO; using System.IO.Compression; using Microsoft.Build.Framework; using Microsoft.Build.Shared; +using Microsoft.Build.Shared.FileSystem; #nullable disable @@ -226,12 +227,25 @@ namespace Microsoft.Build.Logging if (projectImportsCollector != null) { + projectImportsCollector.Close(); + if (CollectProjectImports == ProjectImportsCollectionMode.Embed) { - eventArgsWriter.WriteBlob(BinaryLogRecordKind.ProjectImportArchive, projectImportsCollector.GetAllBytes()); + var archiveFilePath = projectImportsCollector.ArchiveFilePath; + + // It is possible that the archive couldn't be created for some reason. + // Only embed it if it actually exists. + if (FileSystems.Default.FileExists(archiveFilePath)) + { + using (FileStream fileStream = File.OpenRead(archiveFilePath)) + { + eventArgsWriter.WriteBlob(BinaryLogRecordKind.ProjectImportArchive, fileStream); + } + + File.Delete(archiveFilePath); + } } - projectImportsCollector.Close(); projectImportsCollector = null; } diff --git a/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs b/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs index cf69bcbacb..7b40c84f4b 100644 --- a/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs +++ b/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs @@ -218,6 +218,17 @@ namespace Microsoft.Build.Logging Write(bytes); } + public void WriteBlob(BinaryLogRecordKind kind, Stream stream) + { + // write the blob directly to the underlying writer, + // bypassing the memory stream + using var redirection = RedirectWritesToOriginalWriter(); + + Write(kind); + Write(stream.Length); + Write(stream); + } + /// /// Switches the binaryWriter used by the Write* methods to the direct underlying stream writer /// until the disposable is disposed. Useful to bypass the currentRecordWriter to write a string, @@ -1091,6 +1102,11 @@ namespace Microsoft.Build.Logging binaryWriter.Write(bytes); } + private void Write(Stream stream) + { + stream.CopyTo(binaryWriter.BaseStream); + } + private void Write(byte b) { binaryWriter.Write(b); diff --git a/src/Build/Logging/BinaryLogger/ProjectImportsCollector.cs b/src/Build/Logging/BinaryLogger/ProjectImportsCollector.cs index 41fa9daa78..27ededae8c 100644 --- a/src/Build/Logging/BinaryLogger/ProjectImportsCollector.cs +++ b/src/Build/Logging/BinaryLogger/ProjectImportsCollector.cs @@ -7,6 +7,7 @@ using System.IO; using System.IO.Compression; using System.Text; using System.Threading.Tasks; +using Microsoft.Build.Shared; #nullable disable @@ -20,30 +21,10 @@ namespace Microsoft.Build.Logging /// internal class ProjectImportsCollector { - private Stream _stream; - public byte[] GetAllBytes() - { - if (_stream == null) - { - return Array.Empty(); - } - else if (ArchiveFilePath == null) - { - var stream = _stream as MemoryStream; - // Before we can use the zip archive, it must be closed. - Close(false); - return stream.ToArray(); - } - else - { - Close(); - return File.ReadAllBytes(ArchiveFilePath); - } - } - + private Stream _fileStream; private ZipArchive _zipArchive; - private string ArchiveFilePath { get; set; } + public string ArchiveFilePath { get; } /// /// Avoid visiting each file more than once. @@ -55,33 +36,46 @@ namespace Microsoft.Build.Logging public ProjectImportsCollector(string logFilePath, bool createFile, string sourcesArchiveExtension = ".ProjectImports.zip") { + if (createFile) + { + // Archive file will be stored alongside the binlog + ArchiveFilePath = Path.ChangeExtension(logFilePath, sourcesArchiveExtension); + } + else + { + string cacheDirectory = FileUtilities.GetCacheDirectory(); + if (!Directory.Exists(cacheDirectory)) + { + Directory.CreateDirectory(cacheDirectory); + } + + // Archive file will be temporarily stored in MSBuild cache folder and deleted when no longer needed + ArchiveFilePath = Path.Combine( + cacheDirectory, + Path.ChangeExtension( + Path.GetFileName(logFilePath), + sourcesArchiveExtension)); + } + try { - if (createFile) - { - ArchiveFilePath = Path.ChangeExtension(logFilePath, sourcesArchiveExtension); - _stream = new FileStream(ArchiveFilePath, FileMode.Create, FileAccess.ReadWrite, FileShare.Delete); - } - else - { - _stream = new MemoryStream(); - } - _zipArchive = new ZipArchive(_stream, ZipArchiveMode.Create, true); + _fileStream = new FileStream(ArchiveFilePath, FileMode.Create, FileAccess.ReadWrite, FileShare.Delete); + _zipArchive = new ZipArchive(_fileStream, ZipArchiveMode.Create); } catch { // For some reason we weren't able to create a file for the archive. // Disable the file collector. - _stream = null; + _fileStream = null; _zipArchive = null; } } public void AddFile(string filePath) { - if (filePath != null && _stream != null) + if (filePath != null && _fileStream != null) { - lock (_stream) + lock (_fileStream) { // enqueue the task to add a file and return quickly // to avoid holding up the current thread @@ -101,9 +95,9 @@ namespace Microsoft.Build.Logging public void AddFileFromMemory(string filePath, string data) { - if (filePath != null && data != null && _stream != null) + if (filePath != null && data != null && _fileStream != null) { - lock (_stream) + lock (_fileStream) { // enqueue the task to add a file and return quickly // to avoid holding up the current thread @@ -197,7 +191,7 @@ namespace Microsoft.Build.Logging return archivePath; } - public void Close(bool closeStream = true) + public void Close() { // wait for all pending file writes to complete _currentTask.Wait(); @@ -208,10 +202,10 @@ namespace Microsoft.Build.Logging _zipArchive = null; } - if (closeStream && (_stream != null)) + if (_fileStream != null) { - _stream.Dispose(); - _stream = null; + _fileStream.Dispose(); + _fileStream = null; } } } diff --git a/src/MSBuild/XMake.cs b/src/MSBuild/XMake.cs index 06c53027f7..fb86829c8d 100644 --- a/src/MSBuild/XMake.cs +++ b/src/MSBuild/XMake.cs @@ -1498,8 +1498,8 @@ namespace Microsoft.Build.CommandLine } finally { - FileUtilities.ClearCacheDirectory(); projectCollection?.Dispose(); + FileUtilities.ClearCacheDirectory(); // Build manager shall be reused for all build sessions. // If, for one reason or another, this behavior needs to change in future From 4f2a020879c7a7f1f7c2f95beb4952b97412dcac Mon Sep 17 00:00:00 2001 From: David Federman Date: Tue, 6 Jun 2023 16:13:44 -0700 Subject: [PATCH 3/4] Avoid boxing when enumerating project xml children In a recent profile of a graph construction, it was observed that a large amount of boxing was happening for ProjectElementSiblingEnumerable. This change simplifies how xml children are enumerated by adding an internal ChildrenEnumerable property which directly exposes the ProjectElementSiblingEnumerable which should avoid boxing, at least in some code paths (the public API makes it hard to avoid everywhere...). Additionally, a very common usage of enumerating children was to do Children.OfType and wrap it in a ReadOnlyCollection, so I introduced a GetChildrenOfType (and GetChildrenReversedOfType) method which exposes an ICollection which does the same thing but without the boxing of ProjectElementSiblingEnumerable and without the OfType class. It's just 1 collection allocation. --- src/BannedSymbols.txt | 1 + .../ProjectImportElement_Tests.cs | 2 +- .../ProjectImportGroupElement_Tests.cs | 4 +- ...ProjectItemDefinitionGroupElement_Tests.cs | 2 +- .../ProjectItemGroupElement_tests.cs | 2 +- .../ProjectPropertyGroupElement_Tests.cs | 2 +- .../ProjectTargetElement_Tests.cs | 2 +- .../ProjectUsingTaskElement_Tests.cs | 2 +- .../UsingTaskParameterGroup_Tests.cs | 2 +- .../Construction/ProjectChooseElement.cs | 4 +- .../Construction/ProjectElementContainer.cs | 245 +++++++++++++++--- .../Construction/ProjectImportGroupElement.cs | 4 +- .../ProjectItemDefinitionElement.cs | 4 +- .../ProjectItemDefinitionGroupElement.cs | 4 +- src/Build/Construction/ProjectItemElement.cs | 4 +- .../Construction/ProjectItemGroupElement.cs | 3 +- .../Construction/ProjectOtherwiseElement.cs | 8 +- .../ProjectPropertyGroupElement.cs | 6 +- src/Build/Construction/ProjectRootElement.cs | 49 ++-- .../Construction/ProjectTargetElement.cs | 10 +- src/Build/Construction/ProjectTaskElement.cs | 3 +- src/Build/Construction/ProjectWhenElement.cs | 8 +- .../UsingTaskParameterGroupElement.cs | 4 +- src/Build/Definition/Toolset.cs | 2 +- src/Build/Evaluation/Evaluator.cs | 10 +- src/Directory.Build.targets | 2 +- src/Shared/ReadOnlyCollection.cs | 2 +- 27 files changed, 278 insertions(+), 113 deletions(-) diff --git a/src/BannedSymbols.txt b/src/BannedSymbols.txt index 80f588d4b8..f11ba0906a 100644 --- a/src/BannedSymbols.txt +++ b/src/BannedSymbols.txt @@ -1 +1,2 @@ M:System.Globalization.CompareInfo.IndexOf(System.String,System.Char);CompareInfo.IndexOf can unexpectedly allocate strings--use string.IndexOf +P:Microsoft.Build.Construction.ProjectElementContainer.Children;Use ChildrenEnumerable instead to avoid boxing diff --git a/src/Build.OM.UnitTests/Construction/ProjectImportElement_Tests.cs b/src/Build.OM.UnitTests/Construction/ProjectImportElement_Tests.cs index 5950b6a4ec..2b59cd4357 100644 --- a/src/Build.OM.UnitTests/Construction/ProjectImportElement_Tests.cs +++ b/src/Build.OM.UnitTests/Construction/ProjectImportElement_Tests.cs @@ -28,7 +28,7 @@ namespace Microsoft.Build.UnitTests.OM.Construction { ProjectRootElement project = ProjectRootElement.Create(); - Assert.Null(project.Imports.GetEnumerator().Current); + Assert.Empty(project.Imports); } /// diff --git a/src/Build.OM.UnitTests/Construction/ProjectImportGroupElement_Tests.cs b/src/Build.OM.UnitTests/Construction/ProjectImportGroupElement_Tests.cs index 289fe98f4c..f32676c98f 100644 --- a/src/Build.OM.UnitTests/Construction/ProjectImportGroupElement_Tests.cs +++ b/src/Build.OM.UnitTests/Construction/ProjectImportGroupElement_Tests.cs @@ -143,7 +143,7 @@ namespace Microsoft.Build.UnitTests.OM.Construction { ProjectRootElement project = ProjectRootElement.Create(); - Assert.Null(project.Imports.GetEnumerator().Current); + Assert.Empty(project.Imports); } /// @@ -162,7 +162,7 @@ namespace Microsoft.Build.UnitTests.OM.Construction ProjectImportGroupElement importGroup = (ProjectImportGroupElement)Helpers.GetFirst(project.ImportGroups); - Assert.Null(project.Imports.GetEnumerator().Current); + Assert.Empty(project.Imports); Assert.Equal(0, Helpers.Count(importGroup.Imports)); } diff --git a/src/Build.OM.UnitTests/Construction/ProjectItemDefinitionGroupElement_Tests.cs b/src/Build.OM.UnitTests/Construction/ProjectItemDefinitionGroupElement_Tests.cs index 0ffa9b60cc..7191ec67da 100644 --- a/src/Build.OM.UnitTests/Construction/ProjectItemDefinitionGroupElement_Tests.cs +++ b/src/Build.OM.UnitTests/Construction/ProjectItemDefinitionGroupElement_Tests.cs @@ -24,7 +24,7 @@ namespace Microsoft.Build.UnitTests.OM.Construction { ProjectRootElement project = ProjectRootElement.Create(); Assert.Equal(0, Helpers.Count(project.Children)); - Assert.Null(project.ItemDefinitionGroups.GetEnumerator().Current); + Assert.Empty(project.ItemDefinitionGroups); } /// diff --git a/src/Build.OM.UnitTests/Construction/ProjectItemGroupElement_tests.cs b/src/Build.OM.UnitTests/Construction/ProjectItemGroupElement_tests.cs index 8c71efffce..61660b61ef 100644 --- a/src/Build.OM.UnitTests/Construction/ProjectItemGroupElement_tests.cs +++ b/src/Build.OM.UnitTests/Construction/ProjectItemGroupElement_tests.cs @@ -24,7 +24,7 @@ namespace Microsoft.Build.UnitTests.OM.Construction { ProjectRootElement project = ProjectRootElement.Create(); Assert.Equal(0, Helpers.Count(project.Children)); - Assert.Null(project.ItemGroups.GetEnumerator().Current); + Assert.Empty(project.ItemGroups); } /// diff --git a/src/Build.OM.UnitTests/Construction/ProjectPropertyGroupElement_Tests.cs b/src/Build.OM.UnitTests/Construction/ProjectPropertyGroupElement_Tests.cs index 38f84d1b65..d70ad71868 100644 --- a/src/Build.OM.UnitTests/Construction/ProjectPropertyGroupElement_Tests.cs +++ b/src/Build.OM.UnitTests/Construction/ProjectPropertyGroupElement_Tests.cs @@ -23,7 +23,7 @@ namespace Microsoft.Build.UnitTests.OM.Construction { ProjectRootElement project = ProjectRootElement.Create(); Assert.Equal(0, Helpers.Count(project.Children)); - Assert.Null(project.PropertyGroups.GetEnumerator().Current); + Assert.Empty(project.PropertyGroups); } /// diff --git a/src/Build.OM.UnitTests/Construction/ProjectTargetElement_Tests.cs b/src/Build.OM.UnitTests/Construction/ProjectTargetElement_Tests.cs index 891e1bd625..aa46fef257 100644 --- a/src/Build.OM.UnitTests/Construction/ProjectTargetElement_Tests.cs +++ b/src/Build.OM.UnitTests/Construction/ProjectTargetElement_Tests.cs @@ -41,7 +41,7 @@ namespace Microsoft.Build.UnitTests.OM.Construction public void ReadNoTarget() { ProjectRootElement project = ProjectRootElement.Create(); - Assert.Null(project.Targets.GetEnumerator().Current); + Assert.Empty(project.Targets); } /// diff --git a/src/Build.OM.UnitTests/Construction/ProjectUsingTaskElement_Tests.cs b/src/Build.OM.UnitTests/Construction/ProjectUsingTaskElement_Tests.cs index cbb90d0ff9..fe49f9cb24 100644 --- a/src/Build.OM.UnitTests/Construction/ProjectUsingTaskElement_Tests.cs +++ b/src/Build.OM.UnitTests/Construction/ProjectUsingTaskElement_Tests.cs @@ -25,7 +25,7 @@ namespace Microsoft.Build.UnitTests.OM.Construction { ProjectRootElement project = ProjectRootElement.Create(); - Assert.Null(project.UsingTasks.GetEnumerator().Current); + Assert.Empty(project.UsingTasks); } /// diff --git a/src/Build.OM.UnitTests/Construction/UsingTaskParameterGroup_Tests.cs b/src/Build.OM.UnitTests/Construction/UsingTaskParameterGroup_Tests.cs index 9a4b587a16..dcdf5f8ac7 100644 --- a/src/Build.OM.UnitTests/Construction/UsingTaskParameterGroup_Tests.cs +++ b/src/Build.OM.UnitTests/Construction/UsingTaskParameterGroup_Tests.cs @@ -65,7 +65,7 @@ namespace Microsoft.Build.UnitTests.OM.Construction UsingTaskParameterGroupElement parameterGroup = GetParameterGroupXml(s_contentEmptyParameterGroup); Assert.NotNull(parameterGroup); Assert.Equal(0, parameterGroup.Count); - Assert.Null(parameterGroup.Parameters.GetEnumerator().Current); + Assert.Empty(parameterGroup.Parameters); } /// diff --git a/src/Build/Construction/ProjectChooseElement.cs b/src/Build/Construction/ProjectChooseElement.cs index 39d9fc749a..4ecc4131ac 100644 --- a/src/Build/Construction/ProjectChooseElement.cs +++ b/src/Build/Construction/ProjectChooseElement.cs @@ -3,9 +3,7 @@ using System.Collections.Generic; using System.Diagnostics; -using System.Linq; using System.Xml; -using Microsoft.Build.Collections; using Microsoft.Build.ObjectModelRemoting; using Microsoft.Build.Shared; @@ -60,7 +58,7 @@ namespace Microsoft.Build.Construction /// Get the When children. /// Will contain at least one entry. /// - public ICollection WhenElements => new ReadOnlyCollection(Children.OfType()); + public ICollection WhenElements => GetChildrenOfType(); /// /// Get any Otherwise child. diff --git a/src/Build/Construction/ProjectElementContainer.cs b/src/Build/Construction/ProjectElementContainer.cs index 6d8d7a394b..bc1f404e13 100644 --- a/src/Build/Construction/ProjectElementContainer.cs +++ b/src/Build/Construction/ProjectElementContainer.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Collections; using System.Collections.Generic; using System.Diagnostics; using System.Linq; @@ -59,10 +60,15 @@ namespace Microsoft.Build.Construction } /// - /// Get an enumerator over all children, gotten recursively. - /// Walks the children in a depth-first manner. + /// Get an enumerator over all descendants in a depth-first manner. /// - public IEnumerable AllChildren => GetChildrenRecursively(); + public IEnumerable AllChildren => GetDescendants(); + + internal IEnumerable GetAllChildrenOfType() + where T : ProjectElement + => FirstChild == null + ? Array.Empty() + : GetDescendantsOfType(); /// /// Get enumerable over all the children @@ -70,26 +76,41 @@ namespace Microsoft.Build.Construction public ICollection Children { [DebuggerStepThrough] - get - { - return new Collections.ReadOnlyCollection( - new ProjectElementSiblingEnumerable(FirstChild)); - } + get => FirstChild == null + ? Array.Empty() + : new Collections.ReadOnlyCollection(new ProjectElementSiblingEnumerable(FirstChild)); } +#pragma warning disable RS0030 // The ref to the banned API is in a doc comment + /// + /// Use this instead of to avoid boxing. + /// +#pragma warning restore RS0030 + internal ProjectElementSiblingEnumerable ChildrenEnumerable => new ProjectElementSiblingEnumerable(FirstChild); + + internal ProjectElementSiblingSubTypeCollection GetChildrenOfType() + where T : ProjectElement + => FirstChild == null + ? ProjectElementSiblingSubTypeCollection.Empty + : new ProjectElementSiblingSubTypeCollection(FirstChild); + /// /// Get enumerable over all the children, starting from the last /// public ICollection ChildrenReversed { [DebuggerStepThrough] - get - { - return new Collections.ReadOnlyCollection( - new ProjectElementSiblingEnumerable(LastChild, false /* reverse */)); - } + get => LastChild == null + ? Array.Empty() + : new Collections.ReadOnlyCollection(new ProjectElementSiblingEnumerable(LastChild, forwards: false)); } + internal ProjectElementSiblingSubTypeCollection GetChildrenReversedOfType() + where T : ProjectElement + => LastChild == null + ? ProjectElementSiblingSubTypeCollection.Empty + : new ProjectElementSiblingSubTypeCollection(LastChild, forwards: false); + /// /// Number of children of any kind /// @@ -318,7 +339,7 @@ namespace Microsoft.Build.Construction /// public void RemoveAllChildren() { - foreach (ProjectElement child in Children) + foreach (ProjectElement child in ChildrenEnumerable) { RemoveChild(child); } @@ -341,7 +362,7 @@ namespace Microsoft.Build.Construction RemoveAllChildren(); CopyFrom(element); - foreach (ProjectElement child in element.Children) + foreach (ProjectElement child in element.ChildrenEnumerable) { if (child is ProjectElementContainer childContainer) { @@ -395,7 +416,7 @@ namespace Microsoft.Build.Construction var clone = (ProjectElementContainer)Clone(factory); parent?.AppendChild(clone); - foreach (ProjectElement child in Children) + foreach (ProjectElement child in ChildrenEnumerable) { if (child is ProjectElementContainer childContainer) { @@ -667,7 +688,7 @@ namespace Microsoft.Build.Construction /// Result does NOT include the element passed in. /// The caller could filter these. /// - private IEnumerable GetChildrenRecursively() + private IEnumerable GetDescendants() { ProjectElement child = FirstChild; @@ -687,6 +708,30 @@ namespace Microsoft.Build.Construction } } + private IEnumerable GetDescendantsOfType() + where T : ProjectElement + { + ProjectElement child = FirstChild; + + while (child != null) + { + if (child is T childOfType) + { + yield return childOfType; + } + + if (child is ProjectElementContainer container) + { + foreach (T grandchild in container.GetAllChildrenOfType()) + { + yield return grandchild; + } + } + + child = child.NextSibling; + } + } + private static bool TrySearchLeftSiblings(ProjectElement initialElement, Predicate siblingIsAcceptable, out ProjectElement referenceSibling) { return TrySearchSiblings(initialElement, siblingIsAcceptable, s => s.PreviousSibling, out referenceSibling); @@ -721,44 +766,182 @@ namespace Microsoft.Build.Construction return referenceSibling != null; } + internal sealed class ProjectElementSiblingSubTypeCollection : ICollection, ICollection + where T : ProjectElement + { + private readonly ProjectElement _initial; + private readonly bool _forwards; + private List _realizedElements; + + internal ProjectElementSiblingSubTypeCollection(ProjectElement initial, bool forwards = true) + { + _initial = initial; + _forwards = forwards; + } + + public static ProjectElementSiblingSubTypeCollection Empty { get; } = new ProjectElementSiblingSubTypeCollection(initial: null); + + public int Count => RealizedElements.Count; + + public bool IsReadOnly => true; + + bool ICollection.IsSynchronized => false; + + object ICollection.SyncRoot => this; + + private List RealizedElements + { + get + { + if (_realizedElements == null) + { + // Note! Don't use the List ctor which takes an IEnumerable as it casts to an ICollection and calls Count, + // which leads to a StackOverflow exception in this implementation (see Count above) + List list = new(); + foreach (T element in this) + { + list.Add(element); + } + + _realizedElements = list; + } + + return _realizedElements; + } + } + + public void Add(T item) => ErrorUtilities.ThrowInvalidOperation("OM_NotSupportedReadOnlyCollection"); + + public void Clear() => ErrorUtilities.ThrowInvalidOperation("OM_NotSupportedReadOnlyCollection"); + + public bool Contains(T item) => RealizedElements.Contains(item); + + public void CopyTo(T[] array, int arrayIndex) + { + ErrorUtilities.VerifyThrowArgumentNull(array, nameof(array)); + + if (_realizedElements != null) + { + _realizedElements.CopyTo(array, arrayIndex); + } + else + { + int i = arrayIndex; + foreach (T entry in this) + { + array[i] = entry; + i++; + } + } + } + + public bool Remove(T item) + { + ErrorUtilities.ThrowInvalidOperation("OM_NotSupportedReadOnlyCollection"); + return false; + } + + public IEnumerator GetEnumerator() => new Enumerator(_initial, _forwards); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + + void ICollection.CopyTo(Array array, int index) + { + ErrorUtilities.VerifyThrowArgumentNull(array, nameof(array)); + + int i = index; + foreach (T entry in this) + { + array.SetValue(entry, i); + i++; + } + } + + public struct Enumerator : IEnumerator + { + // Note! Should not be readonly or we run into infinite loop issues with mutable structs + private ProjectElementSiblingEnumerable.Enumerator _innerEnumerator; + private T _current; + + internal Enumerator(ProjectElement initial, bool forwards = true) + { + _innerEnumerator = new ProjectElementSiblingEnumerable.Enumerator(initial, forwards); + } + + public T Current + { + get + { + if (_current != null) + { + return _current; + } + + throw new InvalidOperationException(); + } + } + + object IEnumerator.Current => Current; + + public readonly void Dispose() + { + } + + public bool MoveNext() + { + while (_innerEnumerator.MoveNext()) + { + ProjectElement innerCurrent = _innerEnumerator.Current; + if (innerCurrent is T innerCurrentOfType) + { + _current = innerCurrentOfType; + return true; + } + } + + return false; + } + + public void Reset() + { + _innerEnumerator.Reset(); + _current = null; + } + } + } + /// /// Enumerable over a series of sibling ProjectElement objects /// - private struct ProjectElementSiblingEnumerable : IEnumerable + internal readonly struct ProjectElementSiblingEnumerable : IEnumerable { /// /// The enumerator /// - private readonly ProjectElementSiblingEnumerator _enumerator; + private readonly Enumerator _enumerator; /// /// Constructor allowing reverse enumeration /// internal ProjectElementSiblingEnumerable(ProjectElement initial, bool forwards = true) { - _enumerator = new ProjectElementSiblingEnumerator(initial, forwards); + _enumerator = new Enumerator(initial, forwards); } /// /// Get enumerator /// - public readonly IEnumerator GetEnumerator() - { - return _enumerator; - } + public readonly IEnumerator GetEnumerator() => _enumerator; /// /// Get non generic enumerator /// - System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() - { - return _enumerator; - } + IEnumerator IEnumerable.GetEnumerator() => _enumerator; /// /// Enumerator over a series of sibling ProjectElement objects /// - private struct ProjectElementSiblingEnumerator : IEnumerator + public struct Enumerator : IEnumerator { /// /// First element @@ -775,7 +958,7 @@ namespace Microsoft.Build.Construction /// /// Constructor taking the first element /// - internal ProjectElementSiblingEnumerator(ProjectElement initial, bool forwards) + internal Enumerator(ProjectElement initial, bool forwards) { _initial = initial; Current = null; @@ -792,7 +975,7 @@ namespace Microsoft.Build.Construction /// Current element. /// Throws if MoveNext() hasn't been called /// - object System.Collections.IEnumerator.Current + object IEnumerator.Current { get { diff --git a/src/Build/Construction/ProjectImportGroupElement.cs b/src/Build/Construction/ProjectImportGroupElement.cs index d6c6979e99..d513117481 100644 --- a/src/Build/Construction/ProjectImportGroupElement.cs +++ b/src/Build/Construction/ProjectImportGroupElement.cs @@ -3,8 +3,6 @@ using System.Collections.Generic; using System.Diagnostics; -using System.Linq; -using Microsoft.Build.Collections; using Microsoft.Build.ObjectModelRemoting; using Microsoft.Build.Shared; @@ -51,7 +49,7 @@ namespace Microsoft.Build.Construction /// /// Get any contained properties. /// - public ICollection Imports => new ReadOnlyCollection(Children.OfType()); + public ICollection Imports => GetChildrenOfType(); #endregion // Properties diff --git a/src/Build/Construction/ProjectItemDefinitionElement.cs b/src/Build/Construction/ProjectItemDefinitionElement.cs index dc8fa52003..85edb365e8 100644 --- a/src/Build/Construction/ProjectItemDefinitionElement.cs +++ b/src/Build/Construction/ProjectItemDefinitionElement.cs @@ -3,9 +3,7 @@ using System.Collections.Generic; using System.Diagnostics; -using System.Linq; using System.Xml; -using Microsoft.Build.Collections; using Microsoft.Build.ObjectModelRemoting; using Microsoft.Build.Shared; @@ -52,7 +50,7 @@ namespace Microsoft.Build.Construction /// /// Get any child metadata definitions. /// - public ICollection Metadata => new ReadOnlyCollection(Children.OfType()); + public ICollection Metadata => GetChildrenOfType(); /// /// Convenience method to add a piece of metadata to this item definition. diff --git a/src/Build/Construction/ProjectItemDefinitionGroupElement.cs b/src/Build/Construction/ProjectItemDefinitionGroupElement.cs index 892b33cb8d..36a034e32d 100644 --- a/src/Build/Construction/ProjectItemDefinitionGroupElement.cs +++ b/src/Build/Construction/ProjectItemDefinitionGroupElement.cs @@ -3,9 +3,7 @@ using System.Collections.Generic; using System.Diagnostics; -using System.Linq; using System.Xml; -using Microsoft.Build.Collections; using Microsoft.Build.ObjectModelRemoting; using Microsoft.Build.Shared; @@ -47,7 +45,7 @@ namespace Microsoft.Build.Construction /// /// Get a list of child item definitions. /// - public ICollection ItemDefinitions => new ReadOnlyCollection(Children.OfType()); + public ICollection ItemDefinitions => GetChildrenOfType(); /// /// Convenience method that picks a location based on a heuristic: diff --git a/src/Build/Construction/ProjectItemElement.cs b/src/Build/Construction/ProjectItemElement.cs index bb31b120e9..fb98d90833 100644 --- a/src/Build/Construction/ProjectItemElement.cs +++ b/src/Build/Construction/ProjectItemElement.cs @@ -4,9 +4,7 @@ using System; using System.Collections.Generic; using System.Diagnostics; -using System.Linq; using System.Xml; -using Microsoft.Build.Collections; using Microsoft.Build.ObjectModelRemoting; using Microsoft.Build.Shared; @@ -285,7 +283,7 @@ namespace Microsoft.Build.Construction /// /// Get any child metadata. /// - public ICollection Metadata => new ReadOnlyCollection(Children.OfType()); + public ICollection Metadata => GetChildrenOfType(); /// /// Location of the include attribute diff --git a/src/Build/Construction/ProjectItemGroupElement.cs b/src/Build/Construction/ProjectItemGroupElement.cs index b637161a35..03dc40eb8e 100644 --- a/src/Build/Construction/ProjectItemGroupElement.cs +++ b/src/Build/Construction/ProjectItemGroupElement.cs @@ -4,7 +4,6 @@ using System; using System.Collections.Generic; using System.Diagnostics; -using System.Linq; using Microsoft.Build.Collections; using Microsoft.Build.ObjectModelRemoting; using Microsoft.Build.Shared; @@ -55,7 +54,7 @@ namespace Microsoft.Build.Construction /// Get any child items. /// This is a live collection. /// - public ICollection Items => new ReadOnlyCollection(Children.OfType()); + public ICollection Items => GetChildrenOfType(); /// /// True if it is known that no child items have wildcards in their diff --git a/src/Build/Construction/ProjectOtherwiseElement.cs b/src/Build/Construction/ProjectOtherwiseElement.cs index f3002cc8a8..81adce4ec7 100644 --- a/src/Build/Construction/ProjectOtherwiseElement.cs +++ b/src/Build/Construction/ProjectOtherwiseElement.cs @@ -3,8 +3,6 @@ using System.Collections.Generic; using System.Diagnostics; -using System.Linq; -using Microsoft.Build.Collections; using Microsoft.Build.ObjectModelRemoting; using Microsoft.Build.Shared; @@ -57,17 +55,17 @@ namespace Microsoft.Build.Construction /// /// Get an enumerator over any child item groups /// - public ICollection ItemGroups => new ReadOnlyCollection(Children.OfType()); + public ICollection ItemGroups => GetChildrenOfType(); /// /// Get an enumerator over any child property groups /// - public ICollection PropertyGroups => new ReadOnlyCollection(Children.OfType()); + public ICollection PropertyGroups => GetChildrenOfType(); /// /// Get an enumerator over any child chooses /// - public ICollection ChooseElements => new ReadOnlyCollection(Children.OfType()); + public ICollection ChooseElements => GetChildrenOfType(); #endregion diff --git a/src/Build/Construction/ProjectPropertyGroupElement.cs b/src/Build/Construction/ProjectPropertyGroupElement.cs index 4c9fc27fe4..16932b3107 100644 --- a/src/Build/Construction/ProjectPropertyGroupElement.cs +++ b/src/Build/Construction/ProjectPropertyGroupElement.cs @@ -4,8 +4,6 @@ using System; using System.Collections.Generic; using System.Diagnostics; -using System.Linq; -using Microsoft.Build.Collections; using Microsoft.Build.ObjectModelRemoting; using Microsoft.Build.Shared; @@ -47,12 +45,12 @@ namespace Microsoft.Build.Construction /// /// Get any contained properties. /// - public ICollection Properties => new ReadOnlyCollection(Children.OfType()); + public ICollection Properties => GetChildrenOfType(); /// /// Get any contained properties. /// - public ICollection PropertiesReversed => new ReadOnlyCollection(ChildrenReversed.OfType()); + public ICollection PropertiesReversed => GetChildrenReversedOfType(); /// /// Convenience method that picks a location based on a heuristic: diff --git a/src/Build/Construction/ProjectRootElement.cs b/src/Build/Construction/ProjectRootElement.cs index 626751e11c..1db13d96a4 100644 --- a/src/Build/Construction/ProjectRootElement.cs +++ b/src/Build/Construction/ProjectRootElement.cs @@ -287,81 +287,81 @@ namespace Microsoft.Build.Construction /// /// The name is inconsistent to make it more understandable, per API review. /// - public ICollection ChooseElements => new ReadOnlyCollection(Children.OfType()); + public ICollection ChooseElements => GetChildrenOfType(); /// /// Get a read-only collection of the child item definition groups, if any /// - public ICollection ItemDefinitionGroups => new ReadOnlyCollection(Children.OfType()); + public ICollection ItemDefinitionGroups => GetChildrenOfType(); /// /// Get a read-only collection of the child item definitions, if any, in all item definition groups anywhere in the project file. /// - public ICollection ItemDefinitions => new ReadOnlyCollection(AllChildren.OfType()); + public ICollection ItemDefinitions => new ReadOnlyCollection(GetAllChildrenOfType()); /// /// Get a read-only collection over the child item groups, if any. /// Does not include any that may not be at the root, i.e. inside Choose elements. /// - public ICollection ItemGroups => new ReadOnlyCollection(Children.OfType()); + public ICollection ItemGroups => GetChildrenOfType(); /// /// Get a read-only collection of the child items, if any, in all item groups anywhere in the project file. /// Not restricted to root item groups: traverses through Choose elements. /// - public ICollection Items => new ReadOnlyCollection(AllChildren.OfType()); + public ICollection Items => new ReadOnlyCollection(GetAllChildrenOfType()); /// /// Get a read-only collection of the child import groups, if any. /// - public ICollection ImportGroups => new ReadOnlyCollection(Children.OfType()); + public ICollection ImportGroups => GetChildrenOfType(); /// /// Get a read-only collection of the child imports /// - public ICollection Imports => new ReadOnlyCollection(AllChildren.OfType()); + public ICollection Imports => new ReadOnlyCollection(GetAllChildrenOfType()); /// /// Get a read-only collection of the child property groups, if any. /// Does not include any that may not be at the root, i.e. inside Choose elements. /// - public ICollection PropertyGroups => new ReadOnlyCollection(Children.OfType()); + public ICollection PropertyGroups => GetChildrenOfType(); /// /// Geta read-only collection of the child properties, if any, in all property groups anywhere in the project file. /// Not restricted to root property groups: traverses through Choose elements. /// - public ICollection Properties => new ReadOnlyCollection(AllChildren.OfType()); + public ICollection Properties => new ReadOnlyCollection(GetAllChildrenOfType()); /// /// Get a read-only collection of the child targets /// - public ICollection Targets => new ReadOnlyCollection(Children.OfType()); + public ICollection Targets => GetChildrenOfType(); /// /// Get a read-only collection of the child usingtasks, if any /// - public ICollection UsingTasks => new ReadOnlyCollection(Children.OfType()); + public ICollection UsingTasks => GetChildrenOfType(); /// /// Get a read-only collection of the child item groups, if any, in reverse order /// - public ICollection ItemGroupsReversed => new ReadOnlyCollection(ChildrenReversed.OfType()); + public ICollection ItemGroupsReversed => GetChildrenReversedOfType(); /// /// Get a read-only collection of the child item definition groups, if any, in reverse order /// - public ICollection ItemDefinitionGroupsReversed => new ReadOnlyCollection(ChildrenReversed.OfType()); + public ICollection ItemDefinitionGroupsReversed => GetChildrenReversedOfType(); /// /// Get a read-only collection of the child import groups, if any, in reverse order /// - public ICollection ImportGroupsReversed => new ReadOnlyCollection(ChildrenReversed.OfType()); + public ICollection ImportGroupsReversed => GetChildrenReversedOfType(); /// /// Get a read-only collection of the child property groups, if any, in reverse order /// - public ICollection PropertyGroupsReversed => new ReadOnlyCollection(ChildrenReversed.OfType()); + public ICollection PropertyGroupsReversed => GetChildrenReversedOfType(); #endregion @@ -702,7 +702,7 @@ namespace Microsoft.Build.Construction /// Not public as we do not wish to encourage the use of ProjectExtensions. /// internal ProjectExtensionsElement ProjectExtensions - => ChildrenReversed.OfType().FirstOrDefault(); + => GetChildrenReversedOfType().FirstOrDefault(); /// /// Returns an unlocalized indication of how this file was last dirtied. @@ -1905,15 +1905,18 @@ namespace Microsoft.Build.Construction } } - foreach (var sdkNode in Children.OfType()) + foreach (ProjectElement child in ChildrenEnumerable) { - var referencedSdk = new SdkReference( - sdkNode.XmlElement.GetAttribute("Name"), - sdkNode.XmlElement.GetAttribute("Version"), - sdkNode.XmlElement.GetAttribute("MinimumVersion")); + if (child is ProjectSdkElement sdkNode) + { + var referencedSdk = new SdkReference( + sdkNode.XmlElement.GetAttribute("Name"), + sdkNode.XmlElement.GetAttribute("Version"), + sdkNode.XmlElement.GetAttribute("MinimumVersion")); - nodes.Add(ProjectImportElement.CreateImplicit("Sdk.props", currentProjectOrImport, ImplicitImportLocation.Top, referencedSdk, sdkNode)); - nodes.Add(ProjectImportElement.CreateImplicit("Sdk.targets", currentProjectOrImport, ImplicitImportLocation.Bottom, referencedSdk, sdkNode)); + nodes.Add(ProjectImportElement.CreateImplicit("Sdk.props", currentProjectOrImport, ImplicitImportLocation.Top, referencedSdk, sdkNode)); + nodes.Add(ProjectImportElement.CreateImplicit("Sdk.targets", currentProjectOrImport, ImplicitImportLocation.Bottom, referencedSdk, sdkNode)); + } } return nodes; diff --git a/src/Build/Construction/ProjectTargetElement.cs b/src/Build/Construction/ProjectTargetElement.cs index f7618c427b..e989300903 100644 --- a/src/Build/Construction/ProjectTargetElement.cs +++ b/src/Build/Construction/ProjectTargetElement.cs @@ -4,8 +4,6 @@ using System; using System.Collections.Generic; using System.Diagnostics; -using System.Linq; -using Microsoft.Build.Collections; using Microsoft.Build.Execution; using Microsoft.Build.ObjectModelRemoting; using Microsoft.Build.Shared; @@ -58,22 +56,22 @@ namespace Microsoft.Build.Construction /// /// Get an enumerator over any child item groups /// - public ICollection ItemGroups => new ReadOnlyCollection(Children.OfType()); + public ICollection ItemGroups => GetChildrenOfType(); /// /// Get an enumerator over any child property groups /// - public ICollection PropertyGroups => new ReadOnlyCollection(Children.OfType()); + public ICollection PropertyGroups => GetChildrenOfType(); /// /// Get an enumerator over any child tasks /// - public ICollection Tasks => new ReadOnlyCollection(Children.OfType()); + public ICollection Tasks => GetChildrenOfType(); /// /// Get an enumerator over any child onerrors /// - public ICollection OnErrors => new ReadOnlyCollection(Children.OfType()); + public ICollection OnErrors => GetChildrenOfType(); #endregion diff --git a/src/Build/Construction/ProjectTaskElement.cs b/src/Build/Construction/ProjectTaskElement.cs index 49048f632b..94031b4994 100644 --- a/src/Build/Construction/ProjectTaskElement.cs +++ b/src/Build/Construction/ProjectTaskElement.cs @@ -5,7 +5,6 @@ using System; using System.Collections.Generic; using System.Collections.ObjectModel; using System.Diagnostics; -using System.Linq; using System.Xml; using Microsoft.Build.Collections; using Microsoft.Build.ObjectModelRemoting; @@ -126,7 +125,7 @@ namespace Microsoft.Build.Construction /// /// Gets any output children. /// - public ICollection Outputs => new Collections.ReadOnlyCollection(Children.OfType()); + public ICollection Outputs => GetChildrenOfType(); /// /// Enumerable over the unevaluated parameters on the task. diff --git a/src/Build/Construction/ProjectWhenElement.cs b/src/Build/Construction/ProjectWhenElement.cs index 92140145ed..a706a66b28 100644 --- a/src/Build/Construction/ProjectWhenElement.cs +++ b/src/Build/Construction/ProjectWhenElement.cs @@ -3,9 +3,7 @@ using System.Collections.Generic; using System.Diagnostics; -using System.Linq; using System.Xml; -using Microsoft.Build.Collections; using Microsoft.Build.ObjectModelRemoting; using Microsoft.Build.Shared; @@ -48,17 +46,17 @@ namespace Microsoft.Build.Construction /// /// Get an enumerator over any child chooses /// - public ICollection ChooseElements => new ReadOnlyCollection(Children.OfType()); + public ICollection ChooseElements => GetChildrenOfType(); /// /// Get an enumerator over any child item groups /// - public ICollection ItemGroups => new ReadOnlyCollection(Children.OfType()); + public ICollection ItemGroups => GetChildrenOfType(); /// /// Get an enumerator over any child property groups /// - public ICollection PropertyGroups => new ReadOnlyCollection(Children.OfType()); + public ICollection PropertyGroups => GetChildrenOfType(); #endregion diff --git a/src/Build/Construction/UsingTaskParameterGroupElement.cs b/src/Build/Construction/UsingTaskParameterGroupElement.cs index a0fc796495..3e76edb1e8 100644 --- a/src/Build/Construction/UsingTaskParameterGroupElement.cs +++ b/src/Build/Construction/UsingTaskParameterGroupElement.cs @@ -4,8 +4,6 @@ using System; using System.Collections.Generic; using System.Diagnostics; -using System.Linq; -using Microsoft.Build.Collections; using Microsoft.Build.ObjectModelRemoting; using Microsoft.Build.Shared; using ProjectXmlUtilities = Microsoft.Build.Internal.ProjectXmlUtilities; @@ -59,7 +57,7 @@ namespace Microsoft.Build.Construction /// /// Get any contained parameters. /// - public ICollection Parameters => new ReadOnlyCollection(Children.OfType()); + public ICollection Parameters => GetChildrenOfType(); /// /// This does not allow conditions, so it should not be called. diff --git a/src/Build/Definition/Toolset.cs b/src/Build/Definition/Toolset.cs index ec59aaff46..2d61f07793 100644 --- a/src/Build/Definition/Toolset.cs +++ b/src/Build/Definition/Toolset.cs @@ -1062,7 +1062,7 @@ namespace Microsoft.Build.Evaluation preserveFormatting: false); } - foreach (ProjectElement elementXml in projectRootElement.Children) + foreach (ProjectElement elementXml in projectRootElement.ChildrenEnumerable) { ProjectUsingTaskElement usingTask = elementXml as ProjectUsingTaskElement; diff --git a/src/Build/Evaluation/Evaluator.cs b/src/Build/Evaluation/Evaluator.cs index a5b3be796b..731caaa00b 100644 --- a/src/Build/Evaluation/Evaluator.cs +++ b/src/Build/Evaluation/Evaluator.cs @@ -535,7 +535,7 @@ namespace Microsoft.Build.Evaluation List targetChildren = new List(targetElement.Count); List targetOnErrorChildren = new List(); - foreach (ProjectElement targetChildElement in targetElement.Children) + foreach (ProjectElement targetChildElement in targetElement.ChildrenEnumerable) { using (evaluationProfiler.TrackElement(targetChildElement)) { @@ -881,7 +881,7 @@ namespace Microsoft.Build.Evaluation } } - foreach (ProjectElement element in currentProjectOrImport.Children) + foreach (ProjectElement element in currentProjectOrImport.ChildrenEnumerable) { switch (element) { @@ -1499,7 +1499,7 @@ namespace Microsoft.Build.Evaluation { if (EvaluateConditionCollectingConditionedProperties(whenElement, ExpanderOptions.ExpandProperties, ParserOptions.AllowProperties)) { - EvaluateWhenOrOtherwiseChildren(whenElement.Children); + EvaluateWhenOrOtherwiseChildren(whenElement.ChildrenEnumerable); return; } } @@ -1507,7 +1507,7 @@ namespace Microsoft.Build.Evaluation // "Otherwise" elements never have a condition if (chooseElement.OtherwiseElement != null) { - EvaluateWhenOrOtherwiseChildren(chooseElement.OtherwiseElement.Children); + EvaluateWhenOrOtherwiseChildren(chooseElement.OtherwiseElement.ChildrenEnumerable); } } } @@ -1517,7 +1517,7 @@ namespace Microsoft.Build.Evaluation /// Returns true if the condition was true, so subsequent /// WhenElements and Otherwise can be skipped. /// - private bool EvaluateWhenOrOtherwiseChildren(IEnumerable children) + private bool EvaluateWhenOrOtherwiseChildren(ProjectElementContainer.ProjectElementSiblingEnumerable children) { foreach (ProjectElement element in children) { diff --git a/src/Directory.Build.targets b/src/Directory.Build.targets index b3f2e35187..67e5223c25 100644 --- a/src/Directory.Build.targets +++ b/src/Directory.Build.targets @@ -79,7 +79,7 @@ - + diff --git a/src/Shared/ReadOnlyCollection.cs b/src/Shared/ReadOnlyCollection.cs index 32d751d274..d7e5c12ce5 100644 --- a/src/Shared/ReadOnlyCollection.cs +++ b/src/Shared/ReadOnlyCollection.cs @@ -20,7 +20,7 @@ namespace Microsoft.Build.Collections /// Thus this is an omission from the BCL. /// /// Type of element in the collection - internal class ReadOnlyCollection : ICollection, ICollection + internal sealed class ReadOnlyCollection : ICollection, ICollection { /// /// Backing live enumerable. From 657005a80eabcbdd9b96f307f2f746861702a69b Mon Sep 17 00:00:00 2001 From: Michael Shea Date: Wed, 14 Jun 2023 16:46:19 -0400 Subject: [PATCH 4/4] Removing sln level turn off of setplatform feature Currently we turn off dynamic platform resolution for a whole solution if a single project in the solution is assigned a configuration. This is problematic as some projects are outside of the scope of the solution but still have certain targets that run on them that are architecture specific. These projects will build as the wrong architecture because no configuration is defined and no platform negotiation takes place. I removed the conditional that turns platform negotiation off on a sln level. The logic to turn this off on a project level is already in place through checking is a projectreference has setplatform appended to it. This will make sure no projects with configurations defined will be negotiated for as MSbuild adds setplatform metadata to projectreferences with configurations. --- .../Graph/GetCompatiblePlatformGraph_Tests.cs | 99 ++++++++++++++++++- src/Build/Graph/ProjectInterpretation.cs | 15 ++- .../Microsoft.Common.CurrentVersion.targets | 1 - 3 files changed, 110 insertions(+), 5 deletions(-) diff --git a/src/Build.UnitTests/Graph/GetCompatiblePlatformGraph_Tests.cs b/src/Build.UnitTests/Graph/GetCompatiblePlatformGraph_Tests.cs index 845b0c556c..b941649ad7 100644 --- a/src/Build.UnitTests/Graph/GetCompatiblePlatformGraph_Tests.cs +++ b/src/Build.UnitTests/Graph/GetCompatiblePlatformGraph_Tests.cs @@ -27,7 +27,7 @@ namespace Microsoft.Build.Graph.UnitTests /// /// Performs SetPlatform negotiation for all project references when opted /// in via the EnableDynamicPlatformResolution property. - /// + /// /// The static graph mirrors the negotiation during build to determine plartform for each node. /// These tests mirror GetCompatiblePlatform_Tests.cs in order to make sure they both are in sync. /// @@ -351,5 +351,102 @@ namespace Microsoft.Build.Graph.UnitTests GetFirstNodeWithProjectNumber(graph, 2).ProjectInstance.GetPropertyValue("Platform").ShouldBe(GetFirstNodeWithProjectNumber(graph, 1).ProjectInstance.GetPropertyValue("Platform")); } } + + // Validate configurations are defined in project reference protocol + [Fact] + public void SolutionWithoutAllConfigurations() + { + using (TestEnvironment testEnvironment = TestEnvironment.Create()) + { + var firstProjectName = "1"; + var secondProjectName = "2"; + var thirdProjectName = "3"; + TransientTestFolder folder = testEnvironment.CreateFolder(createFolder: true); + TransientTestFolder project1Folder = testEnvironment.CreateFolder(Path.Combine(folder.Path, firstProjectName), createFolder: true); + TransientTestFolder project1SubFolder = testEnvironment.CreateFolder(Path.Combine(project1Folder.Path, firstProjectName), createFolder: true); + TransientTestFile project1 = testEnvironment.CreateFile(project1SubFolder, $"{firstProjectName}.csproj", + @" + + true + x64 + + + + + + + "); + + TransientTestFolder project2Folder = testEnvironment.CreateFolder(Path.Combine(folder.Path, secondProjectName), createFolder: true); + TransientTestFolder project2SubFolder = testEnvironment.CreateFolder(Path.Combine(project2Folder.Path, secondProjectName), createFolder: true); + TransientTestFile project2 = testEnvironment.CreateFile(project2SubFolder, $"{secondProjectName}.proj", + @" + + true + AnyCPU;x64 + + + "); + + TransientTestFolder project3Folder = testEnvironment.CreateFolder(Path.Combine(folder.Path, thirdProjectName), createFolder: true); + TransientTestFolder project3SubFolder = testEnvironment.CreateFolder(Path.Combine(project3Folder.Path, thirdProjectName), createFolder: true); + TransientTestFile project3 = testEnvironment.CreateFile(project3SubFolder, $"{thirdProjectName}.proj", + @" + + true + AnyCPU;x64 + + + "); + + + // Slashes here (and in the .slnf) are hardcoded as backslashes intentionally to support the common case. + TransientTestFile solutionFile = testEnvironment.CreateFile(folder, "SimpleProject.sln", + @" + Microsoft Visual Studio Solution File, Format Version 12.00 + # Visual Studio Version 16 + VisualStudioVersion = 16.0.29326.124 + MinimumVisualStudioVersion = 10.0.40219.1 + Project(""{9A19103F-16F7-4668-BE54-9A1E7A4F7556}"") = ""Project1"", ""1\1\1.csproj"", ""{79B5EBA6-5D27-4976-BC31-14422245A59A}"" + EndProject + Project(""{9A19103F-16F7-4668-BE54-9A1E7A4F7556}"") = ""2"", ""2\2\2.proj"", ""{8EFCCA22-9D51-4268-90F7-A595E11FCB2D}"" + EndProject + Global + GlobalSection(SolutionConfigurationPlatforms) = preSolution + Debug|x64 = Debug|x64 + Release|x64 = Release|x64 + EndGlobalSection + GlobalSection(ProjectConfigurationPlatforms) = postSolution + {79B5EBA6-5D27-4976-BC31-14422245A59A}.Debug|x64.ActiveCfg = Debug|x64 + {79B5EBA6-5D27-4976-BC31-14422245A59A}.Debug|x64.Build.0 = Debug|x64 + {79B5EBA6-5D27-4976-BC31-14422245A59A}.Release|x64.ActiveCfg = Release|x64 + {79B5EBA6-5D27-4976-BC31-14422245A59A}.Release|x64.Build.0 = Release|x64 + + {8EFCCA22-9D51-4268-90F7-A595E11FCB2D}.Debug|x64.ActiveCfg = Debug|Any CPU + {8EFCCA22-9D51-4268-90F7-A595E11FCB2D}.Debug|x64.Build.0 = Debug|Any CPU + {8EFCCA22-9D51-4268-90F7-A595E11FCB2D}.Release|x64.ActiveCfg = Release|Any CPU + {8EFCCA22-9D51-4268-90F7-A595E11FCB2D}.Release|x64.Build.0 = Release|Any CPU + EndGlobalSection + GlobalSection(SolutionProperties) = preSolution + HideSolutionNode = FALSE + EndGlobalSection + GlobalSection(ExtensibilityGlobals) = postSolution + SolutionGuid = {DE7234EC-0C4D-4070-B66A-DCF1B4F0CFEF} + EndGlobalSection + EndGlobal + "); + + ProjectCollection projectCollection = testEnvironment.CreateProjectCollection().Collection; + MockLogger logger = new(); + projectCollection.RegisterLogger(logger); + ProjectGraphEntryPoint entryPoint = new(solutionFile.Path, new Dictionary()); + + // We want to make sure negotiation respects configuration if defined but negotiates if not. + ProjectGraph graphFromSolution = new(entryPoint, projectCollection); + logger.AssertNoErrors(); + GetFirstNodeWithProjectNumber(graphFromSolution, 2).ProjectInstance.GetPropertyValue("Platform").ShouldBe("AnyCPU", "Project2 should have followed the sln config to AnyCPU"); + GetFirstNodeWithProjectNumber(graphFromSolution, 3).ProjectInstance.GetPropertyValue("Platform").ShouldBe("x64", "Project3 isn't in the solution so it should have negotiated to x64 to match Project1"); + } + } } } diff --git a/src/Build/Graph/ProjectInterpretation.cs b/src/Build/Graph/ProjectInterpretation.cs index dd47dbadc8..d617f78e35 100644 --- a/src/Build/Graph/ProjectInterpretation.cs +++ b/src/Build/Graph/ProjectInterpretation.cs @@ -63,7 +63,7 @@ namespace Microsoft.Build.Graph { public TargetSpecification(string target, bool skipIfNonexistent) { - // Verify that if this target is skippable then it equals neither + // Verify that if this target is skippable then it equals neither // ".default" nor ".projectReferenceTargetsOrDefaultTargets". ErrorUtilities.VerifyThrow( !skipIfNonexistent || (!target.Equals(MSBuildConstants.DefaultTargetsMarker) @@ -131,6 +131,8 @@ namespace Microsoft.Build.Graph allowCollectionReuse: solutionConfiguration == null && !enableDynamicPlatformResolution, globalPropertiesModifiers); + bool configurationDefined = false; + // Match what AssignProjectConfiguration does to resolve project references. if (solutionConfiguration != null) { @@ -151,6 +153,8 @@ namespace Microsoft.Build.Graph { referenceGlobalProperties.Remove(PlatformMetadataName); } + + configurationDefined = true; } else { @@ -161,11 +165,16 @@ namespace Microsoft.Build.Graph referenceGlobalProperties.Remove(ConfigurationMetadataName); referenceGlobalProperties.Remove(PlatformMetadataName); } + else + { + configurationDefined = true; + } } } - // Note: Dynamic platform resolution is not enabled for sln-based builds. - else if (!projectReferenceItem.HasMetadata(SetPlatformMetadataName) && enableDynamicPlatformResolution) + // Note: Dynamic platform resolution is not enabled for sln-based builds, + // unless the project isn't known to the solution. + if (enableDynamicPlatformResolution && !configurationDefined && !projectReferenceItem.HasMetadata(SetPlatformMetadataName)) { string requesterPlatform = requesterInstance.GetPropertyValue("Platform"); string requesterPlatformLookupTable = requesterInstance.GetPropertyValue("PlatformLookupTable"); diff --git a/src/Tasks/Microsoft.Common.CurrentVersion.targets b/src/Tasks/Microsoft.Common.CurrentVersion.targets index 18ac9baa9a..3289d86b32 100644 --- a/src/Tasks/Microsoft.Common.CurrentVersion.targets +++ b/src/Tasks/Microsoft.Common.CurrentVersion.targets @@ -1648,7 +1648,6 @@ Copyright (C) Microsoft Corporation. All rights reserved. Configuration information. See AssignProjectConfiguration -->