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

Don't publish PrivateAssets Fixes #39400 #45259

Merged
merged 5 commits into from
Dec 10, 2024

Conversation

Forgind
Copy link
Member

@Forgind Forgind commented Dec 3, 2024

Fixes #39400

PrivateAssets="all" still permitted things to appear in the deps.json file. That probably shouldn't be true, especially since they aren't actually in the assets. This makes it so things that shouldn't be published are excluded from that file. the screenshot below is from the deps.json file created when using the provided repro in #39400 but with these changes.

image

As you can see, Nerdbank.GitVersioning is absent.

See #3348 as it deleted some of the code added in this PR.

@Forgind Forgind requested review from AntonLapounov and a team as code owners December 3, 2024 00:43
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-NetSDK untriaged Request triage from a team member labels Dec 3, 2024
@ViktorHofer
Copy link
Member

What about ProjectReferences that are marked as PrivateAssets. Do they appear as well?

@Forgind
Copy link
Member Author

Forgind commented Dec 3, 2024

What about ProjectReferences that are marked as PrivateAssets. Do they appear as well?

ProjectReferences marked as PrivateAssets do show up:
image

ProjectReferences are a bit tricky. On the one hand, it seems logical that they should be treated in the same way as PackageReferences, and NuGet even documents that PrivateAssets should work on ProjectReferences. However, #952 tracks that we don't properly respect PrivateAssets on ProjectReferences in other cases. In my case, I can see that my ProjectReference'd project's binaries do appear in my output folder:
image

With that in mind, I think it's better to fix this here then attach a note to #952 to fix the deps.json as part of it rather than making this (counterintuitively) fix the deps.json for ProjectReferences without fixing anything else.

Also, #3348 didn't seem to touch ProjectReferences, so it at least wasn't a regression from that.

@Forgind Forgind requested a review from a team as a code owner December 3, 2024 17:36
var packageReference = dependencyContext.CompileLibraries.First(l => l.Name == "System.Runtime.CompilerServices.Unsafe");
packageReference.Assemblies.Should().NotBeEmpty();
// Ensure compile references from a PrivateAssets="all" PackageReference don't exist
var packageReference = dependencyContext.CompileLibraries.FirstOrDefault(l => l.Name == "System.Runtime.CompilerServices.Unsafe", defaultValue: null);
Copy link
Member Author

Choose a reason for hiding this comment

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

This test's project file has these items:

<ItemGroup>
    <PackageReference Include="Microsoft.AspNetCore.App" Version="2.2.6" PrivateAssets="All" />
    <PackageReference Include="Microsoft.AspNetCore.Razor.Design" Version="2.2.0" PrivateAssets="All" />
  </ItemGroup>

and expects their (indirect) references to appear in the deps.json file. They don't appear in the output, so there's no reason they should be in the deps.json.

There are similar tests that don't include the PrivateAssets. Their (indirect) references appear in the deps.json and the output directory as expected, so they don't need to be changed.

Co-authored-by: Michael Yanni <MiYanni@microsoft.com>
Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

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

I've been confused about this before, but I think this is the right change.

Could you add an explicit test case for this? The original bug report in #39400 has a good example.

I'm not sure this is as risky as I was originally thinking it could be, but it still might be good to put this in 10.0 instead of 9.0.200.

@marcpopMSFT marcpopMSFT merged commit 25ad186 into dotnet:release/9.0.2xx Dec 10, 2024
28 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-NetSDK untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants