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

Remove reference assemblies manipulation in Isolated SDK #33

Merged
merged 2 commits into from
Mar 12, 2024

Conversation

MattKotsenas
Copy link
Member

@MattKotsenas MattKotsenas commented Sep 5, 2023

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
Copy link

alexrp commented Jan 19, 2024

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
Copy link
Member Author

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
Copy link
Member Author

MattKotsenas commented Feb 5, 2024

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
Copy link
Member

baronfel commented Feb 5, 2024

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
Copy link
Member Author

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
Copy link
Member

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

@MattKotsenas
Copy link
Member Author

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 MattKotsenas changed the title Add PrivateAssets for reference assemblies Remove reference assemblies manipulation in Isolated SDK Mar 11, 2024
@MattKotsenas
Copy link
Member Author

@baronfel Updated!

@baronfel baronfel merged commit 0ed1278 into dotnet:main Mar 12, 2024
3 checks passed
@baronfel
Copy link
Member

Thanks for this @MattKotsenas

@MattKotsenas MattKotsenas deleted the patch-1 branch April 22, 2024 00:01
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