Skip to content

[APICompat - PackageValidation] Enable baseline assembly references #32930

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jun 1, 2023
Merged

[APICompat - PackageValidation] Enable baseline assembly references #32930

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
81c3bd7
[APICompat - PackageValidation] Enable baseline assembly references
ViktorHofer May 31, 2023
9a0ecc8
Remove unused param
ViktorHofer May 31, 2023
c1172a2
Use already calculated NuGet matching
ViktorHofer Jun 1, 2023
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
4 changes: 1 addition & 3 deletions src/ApiCompat/Microsoft.DotNet.ApiCompatibility/Runner/ApiCompatRunner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,8 @@ private IReadOnlyList<ElementContainer<IAssemblySymbol>> CreateAssemblySymbols(I
ApiCompatRunnerOptions options,
out bool resolvedExternallyProvidedAssemblyReferences)
{
// In order to enable reference support for baseline suppression we need a better way
// to resolve references for the baseline package. Let's not enable it for now.
string[] aggregatedReferences = metadataInformation.Where(m => m.References != null).SelectMany(m => m.References!).Distinct().ToArray();
resolvedExternallyProvidedAssemblyReferences = !options.IsBaselineComparison && aggregatedReferences.Length > 0;
resolvedExternallyProvidedAssemblyReferences = aggregatedReferences.Length > 0;

IAssemblySymbolLoader loader = _assemblySymbolLoaderFactory.Create(resolvedExternallyProvidedAssemblyReferences);
if (resolvedExternallyProvidedAssemblyReferences)
Expand Down
49 changes: 31 additions & 18 deletions src/ApiCompat/Microsoft.DotNet.PackageValidation/ApiCompatRunnerExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using Microsoft.DotNet.ApiCompatibility;
using Microsoft.DotNet.ApiCompatibility.Logging;
Expand All @@ -25,21 +26,15 @@ public static void QueueApiCompatFromContentItem(this IApiCompatRunner apiCompat
Package leftPackage,
Package? rightPackage = null)
{
Debug.Assert(leftContentItems.Count > 0);
Debug.Assert(rightContentItems.Count > 0);

// Don't enqueue duplicate items (if no right package is supplied and items match)
if (rightPackage == null && ContentItemCollectionEquals(leftContentItems, rightContentItems))
if (rightPackage is null && ContentItemCollectionEquals(leftContentItems, rightContentItems))
{
return;
}

MetadataInformation[] left = new MetadataInformation[leftContentItems.Count];
for (int leftIndex = 0; leftIndex < leftContentItems.Count; leftIndex++)
{
left[leftIndex] = GetMetadataInformation(log,
leftPackage,
leftContentItems[leftIndex],
options.IsBaselineComparison ? Resources.Baseline + " " + leftContentItems[leftIndex].Path : null);
}

MetadataInformation[] right = new MetadataInformation[rightContentItems.Count];
for (int rightIndex = 0; rightIndex < rightContentItems.Count; rightIndex++)
{
Expand All @@ -48,28 +43,46 @@ public static void QueueApiCompatFromContentItem(this IApiCompatRunner apiCompat
rightContentItems[rightIndex]);
}

MetadataInformation[] left = new MetadataInformation[leftContentItems.Count];
for (int leftIndex = 0; leftIndex < leftContentItems.Count; leftIndex++)
{
left[leftIndex] = GetMetadataInformation(log,
leftPackage,
leftContentItems[leftIndex],
displayString: options.IsBaselineComparison ? Resources.Baseline + " " + leftContentItems[leftIndex].Path : null,
// Use the assembly references from the right package if the left package doesn't provide them.
assemblyReferences: rightPackage is not null ? right[right.Length > leftIndex ? leftIndex : 0].References : null);
Copy link
Member

Choose a reason for hiding this comment

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

Are the references actually specific to some index here? I thought this index was iterating through multiple content items from the package, multiple assemblies in the same folder, so I would expect all the references to be the same for a given call of this method.

Ultimately it looks like a work item loads all assemblies in the same loader (CSharpCompilation), so associating references with individual content items is probably unnecessary overhead:
https://github.com/dotnet/sdk/blob/5f6b9be62ba8952410d2b181fe5a2ac3fee4c03a/src/ApiCompat/Microsoft.DotNet.ApiCompatibility/Runner/ApiCompatRunner.cs#LL123C31-L123C31

Copy link
Member Author

Choose a reason for hiding this comment

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

It's unnecessary, yes but I felt like keeping the index in sync (as in most cases left and right count is in sync) makes more sense than just always defaulting to the first item.

Copy link
Member

Choose a reason for hiding this comment

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

Fair, but it was confusing to me trying to understand the relationship between the indexes of these two lists. I was trying to understand why we would want to align them but would also be OK with just substituting the first. Perhaps a future refactoring might move references up to the work-item instead of associate with the individual files.

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 will remove that indexing as you are right that it's unnecessary overhead and adds complexity.

Ultimately it looks like a work item loads all assemblies in the same loader (CSharpCompilation), so associating references with individual content items is probably unnecessary overhead:

Just to clarify, a workitem doesn't share the compilation context as this would otherwise result in reference assembly clashes. The individual elements (lefts and rights) each create a compilation context and load the assemblies into that container. Not relevant for this discussion but this domain has some perf optimization potentials. I.e. the assembly symbol loader could respect assembly versions (and public key tokens) and cache the loaded assembly symbols.

}

apiCompatRunner.EnqueueWorkItem(new ApiCompatRunnerWorkItem(left, options, right));
}

private static MetadataInformation GetMetadataInformation(ISuppressableLog log,
Package package,
ContentItem item,
string? displayString = null)
string? displayString = null,
IEnumerable<string>? assemblyReferences = null)
{
displayString ??= item.Path;
string[]? assemblyReferences = null;

if (item.Properties.TryGetValue("tfm", out object? tfmObj))
{
string targetFramework = ((NuGetFramework)tfmObj).GetShortFolderName();

if (package.AssemblyReferences != null && !package.AssemblyReferences.TryGetValue(targetFramework, out assemblyReferences))
if (package.AssemblyReferences != null)
{
log.LogWarning(new Suppression(DiagnosticIds.SearchDirectoriesNotFoundForTfm) { Target = displayString },
DiagnosticIds.SearchDirectoriesNotFoundForTfm,
string.Format(Resources.MissingSearchDirectory,
targetFramework,
displayString));
if (package.AssemblyReferences.TryGetValue(targetFramework, out string[]? assemblyReferencesRaw))
{
assemblyReferences = assemblyReferencesRaw;
}
else
{
log.LogWarning(new Suppression(DiagnosticIds.SearchDirectoriesNotFoundForTfm) { Target = displayString },
DiagnosticIds.SearchDirectoriesNotFoundForTfm,
string.Format(Resources.MissingSearchDirectory,
targetFramework,
displayString));
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/ApiCompat/Microsoft.DotNet.PackageValidation/Package.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ public static Package Create(string? packagePath, Dictionary<string, string[]>?
}

/// <summary>
/// Finds the best compile time assset for a specific framework.
/// Finds the best compile time asset for a specific framework.
/// </summary>
/// <param name="framework">The framework where the package needs to be installed.</param>
/// <returns>A ContentItem representing the best compile time asset.</returns>
Expand Down