Skip to content

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

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

Jump to bottom
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

wants to merge 1 commit into from

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Sep 12, 2022 •
edited
Loading

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:

  • Add tests for packages with placeholder files.

@ViktorHofer ViktorHofer self-assigned this Sep 12, 2022
@ViktorHofer ViktorHofer requested review from ericstj, a team and joperezr as code owners September 12, 2022 14:40
@ViktorHofer ViktorHofer changed the title Don't filter package assets and handle placeholder [APICompat] Don't filter package assets and handle placeholder files Sep 12, 2022
@ViktorHofer ViktorHofer marked this pull request as draft September 12, 2022 15:36
@ViktorHofer ViktorHofer force-pushed the PackageValidationPlaceholder branch from e4f6e20 to 932d183 Compare September 13, 2022 17:40
@marcpopMSFT
Copy link
Member

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.

@ViktorHofer
Copy link
Member Author

Yes, let me close this an get back to it when I have more time for APICompat.

@ViktorHofer ViktorHofer reopened this Jul 24, 2024
@ViktorHofer ViktorHofer force-pushed the PackageValidationPlaceholder branch from 932d183 to 379d728 Compare July 24, 2024 13:05
@ViktorHofer
Don't filter package assets and handle placeholder
874b4b6 
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 ViktorHofer force-pushed the PackageValidationPlaceholder branch from 379d728 to 874b4b6 Compare July 24, 2024 13:15
ericstj
ericstj reviewed Jul 24, 2024
View reviewed changes
src/Compatibility/ApiCompat/Microsoft.DotNet.PackageValidation/ApiCompatRunnerExtensions.cs
}

// 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.