dotnet / reproducible-builds

Contains the DotNet.ReproducibleBuilds package
MIT License
168 stars 18 forks source link

Remove reference assemblies manipulation in Isolated SDK #33

Closed MattKotsenas closed 3 months ago

MattKotsenas commented 10 months ago

The SDK versions 5+ automatically include reference assemblies if needed (see https://github.com/dotnet/sdk/blob/5d0c38bf4aeebd7e28ae178c6e97dda8878018b9/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets#L476-L481). As a result, we no longer need to modify package references ourselves.

alexrp commented 5 months ago

This package declares itself as a development-only dependency in its nuspec: <developmentDependency>true</developmentDependency>

AFAIK PrivateAssets=all is not necessary for such packages. Are you seeing some kind of behavior that suggests otherwise?

MattKotsenas commented 5 months ago

I believe you're correct; I think my use case was messed up because the fix for #20 hasn't been released yet, so I was manually adding the package to update the version and thus needed to set PrivateAssets=all myself.

However, according to @baronfel, I think we can remove the reference assembly references now, as the SDK does it itself: https://github.com/dotnet/sdk/blob/f051b536cc12190488231f3a889df44214c1bc2e/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets#L456-L473

@baronfel, if you agree here, I can remove this, which closes this PR and #26 , and then we can publish a new version?

MattKotsenas commented 4 months ago

OK, went back and verified that adding the .Isolated package to a project incorrectly (I believe) adds the reference assemblies as a NuGet dependency of the project for .NETFramework 4 packages. This makes sense despite the package setting <developmentDependency>true</developmentDependency>, as the Sdk.targets file is adding a <PackageReference> the parent project, so the PrivateAssets change should still be made.

However, the point about the SDK doing this itself I think still stands. @baronfel, would you be willing to take a look and help close out these few reference assembly issues and release a new version? Thanks!

baronfel commented 4 months ago

I need to find some time to get back to this, as I've never really understood how to release a new version of this package. That won't happen for at least a week, though, as I have some work deadlines for other projects early this week, then am leaving for vacation the rest of the week.

MattKotsenas commented 3 months ago

Hey there! Friendly ping on this. I know it's annoying to address this, but I'm in the process of cleaning up a ton of team projects / repos, and being able to centralize on the definitive package for reproducible builds and remove custom setup from each project is a big win for us.

Thanks again!

baronfel commented 3 months ago

What do you think about removing this reference handling entirely since the SDK does it?

MattKotsenas commented 3 months ago

I'm cool with that! 😎

I'll update this PR with the removal and update the description to say the same. Thanks for your help!

MattKotsenas commented 3 months ago

@baronfel Updated!

baronfel commented 3 months ago

Thanks for this @MattKotsenas