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

Add PackageDiff tool and task #18580

Closed
wants to merge 6 commits into from
Closed

Conversation

jtschuster
Copy link
Member

Package Diff tool for dotnet/source-build#4051

Creates a command line tool and an msbuild tooltask to find differences in nuget packages. It does a git-style diffing to show line by line differences in the list of files in the package and in the nuspec file. There isn't yet a way to handle expected differences, so the msbuild task always succeeds, but will log any differences it finds.

I'm planning to do more in depth checks on each assembly in the packages and check for each member on each type and take a list of assemblies with expected differences to skip these checks.

@jtschuster jtschuster requested a review from a team as a code owner February 8, 2024 20:46
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<OutputType>Library</OutputType>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<OutputType>Library</OutputType>


<PropertyGroup>
<OutputType>Library</OutputType>
<TargetFrameworks>netstandard2.0</TargetFrameworks>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<TargetFrameworks>netstandard2.0</TargetFrameworks>
<TargetFramework>netstandard2.0</TargetFramework>

…f/PackageDiff.csproj

Co-authored-by: Michael Simons <msimons@microsoft.com>
<TargetFrameworks>netstandard2.0</TargetFrameworks>
<ImplicitUsings>enable</ImplicitUsings>
<Nullable>enable</Nullable>
<IsPackable>true</IsPackable>
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to create a package for the build task? Is the plan to use it directly here as part of the validation process? If so then we wouldn't need a package.

<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<OutputType>Exe</OutputType>
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 that we need a CLI frontend. The MSBuild task should be enough. We don't have that for the other analysis types either (poison check, prebuilt check, etc.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I had thought we needed the task to be netstandard2.0, which didn't have the reflection API's to get the assembly info. It looks like this task can be just netcurrent, so I can put everything in the task assembly and simplify this a bit.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this doesn't need to support desktop msbuild.

@jtschuster jtschuster marked this pull request as draft February 13, 2024 20:04
</PropertyGroup>

<ItemGroup>
<PackageReference Include="Microsoft.Build.Utilities.Core" PrivateAssets="all" ExcludeAssets="Runtime" Version="$(MicrosoftBuildVersion)" />
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<PackageReference Include="Microsoft.Build.Utilities.Core" PrivateAssets="all" ExcludeAssets="Runtime" Version="$(MicrosoftBuildVersion)" />
<PackageReference Include="Microsoft.Build.Utilities.Core" Version="$(MicrosoftBuildVersion)" />

<ItemGroup>
<PackageReference Include="Microsoft.Build.Utilities.Core" PrivateAssets="all" ExcludeAssets="Runtime" Version="$(MicrosoftBuildVersion)" />
</ItemGroup>
</Project>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
</Project>
</Project>

}
}

public class GetClosestPackages: GetClosestPackageBase
Copy link
Member

Choose a reason for hiding this comment

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

Why not just have the task that accepts multiple packages and remove the singular one?

@ViktorHofer
Copy link
Member

@jtschuster should this PR be closed?

@jtschuster
Copy link
Member Author

Yes, this should be closed, thanks for reminding me.

@jtschuster jtschuster closed this May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants