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

[RAR] Don't do I/O on SDK-provided references #8688

Merged
merged 7 commits into from
May 5, 2023
Merged
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
2 changes: 2 additions & 0 deletions documentation/wiki/ChangeWaves.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t
# Change Waves & Associated Features

## Current Rotation of Change Waves
### 17.8
- [[RAR] Don't do I/O on SDK-provided references](https://github.com/dotnet/msbuild/pull/8688)

### 17.6
- [Parse invalid property under target](https://github.com/dotnet/msbuild/pull/8190)
Expand Down
4 changes: 2 additions & 2 deletions src/Framework/ChangeWaves.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ internal enum ChangeWaveConversionState
/// For dev docs: https://github.com/dotnet/msbuild/blob/main/documentation/wiki/ChangeWaves-Dev.md
internal class ChangeWaves
{
internal static readonly Version Wave17_2 = new Version(17, 2);
internal static readonly Version Wave17_4 = new Version(17, 4);
internal static readonly Version Wave17_6 = new Version(17, 6);
internal static readonly Version[] AllWaves = { Wave17_2, Wave17_4, Wave17_6 };
internal static readonly Version Wave17_8 = new Version(17, 8);
internal static readonly Version[] AllWaves = { Wave17_4, Wave17_6, Wave17_8 };

/// <summary>
/// Special value indicating that all features behind all Change Waves should be enabled.
Expand Down
4 changes: 4 additions & 0 deletions src/Shared/Constants.cs
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,10 @@ internal static class ItemMetadataNames
internal const string subType = "SubType";
internal const string executableExtension = "ExecutableExtension";
internal const string embedInteropTypes = "EmbedInteropTypes";
internal const string frameworkReferenceName = "FrameworkReferenceName";
internal const string assemblyName = "AssemblyName";
internal const string assemblyVersion = "AssemblyVersion";
internal const string publicKeyToken = "PublicKeyToken";

/// <summary>
/// The output path for a given item.
Expand Down
57 changes: 56 additions & 1 deletion src/Tasks.UnitTests/AssemblyDependency/Miscellaneous.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Collections;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.IO;
using System.Linq;
Expand All @@ -11,13 +12,14 @@
using Microsoft.Build.Framework;
using Microsoft.Build.Shared;
using Microsoft.Build.Tasks;
using Microsoft.Build.UnitTests.Shared;
using Microsoft.Build.Tasks.AssemblyDependency;
using Microsoft.Build.Utilities;
using Microsoft.Win32;
using Shouldly;
using Xunit;
using Xunit.Abstractions;
using Xunit.NetCore.Extensions;
using FrameworkNameVersioning = System.Runtime.Versioning.FrameworkName;
using SystemProcessorArchitecture = System.Reflection.ProcessorArchitecture;

#nullable disable
Expand Down Expand Up @@ -8564,5 +8566,58 @@ private static void GenerateRedistAndProfileXmlLocations(string fullRedistConten

File.WriteAllText(profileRedistList, profileListContents);
}

[Fact]
public void SDKReferencesAreResolvedWithoutIO()
{
InitializeRARwithMockEngine(_output, out MockEngine mockEngine, out ResolveAssemblyReference rar);

string refPath = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString());
ladipro marked this conversation as resolved.
Show resolved Hide resolved

TaskItem item = new TaskItem(refPath);
item.SetMetadata("ExternallyResolved", "true");

item.SetMetadata("FrameworkReferenceName", "Microsoft.NETCore.App");
item.SetMetadata("FrameworkReferenceVersion", "8.0.0");

item.SetMetadata("AssemblyName", "System.Candy");
item.SetMetadata("AssemblyVersion", "8.1.2.3");
item.SetMetadata("PublicKeyToken", "b03f5f7f11d50a3a");

rar.Assemblies = new ITaskItem[] { item };
rar.SearchPaths = new string[]
{
"{CandidateAssemblyFiles}",
"{HintPathFromItem}",
"{TargetFrameworkDirectory}",
"{RawFileName}",
};
rar.WarnOrErrorOnTargetArchitectureMismatch = "Warning";

// Execute RAR and assert that we receive no I/O callbacks because the task gets what it needs from item metadata.
rar.Execute(
_ => throw new ShouldAssertException("Unexpected FileExists callback"),
directoryExists,
getDirectories,
_ => throw new ShouldAssertException("Unexpected GetAssemblyName callback"),
(string path, ConcurrentDictionary<string, AssemblyMetadata> assemblyMetadataCache, out AssemblyNameExtension[] dependencies, out string[] scatterFiles, out FrameworkNameVersioning frameworkName)
=> throw new ShouldAssertException("Unexpected GetAssemblyMetadata callback"),
#if FEATURE_WIN32_REGISTRY
getRegistrySubKeyNames,
getRegistrySubKeyDefaultValue,
#endif
_ => throw new ShouldAssertException("Unexpected GetLastWriteTime callback"),
_ => throw new ShouldAssertException("Unexpected GetAssemblyRuntimeVersion callback"),
#if FEATURE_WIN32_REGISTRY
openBaseKey,
#endif
checkIfAssemblyIsInGac,
isWinMDFile,
readMachineTypeFromPEHeader).ShouldBeTrue();

rar.ResolvedFiles.Length.ShouldBe(1);
rar.ResolvedFiles[0].ItemSpec.ShouldBe(refPath);
rar.ResolvedFiles[0].GetMetadata("FusionName").ShouldBe("System.Candy, Version=8.1.2.3, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a");
}
}
}
1 change: 1 addition & 0 deletions src/Tasks.UnitTests/HintPathResolver_Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ private bool ResolveHintPath(string hintPath)
sdkName: "",
rawFileNameCandidate: "FakeSystem.Net.Http",
isPrimaryProjectReference: true,
isImmutableFrameworkReference: false,
wantSpecificVersion: false,
executableExtensions: new string[] { ".winmd", ".dll", ".exe" },
hintPath: hintPath,
Expand Down
17 changes: 2 additions & 15 deletions src/Tasks/AssemblyDependency/AssemblyFoldersExResolver.cs
Original file line number Diff line number Diff line change
Expand Up @@ -183,26 +183,13 @@ private void LazyInitialize()
}
}

/// <summary>
/// Resolve a reference to a specific file name.
/// </summary>
/// <param name="assemblyName">The assemblyname of the reference.</param>
/// <param name="sdkName">The sdkname of the reference.</param>
/// <param name="rawFileNameCandidate">The reference's 'include' treated as a raw file name.</param>
/// <param name="isPrimaryProjectReference">Whether or not this reference was directly from the project file (and therefore not a dependency)</param>
/// <param name="wantSpecificVersion">Whether an exact version match is requested.</param>
/// <param name="executableExtensions">Allowed executable extensions.</param>
/// <param name="hintPath">The item's hintpath value.</param>
/// <param name="assemblyFolderKey">Like "hklm\Vendor RegKey" as provided to a reference by the &lt;AssemblyFolderKey&gt; on the reference in the project.</param>
/// <param name="assembliesConsideredAndRejected">Receives the list of locations that this function tried to find the assembly. May be "null".</param>
/// <param name="foundPath">The path where the file was found.</param>
/// <param name="userRequestedSpecificFile">Whether or not the user wanted a specific file (for example, HintPath is a request for a specific file)</param>
/// <returns>True if the file was resolved.</returns>
/// <inheritdoc/>
public override bool Resolve(
AssemblyNameExtension assemblyName,
string sdkName,
string rawFileNameCandidate,
bool isPrimaryProjectReference,
bool isImmutableFrameworkReference,
bool wantSpecificVersion,
string[] executableExtensions,
string hintPath,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -151,26 +151,13 @@ private void LazyInitialize()
}
}

/// <summary>
/// Resolve a reference to a specific file name.
/// </summary>
/// <param name="assemblyName">The assemblyname of the reference.</param>
/// <param name="sdkName">Not used by this type.</param>
/// <param name="rawFileNameCandidate">Not used by this type.</param>
/// <param name="isPrimaryProjectReference">Whether or not this reference was directly from the project file (and therefore not a dependency)</param>
/// <param name="wantSpecificVersion">Whether an exact version match is requested.</param>
/// <param name="executableExtensions">Allowed executable extensions.</param>
/// <param name="hintPath">Not used by this type.</param>
/// <param name="assemblyFolderKey">Not used by this type.</param>
/// <param name="assembliesConsideredAndRejected">Receives the list of locations that this function tried to find the assembly. May be "null".</param>
/// <param name="foundPath">The path where the file was found.</param>
/// <param name="userRequestedSpecificFile">Whether or not the user wanted a specific file (for example, HintPath is a request for a specific file)</param>
/// <returns>True if the file was resolved.</returns>
/// <inheritdoc/>
public override bool Resolve(
AssemblyNameExtension assemblyName,
string sdkName,
string rawFileNameCandidate,
bool isPrimaryProjectReference,
bool isImmutableFrameworkReference,
bool wantSpecificVersion,
string[] executableExtensions,
string hintPath,
Expand Down
17 changes: 2 additions & 15 deletions src/Tasks/AssemblyDependency/AssemblyFoldersResolver.cs
Original file line number Diff line number Diff line change
Expand Up @@ -27,26 +27,13 @@ public AssemblyFoldersResolver(string searchPathElement, GetAssemblyName getAsse
{
}

/// <summary>
/// Resolve a reference to a specific file name.
/// </summary>
/// <param name="assemblyName">The assemblyname of the reference.</param>
/// <param name="sdkName">The sdk name of the reference.</param>
/// <param name="rawFileNameCandidate">The reference's 'include' treated as a raw file name.</param>
/// <param name="isPrimaryProjectReference">Whether or not this reference was directly from the project file (and therefore not a dependency)</param>
/// <param name="wantSpecificVersion">Whether an exact version match is requested.</param>
/// <param name="executableExtensions">Allowed executable extensions.</param>
/// <param name="hintPath">The item's hintpath value.</param>
/// <param name="assemblyFolderKey">Like "hklm\Vendor RegKey" as provided to a reference by the &lt;AssemblyFolderKey&gt; on the reference in the project.</param>
/// <param name="assembliesConsideredAndRejected">Receives the list of locations that this function tried to find the assembly. May be "null".</param>
/// <param name="foundPath">The path where the file was found.</param>
/// <param name="userRequestedSpecificFile">Whether or not the user wanted a specific file (for example, HintPath is a request for a specific file)</param>
/// <returns>True if the file was resolved.</returns>
/// <inheritdoc/>
public override bool Resolve(
AssemblyNameExtension assemblyName,
string sdkName,
string rawFileNameCandidate,
bool isPrimaryProjectReference,
bool isImmutableFrameworkReference,
bool wantSpecificVersion,
string[] executableExtensions,
string hintPath,
Expand Down
3 changes: 3 additions & 0 deletions src/Tasks/AssemblyDependency/AssemblyResolution.cs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ internal static class AssemblyResolution
/// <param name="sdkName"></param>
/// <param name="rawFileNameCandidate">The file name to match if {RawFileName} is seen. (May be null).</param>
/// <param name="isPrimaryProjectReference">True if this is a primary reference directly from the project file.</param>
/// <param name="isImmutableFrameworkReference">True if <paramref name="rawFileNameCandidate"/> is immutable and guaranteed to exist.</param>
/// <param name="wantSpecificVersion"></param>
/// <param name="executableExtensions">The filename extension of the assembly. Must be this or its no match.</param>
/// <param name="hintPath">This reference's hintpath</param>
Expand All @@ -48,6 +49,7 @@ internal static string ResolveReference(
string sdkName,
string rawFileNameCandidate,
bool isPrimaryProjectReference,
bool isImmutableFrameworkReference,
Copy link
Member

Choose a reason for hiding this comment

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

We're currently saying this is only for immutable framework references, but do you think we might extend that later? If so, maybe we should use a somewhat more generic name?

Copy link
Member

Choose a reason for hiding this comment

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

I'd say what's interesting is not the immutability, but that we can assume its dependency closure is provided by the framework so don't need to explore it--right?

Copy link
Member

Choose a reason for hiding this comment

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

Fair...fullyUnderstoodReferences? Better name with that intent?

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'd say what's interesting is not the immutability, but that we can assume its dependency closure is provided by the framework so don't need to explore it--right?

Not quite right. The property of "dependency closure is provided" is signaled with the ExternallyResolved metadatum. It is a pre-existing behavior that for such assemblies we skip the dependency walk. It is happening here:

if (!reference.ExternallyResolved || FindDependenciesOfExternallyResolvedReferences)
{
// Look for dependent assemblies.
if (_findDependencies)
{
FindDependenciesAndScatterFiles(reference, newEntries);
}
}

This new flag means "the file is guaranteed to exist and never change" and in the Resolver hierarchy it's used only in RawFilenameResolver to skip the file existence check.

As for the name, I was on the fence between "immutable" and "framework" so I used both 😀

bool wantSpecificVersion,
string[] executableExtensions,
string hintPath,
Expand Down Expand Up @@ -79,6 +81,7 @@ internal static string ResolveReference(
sdkName,
rawFileNameCandidate,
isPrimaryProjectReference,
isImmutableFrameworkReference,
wantSpecificVersion,
executableExtensions,
hintPath,
Expand Down
17 changes: 2 additions & 15 deletions src/Tasks/AssemblyDependency/CandidateAssemblyFilesResolver.cs
Original file line number Diff line number Diff line change
Expand Up @@ -36,26 +36,13 @@ public CandidateAssemblyFilesResolver(string[] candidateAssemblyFiles, string se
_candidateAssemblyFiles = candidateAssemblyFiles;
}

/// <summary>
/// Resolve a reference to a specific file name.
/// </summary>
/// <param name="assemblyName">The assemblyname of the reference.</param>
/// <param name="sdkName"></param>
/// <param name="rawFileNameCandidate">The reference's 'include' treated as a raw file name.</param>
/// <param name="isPrimaryProjectReference">Whether or not this reference was directly from the project file (and therefore not a dependency)</param>
/// <param name="wantSpecificVersion">Whether an exact version match is requested.</param>
/// <param name="executableExtensions">Allowed executable extensions.</param>
/// <param name="hintPath">The item's hintpath value.</param>
/// <param name="assemblyFolderKey">Like "hklm\Vendor RegKey" as provided to a reference by the &lt;AssemblyFolderKey&gt; on the reference in the project.</param>
/// <param name="assembliesConsideredAndRejected">Receives the list of locations that this function tried to find the assembly. May be "null".</param>
/// <param name="foundPath">The path where the file was found.</param>
/// <param name="userRequestedSpecificFile">Whether or not the user wanted a specific file (for example, HintPath is a request for a specific file)</param>
/// <returns>True if the file was resolved.</returns>
/// <inheritdoc/>
public override bool Resolve(
AssemblyNameExtension assemblyName,
string sdkName,
string rawFileNameCandidate,
bool isPrimaryProjectReference,
bool isImmutableFrameworkReference,
bool wantSpecificVersion,
string[] executableExtensions,
string hintPath,
Expand Down
17 changes: 2 additions & 15 deletions src/Tasks/AssemblyDependency/DirectoryResolver.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,26 +22,13 @@ public DirectoryResolver(string searchPathElement, GetAssemblyName getAssemblyNa
{
}

/// <summary>
/// Resolve a reference to a specific file name.
/// </summary>
/// <param name="assemblyName">The assemblyname of the reference.</param>
/// <param name="sdkName"></param>
/// <param name="rawFileNameCandidate">The reference's 'include' treated as a raw file name.</param>
/// <param name="isPrimaryProjectReference">Whether or not this reference was directly from the project file (and therefore not a dependency)</param>
/// <param name="wantSpecificVersion">Whether an exact version match is requested.</param>
/// <param name="executableExtensions">Allowed executable extensions.</param>
/// <param name="hintPath">The item's hintpath value.</param>
/// <param name="assemblyFolderKey">Like "hklm\Vendor RegKey" as provided to a reference by the &lt;AssemblyFolderKey&gt; on the reference in the project.</param>
/// <param name="assembliesConsideredAndRejected">Receives the list of locations that this function tried to find the assembly. May be "null".</param>
/// <param name="foundPath">The path where the file was found.</param>
/// <param name="userRequestedSpecificFile">Whether or not the user wanted a specific file (for example, HintPath is a request for a specific file)</param>
/// <returns>True if the file was resolved.</returns>
/// <inheritdoc/>
public override bool Resolve(
AssemblyNameExtension assemblyName,
string sdkName,
string rawFileNameCandidate,
bool isPrimaryProjectReference,
bool isImmutableFrameworkReference,
bool wantSpecificVersion,
string[] executableExtensions,
string hintPath,
Expand Down
17 changes: 2 additions & 15 deletions src/Tasks/AssemblyDependency/FrameworkPathResolver.cs
Original file line number Diff line number Diff line change
Expand Up @@ -30,26 +30,13 @@ public FrameworkPathResolver(string[] frameworkPaths, InstalledAssemblies instal
_installedAssemblies = installedAssemblies;
}

/// <summary>
/// Resolve a reference to a specific file name.
/// </summary>
/// <param name="assemblyName">The assemblyname of the reference.</param>
/// <param name="sdkName"></param>
/// <param name="rawFileNameCandidate">The reference's 'include' treated as a raw file name.</param>
/// <param name="isPrimaryProjectReference">Whether or not this reference was directly from the project file (and therefore not a dependency)</param>
/// <param name="wantSpecificVersion">Whether an exact version match is requested.</param>
/// <param name="executableExtensions">Allowed executable extensions.</param>
/// <param name="hintPath">The item's hintpath value.</param>
/// <param name="assemblyFolderKey">Like "hklm\Vendor RegKey" as provided to a reference by the &lt;AssemblyFolderKey&gt; on the reference in the project.</param>
/// <param name="assembliesConsideredAndRejected">Receives the list of locations that this function tried to find the assembly. May be "null".</param>
/// <param name="foundPath">The path where the file was found.</param>
/// <param name="userRequestedSpecificFile">Whether or not the user wanted a specific file (for example, HintPath is a request for a specific file)</param>
/// <returns>True if the file was resolved.</returns>
/// <inheritdoc/>
public override bool Resolve(
AssemblyNameExtension assemblyName,
string sdkName,
string rawFileNameCandidate,
bool isPrimaryProjectReference,
bool isImmutableFrameworkReference,
bool wantSpecificVersion,
string[] executableExtensions,
string hintPath,
Expand Down
Loading