From a412c8e675d5fdf3a76894f151f03cdf51372073 Mon Sep 17 00:00:00 2001 From: dfederm Date: Wed, 6 Mar 2024 13:23:41 -0800 Subject: [PATCH] Consume requested targets when constructing the graph --- .azuredevops/pipelines/pr-ci.yml | 30 +++++--- .../pipelines/templates/variables.yml | 1 + CONTRIBUTING.md | 2 +- Directory.Packages.props | 14 ++-- src/Common/MSBuildCachePluginBase.cs | 4 +- src/Repack.Tests/RepackTests.cs | 45 ++++++------ src/SharedCompilation/VBCSCompilerReporter.cs | 70 +------------------ 7 files changed, 52 insertions(+), 114 deletions(-) diff --git a/.azuredevops/pipelines/pr-ci.yml b/.azuredevops/pipelines/pr-ci.yml index 86a11db..5774f7c 100644 --- a/.azuredevops/pipelines/pr-ci.yml +++ b/.azuredevops/pipelines/pr-ci.yml @@ -57,11 +57,12 @@ jobs: - checkout: self - - task: Cache@2 - displayName: Cache Visual Studio - inputs: - key: '"vs-release" | "$(Agent.OS)"' - path: $(VsInstallDir) + # Avoiding Caching VS for now as the installer command we're using doesn't work for upgrades + # - task: Cache@2 + # displayName: Cache Visual Studio + # inputs: + # key: '"vs-release" | "$(Agent.OS)"' + # path: $(VsInstallDir) - script: | del %TEMP%\vs_buildtools.exe @@ -78,6 +79,10 @@ jobs: del %TEMP%\vs_buildtools.exe + echo Copying installer logs + mkdir "$(LogDirectory)\VSSetup" + move "%TEMP%\dd_*" "$(LogDirectory)\VSSetup\" + echo Current MSBuild version: "$(MSBuildPath)" --version displayName: 'Install VS' @@ -105,11 +110,12 @@ jobs: - checkout: self - - task: Cache@2 - displayName: Cache Visual Studio - inputs: - key: '"vs-pre" | "$(Agent.OS)"' - path: $(VsInstallDir) + # Avoiding Caching VS for now as the installer command we're using doesn't work for upgrades + # - task: Cache@2 + # displayName: Cache Visual Studio + # inputs: + # key: '"vs-pre" | "$(Agent.OS)"' + # path: $(VsInstallDir) - script: | del %TEMP%\vs_buildtools.exe @@ -126,6 +132,10 @@ jobs: del %TEMP%\vs_buildtools.exe + echo Copying installer logs + mkdir "$(LogDirectory)\VSSetup" + move "%TEMP%\dd_*" "$(LogDirectory)\VSSetup\" + echo Current MSBuild version: "$(MSBuildPath)" --version displayName: 'Install VS Preview' diff --git a/.azuredevops/pipelines/templates/variables.yml b/.azuredevops/pipelines/templates/variables.yml index 2c85f8a..fd8e4a8 100644 --- a/.azuredevops/pipelines/templates/variables.yml +++ b/.azuredevops/pipelines/templates/variables.yml @@ -5,3 +5,4 @@ variables: ArtifactsDirectory: $(Build.ArtifactStagingDirectory)\artifacts # https://github.com/microsoft/azure-pipelines-agent/pull/4077 VSO_DEDUP_REDIRECT_TIMEOUT_IN_SEC: 5 + EnablePipelineCache: true \ No newline at end of file diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b274027..d43e5b6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -14,7 +14,7 @@ contact [opencode@microsoft.com](mailto:opencode@microsoft.com) with any additio ## Setup -It is assumed that you are using VS v17.8 or later. +It is assumed that you are using VS v17.9 or later. ## Building diff --git a/Directory.Packages.props b/Directory.Packages.props index 5af412d..37fd2d6 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -3,13 +3,13 @@ 19.232.34508-buildid25403274 0.1.0-20240121.1 - 17.8.3 + 17.9.5 - + @@ -39,14 +39,14 @@ - + - + - - - + + + diff --git a/src/Common/MSBuildCachePluginBase.cs b/src/Common/MSBuildCachePluginBase.cs index 233c1c8..9332e7d 100644 --- a/src/Common/MSBuildCachePluginBase.cs +++ b/src/Common/MSBuildCachePluginBase.cs @@ -273,8 +273,8 @@ private async Task BeginBuildInnerAsync(CacheContext context, PluginLoggerBase l Parser parser = new(logger, _repoRoot); IReadOnlyDictionary parserInfoForNodes = parser.Parse(graph); - // TODO: MSBuild should give this to us via CacheContext - var entryProjectTargets = Array.Empty(); + // In practice, the underlying type of the IReadOnlyCollection is a ICollection so attempt to cast first. We're not mutating the collection so still abiding by the readonly-ness. + ICollection entryProjectTargets = context.RequestedTargets as ICollection ?? new List(context.RequestedTargets); IReadOnlyDictionary> targetListPerNode = graph.GetTargetLists(entryProjectTargets); Dictionary nodeContexts = new(parserInfoForNodes.Count); diff --git a/src/Repack.Tests/RepackTests.cs b/src/Repack.Tests/RepackTests.cs index 7ac877d..b2e2504 100644 --- a/src/Repack.Tests/RepackTests.cs +++ b/src/Repack.Tests/RepackTests.cs @@ -5,7 +5,6 @@ using System.Collections.Generic; using System.IO; using System.Linq; -using System.Reflection; using Microsoft.MSBuildCache.AzureBlobStorage; using Microsoft.MSBuildCache.AzurePipelines; using Microsoft.MSBuildCache.Tests; @@ -17,33 +16,29 @@ namespace Microsoft.MSBuildCache.Repack.Tests; [TestClass] public class RepackTests { - private static readonly Type[] TypesToCheck = + [DataTestMethod] + [DataRow(typeof(MSBuildCacheAzureBlobStoragePlugin))] + [DataRow(typeof(MSBuildCacheAzurePipelinesPlugin))] + [DataRow(typeof(MSBuildCacheLocalPlugin))] + [DataRow(typeof(SharedCompilation.ResolveFileAccesses))] + public void PluginInterfaceAssembliesNotMerged(Type typeToCheck) { - typeof(MSBuildCacheAzureBlobStoragePlugin), - typeof(MSBuildCacheAzurePipelinesPlugin), - typeof(MSBuildCacheLocalPlugin), - typeof(SharedCompilation.ResolveFileAccesses), - }; +#if DEBUG + // Using Assert.Inconclusive instead of removing the entire test for visibility that the test exists. + Assert.Inconclusive("This test only applies to Release builds."); +#endif - [TestMethod] - public void PluginInterfaceAssembliesNotMerged() - { - foreach (Type type in TypesToCheck) - { - Dictionary references = type.Assembly - .GetReferencedAssemblies() - .Where(a => a.Name is not null) - .ToDictionary( - a => a.Name!, - a => a - ); + HashSet references = typeToCheck.Assembly + .GetReferencedAssemblies() + .Where(a => a.Name is not null) + .Select(reference => reference.Name!) + .ToHashSet(StringComparer.Ordinal); - // Check to make sure that each of the interface assemblies are still actually referenced - foreach (string expectedRefFileName in PluginInterfaceTypeCheckTests.PluginInterfaceNuGetAssemblies) - { - string expectedRef = Path.GetFileNameWithoutExtension(expectedRefFileName); - Assert.IsNotNull(references.FirstOrDefault(a => a.Value.FullName.IndexOf(expectedRef, StringComparison.Ordinal) > 0)); - } + // Check to make sure that each of the interface assemblies are still actually referenced + foreach (string expectedRefFileName in PluginInterfaceTypeCheckTests.PluginInterfaceNuGetAssemblies) + { + string expectedRef = Path.GetFileNameWithoutExtension(expectedRefFileName); + Assert.IsTrue(references.Contains(expectedRef)); } } } diff --git a/src/SharedCompilation/VBCSCompilerReporter.cs b/src/SharedCompilation/VBCSCompilerReporter.cs index ff3128b..2d647fe 100644 --- a/src/SharedCompilation/VBCSCompilerReporter.cs +++ b/src/SharedCompilation/VBCSCompilerReporter.cs @@ -9,8 +9,6 @@ #endif using System.IO; using System.Linq; -using System.Reflection; -using System.Reflection.Emit; using Microsoft.Build.Experimental.FileAccess; using Microsoft.Build.Framework; using Microsoft.CodeAnalysis; @@ -223,25 +221,10 @@ private sealed class AccessRegistrar { private delegate void ReportFileAccessFn(FileAccessData fileAccessData); - private delegate FileAccessData CreateFileAccessFn( - ReportedFileOperation operation, - RequestedAccess requestedAccess, - uint processId, - uint id, - uint correlationId, - uint error, - DesiredAccess desiredAccess, - FlagsAndAttributes flagsAndAttributes, - string path, - string processArgs, - bool isAnAugmentedFileAccess); - private readonly string _basePath; private readonly ReportFileAccessFn _reportFileAccess; - private readonly CreateFileAccessFn _createFileAccess; - private static readonly uint ProcessId = (uint) #if NET472 Process.GetCurrentProcess().Id; @@ -257,57 +240,6 @@ public AccessRegistrar(string basePath, EngineServices engineServices) // Use reflection to get the ReportFileAccess method since it's not exposed. // TODO: Once there is a proper API for this, use it. _reportFileAccess = (ReportFileAccessFn)Delegate.CreateDelegate(typeof(ReportFileAccessFn), engineServices, "ReportFileAccess"); - - ConstructorInfo fileAccessDataCtor = typeof(FileAccessData).GetConstructors()[0]; - ParameterInfo[] fileAccessDataCtorParams = fileAccessDataCtor.GetParameters(); - if (fileAccessDataCtorParams.Length == 9) - { - // This is the one we compiled against so just use it. - _createFileAccess = ( - ReportedFileOperation operation, - RequestedAccess requestedAccess, - uint processId, - uint id, - uint correlationId, - uint error, - DesiredAccess desiredAccess, - FlagsAndAttributes flagsAndAttributes, - string path, - string processArgs, - bool isAnAugmentedFileAccess) - => new FileAccessData(operation, requestedAccess, processId, error, desiredAccess, flagsAndAttributes, path, processArgs, isAnAugmentedFileAccess); - } - else if (fileAccessDataCtorParams.Length == 11) - { - // Adjust to a breaking change made in MSBuild to the FileAccessData ctor. See: https://github.com/dotnet/msbuild/pull/9615 - // Create a dynamic method which basically just invokes the ctor with all the params from the CreateFileAccessFn delegate. - DynamicMethod dynamicMethod = new( - name: string.Empty, - returnType: typeof(FileAccessData), - parameterTypes: fileAccessDataCtorParams.Select(param => param.ParameterType).ToArray(), - owner: typeof(FileAccessData)); - - ILGenerator il = dynamicMethod.GetILGenerator(); - il.Emit(OpCodes.Ldarg_0); - il.Emit(OpCodes.Ldarg_1); - il.Emit(OpCodes.Ldarg_2); - il.Emit(OpCodes.Ldarg_3); - il.Emit(OpCodes.Ldarg_S, 4); - il.Emit(OpCodes.Ldarg_S, 5); - il.Emit(OpCodes.Ldarg_S, 6); - il.Emit(OpCodes.Ldarg_S, 7); - il.Emit(OpCodes.Ldarg_S, 8); - il.Emit(OpCodes.Ldarg_S, 9); - il.Emit(OpCodes.Ldarg_S, 10); - il.Emit(OpCodes.Newobj, fileAccessDataCtor); - il.Emit(OpCodes.Ret); - - _createFileAccess = (CreateFileAccessFn)dynamicMethod.CreateDelegate(typeof(CreateFileAccessFn)); - } - else - { - throw new MissingMethodException("Could not find supported constructor for FileAccessData"); - } } public void RegisterOutput(string? filePath) => RegisterAccess(filePath, RequestedAccess.Write, DesiredAccess.GENERIC_WRITE); @@ -338,7 +270,7 @@ private void RegisterAccess(string? filePath, RequestedAccess requestedAccess, D return; } - FileAccessData fileAccessData = _createFileAccess( + FileAccessData fileAccessData = new( ReportedFileOperation.CreateFile, requestedAccess, ProcessId,