NuGet package should not treat contentfiles as private - Githubissues

microsoft / ApplicationInsights-Profiler-AspNetCore

Application Insights Profiler sample and documentation

MIT License

66 stars 22 forks source link

NuGet package should not treat contentfiles as private #173

Closed taspeotis closed 2 years ago

taspeotis commented 2 years ago

I have ten .NET 6 web applications, they have a NuGet package shared between them that sets up a lot of common stuff. One of the things it sets up is Application Insights.

If I add Microsoft.ApplicationInsights.Profiler.AspNetCore to my common NuGet package and enable profiling in one of the projects that references the common NuGet package (but not Microsoft...Profiler.AspNetCore directly), it will fail with:

Uploader can't be located.

The problem seems to be that NuGet treats contentfiles (where TraceUpload.zip comes from) as private.

You can see some documentation here and a GitHub issue about it here.

Instead of...

<PackageReference Include="Microsoft.ApplicationInsights.Profiler.AspNetCore" Version="2.3.1" />

...I replaced it with...

<PackageReference Include="Microsoft.ApplicationInsights.Profiler.AspNetCore" Version="2.3.1">
  <!-- Default is contentfiles;analyzers;build as per: -->
  <!-- https://docs.microsoft.com/en-us/nuget/consume-packages/package-references-in-project-files#controlling-dependency-assets -->
  <PrivateAssets>analyzers;build</PrivateAssets>
</PackageReference>

...and it worked.

I believe the NuGet package you produce can override PrivateAssets to be analyzers;build so to make it automatically copy the TraceUpload.zip file in projects that don't depend on Microsoft...Profiler.AspNetCore directly.

xiaomi7732 commented 2 years ago

@taspeotis Thanks for the report. That is an interesting scenario. We will look into that.

xiaomi7732 commented 2 years ago

Hey @taspeotis, I looked into the documentation and learned from it. Here's my current understanding:

You are doing it correct by setting <PrivateAssets>analyzers;build</PrivateAssets> for package reference of Profiler.ASPNETCore in your shared project so that the contentFiles will flow into the header projects, which reference the shared.
As package author, I don't think we can overwrite the behavior of contentFiles:
- Otherwise, imaging various package owners setting various values, it beat up the purpose of the default behavior

Or I could totally be wrong. And if there's anyone find out a solution and I will be more than glad to take it.

That said, I think this is very good scenario for real world projects. I think we should come up with a walk-though / guidance on how to do it.

What do you think?

xiaomi7732 commented 2 years ago

Tag one more related issue: https://github.com/NuGet/Home/issues/6091

taspeotis commented 2 years ago

Thanks for looking into it. I don't have a good solution. Some thoughts:

I didn't realise the .nuspec didn't give you the flexibility to edit this. I had assumed there was a way to do it, because when you add a package like EF Core Tools or the ASP.NET Integration Testing their entries in the .csproj set PrivateAssets and ExcludeAssets. This may be a behaviour of NuGet though and not their packages.
Solving it with documentation might be okay? At least making it clear to users what they should expect to do if they do not install the package directly.
I recall that NuGet supports (or used to support) post install scripts that might be useful to edit the .csproj it is installed into. Install scripts seem to be outdated though.
Have you considered shipping TraceUpload.zip as an embedded resource rather than a contentFile? The .zip contents would have to be extracted out of the assembly as a stream, and decompressed into the user's temporary folder. But I don't see this as substantially different from shipping the .zip side by side the assembly; it has to get unzipped anyway.

Again, thanks for your work on this feature.

xiaomi7732 commented 2 years ago

@taspeotis Thank you so much for the suggestions. We probably going to update the document as a short term mitigation for this issue and we will need to look into various ways for shipping the uploader, and having it as embedded resource will be considered :-). Thanks again for the input!

On the record, this could be combined with a related issue to making uploader workable with in self-contained app #170 .

xiaomi7732 commented 2 years ago

Hi @taspeotis What do you think about a document like this: Reuse Profiler NuGet Package.

taspeotis commented 2 years ago

Thank you for writing that page, it describes how to solve the problem for anyone who uses the NuGet package "indirectly" like I do.

You're welcome to close this issue since the behaviour is documented, and my original request ("package should not treat contentfiles as private") I understand is actually to impossible to solve as requested, since the .nuspec doesn't have any provision for it.

xiaomi7732 commented 2 years ago

Thanks for the report @taspeotis. It really helps clear up the use case for short term. In the long run, I think we are opening for adopting ways to make it easier for the users. Constantly seeking for opportunities.

I'll close this issue once the change is merged.

xiaomi7732 commented 2 years ago

Documented.