[APICompat] Don't filter package assets and handle placeholder files #27822
Conversation
ViktorHofer
requested review from
ericstj,
a team and
joperezr
as code owners
ViktorHofer
changed the title
ViktorHofer
force-pushed
the
PackageValidationPlaceholder
branch
from
e4f6e20 to
932d183
Compare
|
Old PR review: This PR is more than a year old and does not appear to have active engagement. A reviewer has been notified offline to review and take action. If no action is taken, this PR will be closed in 14 days. Please comment if you want this PR left open for further investigation. |
|
Yes, let me close this an get back to it when I have more time for APICompat. |
ViktorHofer
force-pushed
the
PackageValidationPlaceholder
branch
from
932d183 to
379d728
Compare
Fixes dotnet#18165 Instead of manually filtering package assets, let NuGet calculate the runtime and compile time assets and handle placeholder files which are returned by NuGet.
ViktorHofer
force-pushed
the
PackageValidationPlaceholder
branch
from
379d728 to
874b4b6
Compare
| } | ||
|
|
||
| // Make sure placeholder package files aren't enqueued as api comparison check work items. | ||
| Debug.Assert(!leftContentItems.IsPlaceholderFile() && !rightContentItems.IsPlaceholderFile(), |
There was a problem hiding this comment.
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.
| @@ -31,6 +32,19 @@ internal static class ApiCompatRunnerExtensions | |||
| return; | |||
| } | |||
|
|
|||
| // Don't perform api compatibility checks on placeholder files. | |||
| if (leftContentItems.IsPlaceholderFile() && rightContentItems.IsPlaceholderFile()) | |||
There was a problem hiding this comment.
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.
| { | ||
| 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. |
There was a problem hiding this comment.
In strict mode I think we would want to flag this.
| @@ -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(); | |||
There was a problem hiding this comment.
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.
Fixes #18165
Instead of manually filtering package assets, let NuGet calculate the runtime and compile time assets and handle placeholder files which are returned by NuGet.
TODO: