[APICompat - PackageValidation] Enable baseline assembly references #32930

ViktorHofer · 2023-05-31T13:45:38Z

If assembly references aren't provided for the baseline package via the MSBuild task or the CLI frontend, use the current package's assembly references as an approximation.

As current assembly references are grouped by TFMs, use the nearest compatible TFM for which assembly references are provided. This guarantees that assembly references from current are used even if TFMs don't match.

@ericstj what do you think about the assembly reference change related to picking the nearest compatible TFM? Do we want a baseline package with a lib/net7.0/a.dll asset to use the assembly references from the current package lib/net8.0/a.dll asset? Note that the current assembly symbol loader ignored versions.

ghost · 2023-05-31T13:45:45Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

ericstj · 2023-05-31T16:36:54Z

@ericstj what do you think about the assembly reference change related to picking the nearest compatible TFM?

What I was imagining we'd do was use the right-hand-side dependencies when the left-hand-side dependencies were missing. This was what we did previously with API Compat.

Nearest might be OK - so long as you're going forward and not back (I always get mixed up with NuGet's terms around this), since the nearest forward should be compatible and should be a parallel operation to the compatibility tuples we're creating between frameworks.

ericstj

Can we add a test case for this somehow?

ViktorHofer · 2023-05-31T17:20:15Z

What I was imagining we'd do was use the right-hand-side dependencies when the left-hand-side dependencies were missing. This was what we did previously with API Compat.

Yes, that's what this PR is doing.

Nearest might be OK - so long as you're going forward and not back (I always get mixed up with NuGet's terms around this), since the nearest forward should be compatible and should be a parallel operation to the compatibility tuples we're creating between frameworks.

I'm slightly confused what you refer to with going back. The current package (right hand side) usually uses the same or a newer TFM and presumably rarely an older TFM. In my example above, would we want to allow the following behavior or not (?):

Do we want a baseline package with a lib/net7.0/a.dll asset to use the assembly references from the current package lib/net8.0/a.dll asset?

...

Can we add a test case for this somehow?

Non trivial as many of our package validation integration tests are currently disabled because of #23533. I'm unsure if I can come up with a new test without deeply looking and with that fixing the test issue. I did run my changes on top off the msbuild change and the error went away. I also verified in the debugger that the assembly references are considered on the left hand side - as expected.

ericstj · 2023-05-31T21:16:32Z

Yes, that's what this PR is doing.

Not quite, it's computing which references to use independently based on the TFM of each side. I meant something more specific. Given a tuple comparing {A} to {B, b-references} use b-references for A -- without any calculation involving TFM at all. Its a bit mechanical and leaves less up to chance. For example, in this method:
https://github.com/dotnet/sdk/blob/e0a643139aa4ec61f8953f36ff3cd0988bbbce51/src/ApiCompat/Microsoft.DotNet.PackageValidation/ApiCompatRunnerExtensions.cs#LL20C28-L20C57
All the content items on the right would have the same TFM and you could pass that down to GetMetadataInformation and use it in case the left-side TFM didn't exist in package.AssemblyReferences. Maybe as a "fallbackReferenceFramework" parameter?

would we want to allow the following behavior or not

Yeah, we'd want that behavior. I'm agreeing with you -- I'm just asking to double check that NuGetFrameworkUtility.GetNearest is doing the right thing here. I can never remember which direction that API searches. Have a look at this tool to illustrate what we don't want to do: https://nugettools.azurewebsites.net/6.4.0/get-nearest-framework?project=net7.0&package=net6.0%0D%0Anet8.0. Regardless, if you can avoid doing an extra calculation here I think that would be better.

ViktorHofer · 2023-06-01T09:50:03Z

Not quite, it's computing which references to use independently based on the TFM of each side. I meant something more specific. Given a tuple comparing {A} to {B, b-references} use b-references for A -- without any calculation involving TFM at all. Its a bit mechanical and leaves less up to chance.

What makes this hard with the current architecture is that the component that loads symbols and compares left against rights does have a 1:n relationship. A single left is often compared against multiple rights. As an example:

Package	TFMs
Baseline	netstandard2.0
Current	netstandard2.0, net7.0

APICompat / PackageValidation validates the [Baseline] netstandard2.0 asset against the netstandard2.0 and net7.0 assets. That design is implemented in all ApiCompatibility.dll components: ApiCompatRunner, ApiComparer, RuleRunner except for the rules themselves which operate on symbols via a 1:1 relationship.
While in this example, it's clear which reference assemblies to pick (as there's a direct match), it's less clear in the following case:

Package	TFMs
Baseline	netstandard2.0
Current	net6.0

In that scenario, the customer decides to drop the netstandard2.0 asset but still expects APICompat to validate that the API between the netstandard2.0 and the net6.0 isn't changed in an incompatible way. It's not entirely clear to me if we want to pass the net6.0 reference assemblies from the current asset to the [Baseline] netstandard2.0 asset. My change doesn't do that as the net6.0 reference assembly group can't be referenced from the netstandard2.0 baseline TFM from a NuGet compatibility standpoint.

Nearest might be OK - so long as you're going forward and not back (I always get mixed up with NuGet's terms around this), since the nearest forward should be compatible and should be a parallel operation to the compatibility tuples we're creating between frameworks.

OK, I now understand what you mean by that. Yes, right now we are going backwards instead of forward. Will see if I can change that and avoid the additional compatibility check.

If assembly references aren't provided for the baseline package via the MSBuild task or the CLI frontend, use the current package's assembly references as an approximation. As current assembly references are grouped by TFMs, use the nearest compatible TFM for which assembly references are provided. This guarantees that assembly references from current are used even if TFMs don't match.

ViktorHofer · 2023-06-01T13:42:02Z

I found a place that enqueues package validation assemblies with an 1:1 mapping (TFM to TFM) which allows to re-use the assembly references from the right. Let's see what CI says. I re-ran the tests on top of the msbuild PR and the errors disappeared.

ericstj · 2023-06-01T15:29:11Z

It's not entirely clear to me if we want to pass the net6.0 reference assemblies from the current asset to the [Baseline] netstandard2.0 asset.

Think about API Compat baseline check as doing an experiment on framework supported by the latest package. It's doing a what if check between the previous version and the new version on a supported framework. Imagine all the references as the "control" in that experiment. So API Compat is answering the question of what will break if I updated the baseline package to the latest package in a project targeting a specific framework? The references in that case are not the variable under test, so treating them like a control is reasonable -- it does mean we might miss breaking changes in those references but we're already excluding them from the comparison rules so IMO it's an acceptable heuristic.

ViktorHofer · 2023-06-01T15:35:55Z

With these changes, I do see quite a lot of errors in a runtime allconfigurations build. I just verified that at least one of them is legit and due to dotnet/runtime#75235.

Looking at the other ones now.

ericstj

This looks better, just one small suggestion.

ericstj · 2023-06-01T15:34:47Z

src/ApiCompat/Microsoft.DotNet.PackageValidation/ApiCompatRunnerExtensions.cs

+                    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);


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

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.

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.

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.

ericstj

Change seems OK. My feedback is more around a future refactoring. Thank you for fixing this, by the way. I think this fix will help quite a bit.

ViktorHofer · 2023-06-01T20:34:45Z

All the errors that appeared in runtime with this change are correct. There is one odd case though:

APIs switching from the public ObsoleteAttribute to the internal polyfill one (for netstandard2.0). That is flagged as an attribute removal (as the public attribute is removed and APICompat filters out the internal one).

... and suppress the new errors that are flagged because of breaking changes introduced in .NET 8. See dotnet/sdk#32930 for more details on the added validation in APICompat.

* Update APICompat.Task to latest version ... and suppress the new errors that are flagged because of breaking changes introduced in .NET 8. See dotnet/sdk#32930 for more details on the added validation in APICompat. * Suppress new errors that were intentionally made * Update Version.Details.xml * Update CompatibilitySuppressions.xml * Update CompatibilitySuppressions.xml

ViktorHofer requested review from ericstj, a team and joperezr as code owners May 31, 2023 13:45

ViktorHofer self-assigned this May 31, 2023

ghost added the untriaged Request triage from a team member label May 31, 2023

ViktorHofer added the Area-ApiCompat label May 31, 2023

ViktorHofer mentioned this pull request May 31, 2023

Relay remoted exceptions dotnet/msbuild#8779

Merged

ericstj reviewed May 31, 2023

View reviewed changes

ViktorHofer added 3 commits June 1, 2023 15:34

Remove unused param

9a0ecc8

Use already calculated NuGet matching

c1172a2

ViktorHofer force-pushed the PackageValidationBaselineUseAssemblyReferences branch from a4eb4b8 to c1172a2 Compare June 1, 2023 13:34

ericstj reviewed Jun 1, 2023

View reviewed changes

ericstj approved these changes Jun 1, 2023

View reviewed changes

ViktorHofer merged commit c47af12 into main Jun 1, 2023

ViktorHofer deleted the PackageValidationBaselineUseAssemblyReferences branch June 1, 2023 20:30

This was referenced Jun 1, 2023

[Package Validation] Baseline package validation should run with references #18676

Closed

Unnecessary ApiCompat CP0001 errors when type forwards used #31271

Closed

ViktorHofer mentioned this pull request Jun 2, 2023

Update APICompat.Task to latest version dotnet/runtime#87046

Merged

ViktorHofer mentioned this pull request Jun 3, 2023

Validate existing suppressions and remove unnecessary suppressions when re-generating suppression files #32964

Merged

ericstj mentioned this pull request Sep 8, 2023

Investigate APICompat errors caused by assembly references re-used from the right-hand side during package validation #35214

Closed

[APICompat - PackageValidation] Enable baseline assembly references #32930

[APICompat - PackageValidation] Enable baseline assembly references #32930

Uh oh!

Conversation

ViktorHofer commented May 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented May 31, 2023

Uh oh!

ericstj commented May 31, 2023

Uh oh!

ericstj left a comment

Choose a reason for hiding this comment

Uh oh!

ViktorHofer commented May 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericstj commented May 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ViktorHofer commented Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ViktorHofer commented Jun 1, 2023

Uh oh!

ericstj commented Jun 1, 2023

Uh oh!

ViktorHofer commented Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericstj left a comment

Choose a reason for hiding this comment

Uh oh!

ericstj Jun 1, 2023

Choose a reason for hiding this comment

Uh oh!

ViktorHofer Jun 1, 2023

Choose a reason for hiding this comment

Uh oh!

ericstj Jun 1, 2023

Choose a reason for hiding this comment

Uh oh!

ViktorHofer Jun 1, 2023

Choose a reason for hiding this comment

Uh oh!

ericstj left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer commented Jun 1, 2023

Uh oh!

Uh oh!

ViktorHofer commented May 31, 2023 •

edited

Loading

ViktorHofer commented May 31, 2023 •

edited

Loading

ericstj commented May 31, 2023 •

edited

Loading

ViktorHofer commented Jun 1, 2023 •

edited

Loading

ViktorHofer commented Jun 1, 2023 •

edited

Loading

ericstj left a comment •

edited

Loading