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.

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

[APICompat] Don't filter package assets and handle placeholder files #27822

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

[APICompat] Don't filter package assets and handle placeholder files #27822

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/Compatibility/ApiCompat/Microsoft.DotNet.PackageValidation/ApiCompatRunnerExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using Microsoft.DotNet.ApiCompatibility;
using Microsoft.DotNet.ApiCompatibility.Logging;
using Microsoft.DotNet.ApiCompatibility.Runner;
using Microsoft.DotNet.ApiSymbolExtensions.Logging;
using NuGet.ContentModel;
using NuGet.Frameworks;

Expand All @@ -31,6 +32,19 @@ public static void QueueApiCompatFromContentItem(this IApiCompatRunner apiCompat
return;
}

// Don't perform api compatibility checks on placeholder files.
if (leftContentItems.IsPlaceholderFile() && rightContentItems.IsPlaceholderFile())
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the condition here should be if left is a placeholder and we're not in strict mode?

When in strict mode, then if left is placeholder right must also be placeholder.

Perhaps we cover these cases by just fleshing out the comparisons with tests involving placeholders.

{
leftContentItems[0].Properties.TryGetValue("tfm", out object? tfmRaw);
string tfm = tfmRaw?.ToString() ?? string.Empty;

log.LogMessage(MessageImportance.Normal, string.Format(Resources.SkipApiCompatForPlaceholderFiles, tfm));
}

// Make sure placeholder package files aren't enqueued as api comparison check work items.
Debug.Assert(!leftContentItems.IsPlaceholderFile() && !rightContentItems.IsPlaceholderFile(),
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is an accurate assert. What if a package explicitly adds a placeholder in a compatible framework?

It might do this if a) the library became part of that framework and we could compare the package library to the framework library (in references) or b) as an intentional breaking change in which case we should still compare and raise the missing file.

"Placeholder files must not be enqueued for api comparison checks.");

// If a right hand side package is provided in addition to the left side, package validation runs in baseline comparison mode.
// The assumption stands that the right hand side is "current" and has more information than the left, i.e. assembly references.
// If the left package doesn't provide assembly references, fallback to the references from the right side if the TFMs are compatible.
Expand Down
3 changes: 1 addition & 2 deletions src/Compatibility/ApiCompat/Microsoft.DotNet.PackageValidation/Package.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
using NuGet.ContentModel;
using NuGet.Frameworks;
using NuGet.Packaging;
using NuGet.Packaging.Core;
using NuGet.RuntimeModel;

namespace Microsoft.DotNet.PackageValidation
Expand Down Expand Up @@ -136,7 +135,7 @@ public static Package Create(string? packagePath, IReadOnlyDictionary<NuGetFrame
NuspecReader nuspecReader = packageReader.NuspecReader;
string packageId = nuspecReader.GetId();
string version = nuspecReader.GetVersion().ToString();
IEnumerable<string> packageAssets = packageReader.GetFiles().Where(t => t.EndsWith(".dll")).ToArray();
IEnumerable<string> packageAssets = packageReader.GetFiles().ToArray();
Copy link
Member

Choose a reason for hiding this comment

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

Make sure you add a couple tests to ensure we can handle the other file types that NuGet will give us.
https://github.com/NuGet/NuGet.Client/blob/46f80a3f1a83f3c5f33a20fa9fb72871e1e2d4eb/src/NuGet.Core/NuGet.Packaging/ContentModel/ManagedCodeConventions.cs#L28

I wonder if we should ignore exe, or allow folks to specify something to ignore exe's? I guess they could add a suppression to ignore all things from the comparison of an exe.


return new Package(packagePath!, packageId, version, packageAssets, packageAssemblyReferences);
}
Expand Down
9 changes: 9 additions & 0 deletions src/Compatibility/ApiCompat/Microsoft.DotNet.PackageValidation/PackageExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,21 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using NuGet.ContentModel;
using NuGet.Frameworks;

namespace Microsoft.DotNet.PackageValidation
{
internal static class PackageExtensions
{
private const string PlaceholderFile = "_._";

public static bool IsPlaceholderFile(this ContentItem contentItem) =>
Path.GetFileName(contentItem.Path) == PlaceholderFile;

public static bool IsPlaceholderFile(this IReadOnlyList<ContentItem> contentItems) =>
contentItems.Count == 1 && contentItems[0].IsPlaceholderFile();

public static bool SupportsRuntimeIdentifier(this NuGetFramework tfm, string rid) =>
tfm.Framework != ".NETFramework" || rid.StartsWith("win", StringComparison.OrdinalIgnoreCase);
}
Expand Down
59 changes: 31 additions & 28 deletions src/Compatibility/ApiCompat/Microsoft.DotNet.PackageValidation/Resources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
<?xml version="1.0" encoding="utf-8"?>
<root>
<!--
Microsoft ResX Schema
<!--
Microsoft ResX Schema

Version 2.0
The primary goals of this format is to allow a simple XML format
that is mostly human readable. The generation and parsing of the
various data types are done through the TypeConverter classes

The primary goals of this format is to allow a simple XML format
that is mostly human readable. The generation and parsing of the
various data types are done through the TypeConverter classes
associated with the data types.

Example:

... ado.net/XML headers & schema ...
<resheader name="resmimetype">text/microsoft-resx</resheader>
<resheader name="version">2.0</resheader>
Expand All @@ -26,36 +26,36 @@
<value>[base64 mime encoded string representing a byte array form of the .NET Framework object]</value>
<comment>This is a comment</comment>
</data>
There are any number of "resheader" rows that contain simple

There are any number of "resheader" rows that contain simple
name/value pairs.
Each data row contains a name, and value. The row also contains a
type or mimetype. Type corresponds to a .NET class that support
text/value conversion through the TypeConverter architecture.
Classes that don't support this are serialized and stored with the

Each data row contains a name, and value. The row also contains a
type or mimetype. Type corresponds to a .NET class that support
text/value conversion through the TypeConverter architecture.
Classes that don't support this are serialized and stored with the
mimetype set.
The mimetype is used for serialized objects, and tells the
ResXResourceReader how to depersist the object. This is currently not

The mimetype is used for serialized objects, and tells the
ResXResourceReader how to depersist the object. This is currently not
extensible. For a given mimetype the value must be set accordingly:
Note - application/x-microsoft.net.object.binary.base64 is the format
that the ResXResourceWriter will generate, however the reader can

Note - application/x-microsoft.net.object.binary.base64 is the format
that the ResXResourceWriter will generate, however the reader can
read any of the formats listed below.

mimetype: application/x-microsoft.net.object.binary.base64
value : The object must be serialized with
value : The object must be serialized with
: System.Runtime.Serialization.Formatters.Binary.BinaryFormatter
: and then encoded with base64 encoding.

mimetype: application/x-microsoft.net.object.soap.base64
value : The object must be serialized with
value : The object must be serialized with
: System.Runtime.Serialization.Formatters.Soap.SoapFormatter
: and then encoded with base64 encoding.

mimetype: application/x-microsoft.net.object.bytearray.base64
value : The object must be serialized into a byte array
value : The object must be serialized into a byte array
: using a System.ComponentModel.TypeConverter
: and then encoded with base64 encoding.
-->
Expand Down Expand Up @@ -150,4 +150,7 @@
<data name="BaselineTargetFrameworkIgnoredButPresentInCurrentPackage" xml:space="preserve">
<value>Target framework '{0}' in the baseline package is ignored but exists in the current package.</value>
</data>
</root>
<data name="SkipApiCompatForPlaceholderFiles" xml:space="preserve">
<value>Skipping api compatiblity check for target framework {0} as the package assets are placeholder files.</value>
</data>
</root>
33 changes: 29 additions & 4 deletions ...ility/ApiCompat/Microsoft.DotNet.PackageValidation/Validators/BaselinePackageValidator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,21 @@ public void Validate(PackageValidatorOption options)
{
// Search for compatible compile time assets in the latest package.
IReadOnlyList<ContentItem>? latestCompileAssets = options.Package.FindBestCompileAssetForFramework(baselineTargetFramework);
if (latestCompileAssets == null)
// Emit an error if
// - No latest compile time asset is available or
// - The latest compile time asset is a placeholder but the baseline compile time asset isn't.
if (latestCompileAssets == null ||
(latestCompileAssets.IsPlaceholderFile() && !baselineCompileAssets.IsPlaceholderFile()))
{
log.LogError(new Suppression(DiagnosticIds.TargetFrameworkDropped) { Target = baselineTargetFramework.ToString() },
DiagnosticIds.TargetFrameworkDropped,
string.Format(Resources.MissingTargetFramework,
baselineTargetFramework));
}
else if (baselineCompileAssets.IsPlaceholderFile() && !latestCompileAssets.IsPlaceholderFile())
{
// Ignore the newly added compile time asset in the latest package.
Copy link
Member

Choose a reason for hiding this comment

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

In strict mode I think we would want to flag this.

}
else if (options.EnqueueApiCompatWorkItems)
{
apiCompatRunner.QueueApiCompatFromContentItem(log,
Expand All @@ -67,13 +75,21 @@ public void Validate(PackageValidatorOption options)
{
// Search for compatible runtime assets in the latest package.
IReadOnlyList<ContentItem>? latestRuntimeAssets = options.Package.FindBestRuntimeAssetForFramework(baselineTargetFramework);
if (latestRuntimeAssets == null)
// Emit an error if
// - No latest runtime asset is available or
// - The latest runtime asset is a placeholder but the baseline runtime asset isn't.
if (latestRuntimeAssets == null ||
(latestRuntimeAssets.IsPlaceholderFile() && !baselineRuntimeAssets.IsPlaceholderFile()))
{
log.LogError(new Suppression(DiagnosticIds.TargetFrameworkDropped) { Target = baselineTargetFramework.ToString() },
DiagnosticIds.TargetFrameworkDropped,
string.Format(Resources.MissingTargetFramework,
baselineTargetFramework));
}
else if (baselineRuntimeAssets.IsPlaceholderFile() && !latestRuntimeAssets.IsPlaceholderFile())
{
// Ignore the newly added run time asset in the latest package.
}
else if (options.EnqueueApiCompatWorkItems)
{
apiCompatRunner.QueueApiCompatFromContentItem(log,
Expand All @@ -95,19 +111,28 @@ public void Validate(PackageValidatorOption options)

foreach (IGrouping<string, ContentItem> baselineRuntimeSpecificAssetsRidGroup in baselineRuntimeSpecificAssetsRidGroupedPerRid)
{
IReadOnlyList<ContentItem> baselineRuntimeSpecificAssetsForRid = baselineRuntimeSpecificAssetsRidGroup.ToArray();
IReadOnlyList<ContentItem>? latestRuntimeSpecificAssets = options.Package.FindBestRuntimeAssetForFrameworkAndRuntime(baselineTargetFramework, baselineRuntimeSpecificAssetsRidGroup.Key);
if (latestRuntimeSpecificAssets == null)
// Emit an error if
// - No latest runtime specific asset is available or
// - The latest runtime specific asset is a placeholder but the baseline runtime specific asset isn't.
if (latestRuntimeSpecificAssets == null ||
(latestRuntimeSpecificAssets.IsPlaceholderFile() && !baselineRuntimeSpecificAssetsForRid.IsPlaceholderFile()))
{
log.LogError(new Suppression(DiagnosticIds.TargetFrameworkAndRidPairDropped) { Target = baselineTargetFramework.ToString() + "-" + baselineRuntimeSpecificAssetsRidGroup.Key },
DiagnosticIds.TargetFrameworkAndRidPairDropped,
string.Format(Resources.MissingTargetFrameworkAndRid,
baselineTargetFramework,
baselineRuntimeSpecificAssetsRidGroup.Key));
}
else if (baselineRuntimeSpecificAssetsForRid.IsPlaceholderFile() && !latestRuntimeSpecificAssets.IsPlaceholderFile())
{
// Ignore the newly added runtime specific asset in the latest package.
}
else if (options.EnqueueApiCompatWorkItems)
{
apiCompatRunner.QueueApiCompatFromContentItem(log,
baselineRuntimeSpecificAssetsRidGroup.ToArray(),
baselineRuntimeSpecificAssetsForRid,
latestRuntimeSpecificAssets,
apiCompatOptions,
options.BaselinePackage,
Expand Down
17 changes: 8 additions & 9 deletions ...at/Microsoft.DotNet.PackageValidation/Validators/CompatibleFrameworkInPackageValidator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,29 +40,28 @@ public void Validate(PackageValidatorOption options)
foreach (NuGetFramework framework in options.Package.FrameworksInPackage.OrderByDescending(f => f.Version))
{
IReadOnlyList<ContentItem>? compileTimeAsset = options.Package.FindBestCompileAssetForFramework(framework);
if (compileTimeAsset != null)
if (compileTimeAsset != null && !compileTimeAsset.IsPlaceholderFile())
{
compileAssetsQueue.Enqueue((framework, compileTimeAsset));
}
}

while (compileAssetsQueue.Count > 0)
// Iterate as long as assets are available for comparison.
while (compileAssetsQueue.Count > 1)
{
(NuGetFramework framework, IReadOnlyList<ContentItem> compileTimeAsset) = compileAssetsQueue.Dequeue();

// If no assets are available for comparison, stop the iteration.
if (compileAssetsQueue.Count == 0) break;

SelectionCriteria managedCriteria = conventions.Criteria.ForFramework(framework);

ContentItemCollection contentItemCollection = new();
// The collection won't contain the current compile time asset as it is already dequeued.
contentItemCollection.Load(compileAssetsQueue.SelectMany(a => a.Item2).Select(a => a.Path));

// Search for a compatible compile time asset and compare it.
IList<ContentItem>? compatibleFrameworkAsset = contentItemCollection.FindBestItemGroup(managedCriteria, patternSet)?.Items;
if (compatibleFrameworkAsset != null)
IList<ContentItem>? compatibleCompileTimeAsset = contentItemCollection.FindBestItemGroup(managedCriteria, patternSet)?.Items;
if (compatibleCompileTimeAsset != null)
{
apiCompatRunner.QueueApiCompatFromContentItem(log,
new ReadOnlyCollection<ContentItem>(compatibleFrameworkAsset),
new ReadOnlyCollection<ContentItem>(compatibleCompileTimeAsset),
compileTimeAsset,
apiCompatOptions,
options.Package);
Expand Down
Loading
Loading