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

[ApiDiff] Implementation of the new ApiDiff that reuses GenAPI features #46425

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

carlossanlop
Copy link
Member

@carlossanlop carlossanlop commented Jan 30, 2025

Follow up of #46424

This PR is part of the work needed to create an ApiDiff tool that reuses some of the code from Microsoft.DotNet.GenAPI. The idea is to make the larger PR smaller and make it easier to review: #45389

The proposed changes in this PR include:

  • Full implementation of the tool that can generate an API diff from two assemblies.

CODEOWNERS Show resolved Hide resolved
@carlossanlop
Copy link
Member Author

I added a very simple test that verifies the contents of a basic DLL before and after adding one API.
Unfortunately the DLLs triggered the error below, so I might have to add the DLL as an actual pair of projects (before and after), compile them as part of the SDK repo build, and then I hope the two DLLs can be compared (the restriction is that they need to have the same assembly name).

expand to see the full error
repo-projects\Directory.Build.targets(504,5): error MSB3073: The command "D:\a\_work\1\vmr\src\sdk\eng\common\build.cmd -restore -build -pack -publish -sign -ci -configuration Release -bl /p:DotNetBuildRepo=true /p:DotNetBuildOrchestrator=true /p:TargetRid=win-x64 /p:RestoreConfigFile=D:\a\_work\1\vmr\artifacts\obj\sdk\NuGet.config /p:ForceDryRunSigning=true /p:SourceBuiltAssetsDir=D:\a\_work\1\vmr\artifacts\assets\Release\ /p:SourceBuiltAssetManifestsDir=D:\a\_work\1\vmr\artifacts\obj\manifests\Release\sdk\ -nativeToolsOnMachine /p:PackageProjectUrl=https://github.com/dotnet/sdk /p:PortableRid=win-x64 -v minimal /p:NETCoreAppMaximumVersion=99.9 /p:PortableOSName= /p:Rid=win-x64 /p:Architecture=x64 /p:DOTNET_INSTALL_DIR=D:\a\_work\1\vmr\.dotnet\ /p:SkipBuildingInstallers=true /p:SkipBuildingSdkBundle=true /p:PublicBaseURL=file://D:\a\_work\1\vmr\artifacts\assets\Release\ /p:FallbackPublicBaseURL=https://dotnetbuilds.blob.core.windows.net/public/ /p:UsePortableLinuxSharedFramework=false
❌artifacts\source-built-sdks\Microsoft.DotNet.Arcade.Sdk\tools\Sign.proj(75,5): error SIGN004: Signing 3rd party library 'D:\a\_work\1\vmr\src\sdk\artifacts\tmp\Release\ContainerSigning\339\tools/net8.0/any/Argon.dll' with Microsoft certificate 'MicrosoftDotNet500'. The library is considered 3rd party library due to its copyright: 'Copyright 2024. All rights reserved'.
❌artifacts\source-built-sdks\Microsoft.DotNet.Arcade.Sdk\tools\Sign.proj(75,5): error SIGN004: Signing 3rd party library 'D:\a\_work\1\vmr\src\sdk\artifacts\tmp\Release\ContainerSigning\340\tools/net8.0/any/DiffEngine.dll' with Microsoft certificate 'MicrosoftDotNet500'. The library is considered 3rd party library due to its copyright: 'Copyright 2024. All rights reserved'.

@carlossanlop
Copy link
Member Author

carlossanlop commented Feb 4, 2025

I just noticed I didn't implement the addition of the table of contents file. Working on that.

@carlossanlop carlossanlop removed the untriaged Request triage from a team member label Feb 4, 2025
@ViktorHofer
Copy link
Member

I think you just need to update Signing.props in this repo and tell the signing infrastructure how to sign the external assembly. This is by design afaik. @mmitche has more knowledge on this.

@carlossanlop
Copy link
Member Author

carlossanlop commented Feb 6, 2025

I think you just need to update Signing.props in this repo and tell the signing infrastructure how to sign the external assembly. This is by design afaik. @mmitche has more knowledge on this.

Here!

sdk/eng/Signing.props

Lines 28 to 29 in b513b97

<!-- Third party DLLs used by tests -->
<FileSignInfo Include="Castle.Core.dll" CertificateName="None" />

@carlossanlop
Copy link
Member Author

carlossanlop commented Feb 6, 2025

@mmitche do I sign the packages like I did in this commit? 8a94049

Or do I sign them using $(ExternalCertificateId) as indicated here?

sdk/eng/Signing.props

Lines 55 to 60 in b513b97

<!--
These are third party libraries that we use in Arcade. We need to sign them even if they
are already signed. However, they must be signed with a 3rd party certificate.
-->
<ItemGroup>
<FileSignInfo Include="MessagePack.Annotations.dll" CertificateName="$(ExternalCertificateId)" />

@carlossanlop
Copy link
Member Author

Do I just add <DotNetBuildSourceOnly>false</DotNetBuildSourceOnly> to the csprojs?

@ViktorHofer @MichaelSimons assuming we can take care of the signing problem of DiffPlex, is this the right way to exclude a src project from source-build?

image

@MichaelSimons
Copy link
Member

Do I just add <DotNetBuildSourceOnly>false</DotNetBuildSourceOnly> to the csprojs?

@ViktorHofer @MichaelSimons assuming we can take care of the signing problem of DiffPlex, is this the right way to exclude a src project from source-build?

image

Yes - This is documented here

@carlossanlop
Copy link
Member Author

Progress: setting DiffPlex resources to use no certificate got rid of the failure (pending confirming if it's the right choice, or if using $(ExternalCertificateId) is preferred, @mmitche).

The new failures are now related to the ApiDiff tool itself. Seems that the tests fail in Linux due to the wrong NewLine chars being used.

@carlossanlop carlossanlop force-pushed the ApiDiff branch 2 times, most recently from a1110f7 to ab99ff7 Compare February 7, 2025 01:37
// we try to process it but we end up throwing an exception as it is unrecognized.
// It's going to be fixed with https://github.com/dotnet/roslyn/pull/77102
[Fact(Skip = "https://github.com/dotnet/roslyn/issues/77101")]
public void TestExcludeUnmodifiedOperator()
Copy link
Member Author

Choose a reason for hiding this comment

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

@ericstj I implemented API exclusion in the tool. Unfortunately, it is still failing because the API needs to be processed/recognized before determining if it needs to be excluded. I added a skipped test anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that I added 3 other API exclusion tests above this one, but they are excluding non-checked operators, so those work fine.

Copy link
Member

Choose a reason for hiding this comment

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

processed/recognized before determining if it needs to be excluded

I would hope we could feed the exclusion down into the filter used for genapi. I think that should avoid trying to generate syntax for the API - since we can check if it's included before producing the syntax (just like we do for internal, for example).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, let me look at at that. Looks like I missed something.

/// <summary>
/// The default attributes to exclude from the diff.
/// </summary>
public static readonly string[] DefaultAttributesToExclude = [
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this can be improved by using the contents of this file by default: https://github.com/dotnet/runtime/blob/main/eng/DefaultGenApiDocIds.txt

Those are the attributes GenAPI excludes when generating the ref source files.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe for now, this string array is enough, and I can improve this in a follow-up PR.

@@ -8,8 +8,4 @@
<PackageVersion Update="Microsoft.CodeAnalysis.CSharp" Version="4.4.0" />
</ItemGroup>

<ItemGroup>
<PackageVersion Include="Microsoft.CodeAnalysis.Workspaces.Common" Version="$(MicrosoftCodeAnalysisWorkspacesCommonPackageVersion)" />
Copy link
Member Author

Choose a reason for hiding this comment

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

@ericstj this was not needed anymore. It's in the Versions.props. I forgot to remove it in this PR, it was a temporary workaround while I merged the PR that introduced the new Versions.props and Version.Details.xml entries, which have been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants