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.

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

.Net: Enable package compatibility validation #4438

Merged

Conversation

dmytrostruk
Copy link
Member

@dmytrostruk dmytrostruk commented Dec 28, 2023

Motivation and Context

Resolves: #4294

This PR contains changes to enable package compatibility validation for packages without alpha or preview prefix. This will allow to catch breaking changes in public API earlier and resolve them before the release.

Verification:
After following test commit with new optional parameter - dd8745c, PR build pipeline failed with following error:
https://github.com/microsoft/semantic-kernel/actions/runs/7350765480/job/20012998030?pr=4438
image

Contribution Checklist

Sorry, something went wrong.

Partially verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.
@dmytrostruk dmytrostruk self-assigned this Dec 28, 2023
@dmytrostruk dmytrostruk requested a review from a team as a code owner December 28, 2023 16:45
@shawncal shawncal added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel kernel.core labels Dec 28, 2023
@dmytrostruk
Copy link
Member Author

@stephentoub For compatibility validation I suppressed following errors, let me know if it's okay:

  • CP0003 - This error occurs due to difference in assemblies PublicKey. We set PublicKey only for Publish configuration in release pipeline, while we don't have PublicKey for Debug or Release configurations (and I'm not sure we need them when packing locally or in PR pipeline). So, I suppressed this rule for Debug and Release configurations, while it will be applied for Publish configuration in our release pipeline to check if assembly attributes are the same.
  • CP1002 - This error occurs only for .NET 8 SDK, when packing SemanticKernel.Core project it wants to find referenced project dll (Microsoft.SemanticKernel.Abstractions.dll) which doesn't exist. (Error can be found here: https://github.com/microsoft/semantic-kernel/actions/runs/7349585442/job/20009783098). Documentation says that it's possible to provide reference with <PackageValidationReferencePath Include="<path>" TargetFramework="<tfm>" />, but I'm not sure this reference exists, since we pack each project separately, and nupkg of each project doesn't contain dll for referenced project. Please let me know if I'm missing something.

Thanks!

@dmytrostruk dmytrostruk added the PR: ready for review All feedback addressed, ready for reviews label Dec 28, 2023
@stephentoub
Copy link
Member

@stephentoub For compatibility validation I suppressed following errors, let me know if it's okay:

  • CP0003 - This error occurs due to difference in assemblies PublicKey. We set PublicKey only for Publish configuration in release pipeline, while we don't have PublicKey for Debug or Release configurations (and I'm not sure we need them when packing locally or in PR pipeline). So, I suppressed this rule for Debug and Release configurations, while it will be applied for Publish configuration in our release pipeline to check if assembly attributes are the same.
  • CP1002 - This error occurs only for .NET 8 SDK, when packing SemanticKernel.Core project it wants to find referenced project dll (Microsoft.SemanticKernel.Abstractions.dll) which doesn't exist. (Error can be found here: https://github.com/microsoft/semantic-kernel/actions/runs/7349585442/job/20009783098). Documentation says that it's possible to provide reference with <PackageValidationReferencePath Include="<path>" TargetFramework="<tfm>" />, but I'm not sure this reference exists, since we pack each project separately, and nupkg of each project doesn't contain dll for referenced project. Please let me know if I'm missing something.

Thanks!

@ViktorHofer, can you assist @dmytrostruk here? We're trying to get package validation set up for Semantic Kernel.

@ViktorHofer
Copy link
Member

ViktorHofer commented Jan 4, 2024

Sure but I'm out till Monday. cc @ericstj

@ericstj
Copy link
Member

ericstj commented Jan 4, 2024

CP0003 ... we don't have PublicKey for Debug or Release configuration

Seems odd, if it were me I'd just have Debug and Release use public signing with the same public key. That minimizes the differences between configurations that run tests. I guess you might have another reason to not sign, but if not consider using public signing.

CP1002

Interesting - I wouldn't expect that to happen. It should be bringing the reference from ResolveReferences. Perhaps you're packing without building so ResolveReferences doesn't run? Let me pull this down and try. I might be able to provide a workaround.

@dmytrostruk
Copy link
Member Author

@ericstj

Perhaps you're packing without building so ResolveReferences doesn't run?

Actually, we pack after build by using <GeneratePackageOnBuild>true</GeneratePackageOnBuild>, so I'm not sure it's a root cause.

Let me pull this down and try. I might be able to provide a workaround.

Thank you!

@ericstj
Copy link
Member

ericstj commented Jan 4, 2024

Yep, I got the repro and I see the reference being passed in. Something fishy happening where the lookup isn't working. I'll debug a bit then report the findings.

Suppressing is OK as a workaround. Running API Compat with references can give you more coverage; for example, when a type is forwarded outside the assembly we can compare the target type in the new assembly with the previous type.

@dmytrostruk
Copy link
Member Author

Yep, I got the repro and I see the reference being passed in. Something fishy happening where the lookup isn't working. I'll debug a bit then report the findings.

Suppressing is OK as a workaround. Running API Compat with references can give you more coverage; for example, when a type is forwarded outside the assembly we can compare the target type in the new assembly with the previous type.

@ericstj Thanks for letting me know! For CP0003 I will check public signing. I will temporary suppress CP1002 and bring it back when the issue is resolved.

@ericstj
Copy link
Member

ericstj commented Jan 4, 2024

These are actually caused by the same problem. What's happening is when loading the baseline assembly from your shipping nuget package we load Microsoft.SemanticKernel.Core, Version=1.0.1.0, Culture=neutral, PublicKeyToken=f300afd708cefcd3 and try to load its reference Microsoft.SemanticKernel.Abstractions, Version=1.0.1.0, Culture=neutral, PublicKeyToken=f300afd708cefcd3 using the right side (latest built) references.

Since your latest build is omitting the public key token, it doesn't satisfy the reference, thus the load failure. The compiler is doing the loading for us, so we don't really have anything we can do to change this behavior in API compat.

The best way to solve both problems is using public signing. That way you don't fiddle with the identity of your assembly.

@ericstj
Copy link
Member

ericstj commented Jan 4, 2024

Here's a change that fixes both warnings by enabling public-signing. It builds for me but I haven't tested anything. Use if you like: ericstj@821066d

@dmytrostruk
Copy link
Member Author

Here's a change that fixes both warnings by enabling public-signing. It builds for me but I haven't tested anything. Use if you like: ericstj@821066d

@ericstj Thanks a lot!

@dmytrostruk dmytrostruk added this pull request to the merge queue Jan 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 9, 2024
@dmytrostruk dmytrostruk added this pull request to the merge queue Jan 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 9, 2024
@markwallace-microsoft markwallace-microsoft added this pull request to the merge queue Jan 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 9, 2024
@markwallace-microsoft markwallace-microsoft added this pull request to the merge queue Jan 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 9, 2024
@markwallace-microsoft markwallace-microsoft added this pull request to the merge queue Jan 9, 2024
Merged via the queue into microsoft:main with commit d3586dc Jan 9, 2024
Bryan-Roe pushed a commit to Bryan-Roe-ai/semantic-kernel that referenced this pull request Oct 6, 2024
### Motivation and Context

<!-- Thank you for your contribution to the semantic-kernel repo!
Please help reviewers and future users, providing the following
information:
  1. Why is this change required?
  2. What problem does it solve?
  3. What scenario does it contribute to?
  4. If it fixes an open issue, please link to the issue here.
-->

Resolves: microsoft#4294

This PR contains changes to enable package compatibility validation for
packages without `alpha` or `preview` prefix. This will allow to catch
breaking changes in public API earlier and resolve them before the
release.

Verification:
After following test commit with new optional parameter -
microsoft@dd8745c,
PR build pipeline failed with following error:

https://github.com/microsoft/semantic-kernel/actions/runs/7350765480/job/20012998030?pr=4438
<img width="787" alt="image"
src="https://github.com/microsoft/semantic-kernel/assets/13853051/a0292351-4811-48d5-b64f-24401a4c5e57">


### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [x] The code builds clean without any errors or warnings
- [x] The PR follows the [SK Contribution
Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [x] All unit tests pass, and I have added new tests where possible
- [x] I didn't break anyone 😄

---------

Co-authored-by: Mark Wallace <127216156+markwallace-microsoft@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kernel.core kernel Issues or pull requests impacting the core kernel .NET Issue or Pull requests regarding .NET code PR: ready for review All feedback addressed, ready for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.Net: Enable package compat validation after 1.0 is released
8 participants