NuGet / Home

Repo for NuGet Client issues
Other
1.5k stars 252 forks source link

project.assets.json should indicate "analyzer" assets like "compile" and "runtime" #6279

Open nguerrera opened 6 years ago

nguerrera commented 6 years ago

project.assets.json doesn't actually indicate which analyzer references a project should use.

Unlike "compile" and "runtime", the SDK is just pattern matching on all files in the package to find analyzers: https://github.com/dotnet/sdk/blob/634680ab0ae045ba45977b6293b46f95a0385aac/src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageDependencies.cs#L251

"analyzer" assets should work like "compile" and "runtime" and the SDK should resolve them the same way. The current mechanism blocks respecting PrivateAssets and ExcludeAssets=analyzers

cc @emgarten

emgarten commented 6 years ago

Analyzers were added without work being done on the NuGet side originally. We should add it to project.assets.json so that the SDK can switch over to using that instead of reading packages directly to find the analyzers folder.

nguerrera commented 6 years ago

Related to this, is there a spec for how analyzer assets should be selected based on project language?

Current implementations are here: https://github.com/NuGet/NuGet.BuildTasks/blob/1d8af3499f94a32c1d128a42faceade39c1f4337/src/Microsoft.NuGet.Build.Tasks/ResolveNuGetPackageAssets.cs#L444-L470 https://github.com/dotnet/sdk/blob/634680ab0ae045ba45977b6293b46f95a0385aac/src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageDependencies.cs#L251-L272

I believe they are semantically equivalent, but both have some strange behavior if I'm reading correctly.

cc @jinujoseph @natidea @jasonmalinowski

jasonmalinowski commented 6 years ago

Adding @jmarolf too.

This seems to ignores the possibility of other languages gaining analyzers. e.g. If there were F# analyzers, they would match VB and C# projects.

I think that was just the code trying to be tricky and deal with the "no language either" thing at the same time.

nguerrera commented 6 years ago

cc @davidfowl

davidfowl commented 6 years ago

This seems pretty broken. It means that analyzers don't respect the PrivateAssets or flags specified on the PacakgeReference (I see a few people suffering from this already today).

nguerrera commented 6 years ago

Yes, it is pretty broken. See two linked sdk bugs that are blocked on this.

roryprimrose commented 6 years ago

I have to run this powershell on my builds as a workaround using the parameters -path $(Build.SourcesDirectory) -packageName $(Build.DefinitionName)

Param(
   [string]$path,
   [string]$packageName
)

Add-Type -Assembly System.IO.Compression.FileSystem

Function Remove-RoslynAnalyzers([string] $filePath, [string] $projectName)
{
    if ((Test-Path -Path $filePath) -eq $false)
    {
        Write-Host "$filePath was not found"

        return
    }

    Write-Host "Scanning $filePath"

    $ZipStream = [System.IO.Compression.ZipFile]::Open("$filePath", 'Update')
    $ZipItem = $ZipStream.GetEntry("$projectName.nuspec")

    if ($ZipItem -eq $null)
    {
        Write-Host "$projectName.nuspec was not found in the zip"

        return
    }

    [xml]$nuspec = $null
    $reader = $null

    $reader = New-Object System.IO.StreamReader($ZipItem.Open())

    try
    {
        [xml]$nuspec = $reader.ReadToEnd()
    }
    finally
    {
        if ($null -ne $reader) 
        { 
            $reader.Close() 
            $reader.Dispose() 
        }
    }

    Clean-NuSpec $nuspec

    # Re-open the file this time with streamwriter
    $writer = [System.IO.StreamWriter]($ZipItem).Open()

    try
    {
        # If needed, zero out the file -- in case the new file is shorter than the old one
        $writer.BaseStream.SetLength(0)

        # Insert the $text to the file and close
        $writer.Write($nuspec.OuterXml)
    }
    finally
    {
        $writer.Flush()
        $writer.Close()
    }

    # Write the changes and close the zip file
    $ZipStream.Dispose()
}

Function Clean-NuSpec([xml]$xml)
{
    $namespace = "http://schemas.microsoft.com/packaging/2013/05/nuspec.xsd"

    [System.Xml.XmlNamespaceManager] $nsMgr = New-Object System.Xml.XmlNamespaceManager($nuspec.NameTable)
    $nsMgr.AddNamespace("ns", "http://schemas.microsoft.com/packaging/2013/05/nuspec.xsd");

    Remove-Package $nuspec "CodeCracker.CSharp" $nsMgr

    [string]$analyzersPath = "//ns:package/ns:metadata/ns:dependencies/ns:group/ns:dependency[contains(@id,'.Analyzers')]"

    $nodes = $nuspec.SelectNodes($analyzersPath, $nsMgr)

    foreach ($node in $nodes)
    {
        [string]$id = $node.Attributes["id"].Value

        Write-Host "Removing $id"

        $node.ParentNode.RemoveChild($node)
    }
}

Function Remove-Package([xml]$xml, [string] $name, [System.Xml.XmlNamespaceManager] $nsMgr)
{
    [string]$path = "//ns:package/ns:metadata/ns:dependencies/ns:group/ns:dependency[@id='$name']"

    $nodes = $xml.SelectNodes($path, $nsMgr)

    foreach ($node in $nodes)
    {
        [string]$id = $node.Attributes["id"].Value

        Write-Host "Removing $id"

        $node.ParentNode.RemoveChild($node)
    }
}

$items = Get-ChildItem $path -Recurse -Filter "$packageName.*.nupkg"

foreach ($item in $items)
{
     Remove-RoslynAnalyzers $item.FullName $packageName | out-null
}
davidfowl commented 6 years ago

@rrelyea I see this is both a pri 0 and on the backlog. Any chance we can re-triage this?

nguerrera commented 6 years ago

See also https://github.com/dotnet/sdk/issues/968#issuecomment-410319090 that notes that PrivateAssets="all" can prevent analyzers from flowing, if you can afford to prevent all other package assets from flowing.

AArnott commented 6 years ago

Is there a workaround for the problem we're seeing in #7426? Specifically, we want to suppress chaining in analyzers as a transitive dependency through our package.

AArnott commented 6 years ago

I just tried adding ExcludeAssets="analyzers" to my PackageReference and it still didn't exclude the analyzers. Is there no way to turn off analyzers brought in by a transitive dependency?

levhaikin commented 5 years ago

have you tried <PrivateAssets>all</PrivateAssets>?

e.g.: `

all
<PackageReference Include="StyleCop.Analyzers" Version="1.1.1-beta.61" >
  <PrivateAssets>all</PrivateAssets>
</PackageReference>

`

AArnott commented 5 years ago

@levhaikin PrivateAssets=all only applies if I am the one with a direct dependency on an analyzer package. And yes, that works.

But if I'm referencing a package that should propagate to my consumers, but that package itself has an analyzer dependency, I should be able to suppress that using ExcludeAssets="analyzers", but it doesn't work.

levhaikin commented 5 years ago

I see.

sakopov commented 5 years ago

Any news on this?

joshlang commented 5 years ago

I'm just here posting my interest in seeing this bug resolved. It's BONKERS. (finally, a proper scenario in which to deploy that word).

The analyzers are like a virus.

Everything we do is Async. We shouldn't be forced into adding "Async" onto every method name on 100+ projects in a solution just because we use a nuget package with threading analyzers (StreamJsonRpc in this case).

ViktorHofer commented 5 years ago

We are also hitting this in corefx now. Seems like this issue is stale?

danielmeza commented 4 years ago

Here I get the same issue, any update on this?

aidapsibr commented 4 years ago

This is a confounding issue to encounter where everything the docs say to do has no effect and eventually after asking experts on twitter and nothing working either, you end up finding this bug. Now I'm considering republishing a package that I don't own so that I can get rid of the transitive analyzer OR creating custom MSBuild tasks to forcibly rip analyzers out prior to compiling. Send help.

nguerrera commented 4 years ago

Seeing a spike on reports on this. Can we add it to an agenda in a weekly sync in the new year and see if we can get this on the roadmap?

ericstj commented 3 years ago

What's the state of this issue? It seems more important now that we're adding source-generators to packages which will be treated the same way.

tristanlabelle commented 3 years ago

+1, also hitting this now that I'm porting projects to SDK-style and using <PackageReferences>

matkoch commented 3 years ago

@JonDouglas just tagging to make sure someone notices this and we get an assignee.

This issue in fact became more relevant with source generators.

JonDouglas commented 3 years ago

Hi all,

Please continue to use the 👍 reaction on the first comment so we can get an accurate understanding of the needs for this issue. In addition, if anyone has major thoughts as to a reproduction or design for this issue, please feel free to comment here or consider writing up a proposal for our team to review.

I'll take a read through this issue just to understand what's being asked here. We'll likely need to understand the nature of this work for .NET 7 and other upcoming releases.

kevinchalet commented 3 years ago

We'll likely need to understand the nature of this work for .NET 7 and other upcoming releases.

Waiting for .NET 7 would be terrible, IMO, given how many projects are going to be impacted (multi-targeting is a frequent scenario and analyzers are now included in many popular packages, including the .NET Platform logging extensions).

bruno-garcia commented 3 years ago

If this is a blocker for: https://github.com/dotnet/sdk/issues/18148 I'm really afraid of this not landing on .NET 6. Particularly when on .NET 6 some packages are dropping older targets like netstandard2.0 and net461. this impacts library authors like me in this situation: https://github.com/dotnet/runtime/issues/53454

zkat commented 3 years ago

Our understanding is that https://github.com/dotnet/sdk/issues/18148 is not strictly blocked by this issue, as it can be implemented without NuGet's help. If this is not the case, please update us (and @ us). /cc @bruno-garcia

ghost commented 3 years ago

This issue has been automatically marked as stale because it has been marked as requiring customer feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within another 14 days of this comment.

danielmeza commented 3 years ago

Good try @msfbot 😄

zubivan commented 2 years ago

Hello all, any chance a fix for this issue lands for .net 6? Migrating a big project now and just ran into it.

ViktorHofer commented 2 years ago

@nkolev92 what does Category:SeasonOfGiving mean? Is that some sort of backlog? Note that this became much more important with the addition of source generators that often ship via nuget packages.

nkolev92 commented 2 years ago

Hey all, if you are curious about the Category:SeasonOfGiving effort, refer to our blog, https://devblogs.microsoft.com/nuget/nuget-season-of-giving/#season-of-giving.

erdembayar commented 2 years ago

Unassigned myself for now. If no one pickup for Nov then I'll pick up again in December.

MeikelLP commented 3 months ago

Still unable to exclude analyzers after 7 years...

Drmorph commented 3 months ago

if someone search a hack to be able to do something like ExcludeAssets=analyzers (in my case StyleCop.Analyzers) , i found an old tweet https://twitter.com/Nick_Craver/status/1173996405276467202?s=09 who help me a lot to do it , added this on my .csproj

  <Target Name="DisableStaticAnalysis" BeforeTargets="CoreCompile" Condition="$(RunStaticAnalysis) == '' OR $(RunStaticAnalysis) == 'false'">
    <ItemGroup>
      <Analyzer Remove="@(Analyzer)" Condition="%(Filename) == 'StyleCop.Analyzers'" />
    </ItemGroup>
  </Target>
dominikjeske commented 1 month ago

Any updates on this?

davidebbo commented 1 week ago

That seems like a really bad issue, because those analyzers end up being viral. In my scenario:

And the end result is that every single project in my solution gets those unwanted analyzers.

Is there a way to disable them globally in the solution? Doing it at project level is quite painful...

AArnott commented 1 week ago

@davidebbo Yes, just add an .editorconfig file in the root of your solution and set the severity for each analyzer to None that you don't want to see.

davidebbo commented 6 days ago

@AArnott ah yes, that's a much better solution, thanks!

e.g. (but you don't need to manually edit it if you do it through the designer):

dotnet_diagnostic.VSTHRD003.severity = none
dotnet_diagnostic.VSTHRD002.severity = none
dotnet_diagnostic.VSTHRD103.severity = none
dotnet_diagnostic.VSTHRD110.severity = none
dotnet_diagnostic.VSTHRD011.severity = none
dominikjeske commented 5 days ago

yes but this is not fixing the problem but rather covering up the problem - when someone is developing nuget package there should be readme with info - just add .editorconfig to hide warnings from analyzers you are not expecting :)

davidebbo commented 5 days ago

yes but this is not fixing the problem but rather covering up the problem - when someone is developing nuget package there should be readme with info - just add .editorconfig to hide warnings from analyzers you are not expecting :)

Of course, it's just a workaround. Fixing the underlying issue is still very much needed. This is just to unblock...