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

Add SQL code analysis from PackageReference or ProjectReference #479

Merged
merged 8 commits into from
Sep 17, 2024

Conversation

zijchen
Copy link
Member

@zijchen zijchen commented Aug 5, 2024

Corresponding SDK changes for analyzer package support, fixes #332

TODO: Need new DacFx version

Add PackageReference to analyzer Nuget via

<PackageReference Include="CodeAnalyzerSample" Version="1.0.0" />
  • Analyzer DLL must be in analyzers/dotnet/cs
  • Analyzer package can target any .NET version DacFx supports

You can also add reference via ProjectReference

<ProjectReference Include="C:\test\test_analyzer\CodeAnalyzerSample.csproj"
                      PrivateAssets="all"
                      ReferenceOutputAssembly="false"
                      OutputItemType="Analyzer"
                      SetTargetFramework="TargetFramework=netstandard2.1" />
  • You must override target GetTargetPath in your csproj

See example analyzer in CodeAnalyzerSample.csproj

@@ -0,0 +1,16 @@
<Project Sdk="Microsoft.NET.Sdk">
Copy link
Contributor

Choose a reason for hiding this comment

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

As a rules author, do I really need to add all this cruft to my project to build an analyzer package? Seems complex and error prone..

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't find a straightforward sample either, seems like .NET doesn't have an easy path to creating an analyzer package. The sample I used is from https://github.com/dotnet/samples/blob/main/csharp/roslyn-sdk/Tutorials/MakeConst/MakeConst.Package/MakeConst.Package.csproj

Copy link
Contributor

Choose a reason for hiding this comment

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

Again, this is not a roslyn analyzer and DacFx has its own analysis result mechanism.

But OK - there will be few publishers of rules and hopefully many consumers

Copy link
Member Author

Choose a reason for hiding this comment

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

We can add this in a future dotnet new template @dzsquared

<Target Name="GetTargetPath" />
<Target Name="_AddAnalyzersToOutput">
<ItemGroup>
<TfmSpecificPackageFile Include="$(OutputPath)\CodeAnalyzerSample.dll" PackagePath="analyzers/dotnet/cs" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this uncommon location? How about "tools" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, but this is not a roslyn analyzer and the target language is not "cs" ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I can understand that, my motivation was to keep it similar to C# and use Nuget's package resolution logic. We did build in the extensibility via SqlCodeAnalysisAssemblyPaths so if there is a growing demand for another location, we can easily hook that up.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nuget's package resolution logic

Fair, if that adds value for you.

But maybe just use analyzers\dotnet\ ??

@ErikEJ
Copy link
Contributor

ErikEJ commented Aug 6, 2024

This is very cool, looking forward.

I already publish a couple of NuGet packages with analyzer rules, targeting both .net standard 2.1 and netfx 4.6.2: https://www.nuget.org/packages?q=erikej.dacfx

So very interested in this topic.

The packages that I publish are currently used for consumption by a project, that use DacFX, not for consumption in a .sqlproj project. But it would be very nice to be able to support both scenarios in the same NuGet package, IMHO.

@ErikEJ
Copy link
Contributor

ErikEJ commented Aug 6, 2024

What does "You must override target GetTargetPath in your csproj" mean??

@ErikEJ
Copy link
Contributor

ErikEJ commented Aug 6, 2024

So, this is the result of Nuget pack with the sample here:

image

And this is my package:

image

Questions:

What is the value of having the .dll in two places?

How do I target multiple frameworks? Or is this a .NET only proposal? (No NetFX) ?

@zijchen
Copy link
Member Author

zijchen commented Aug 6, 2024

What does "You must override target GetTargetPath in your csproj" mean??

This is only necessary if you want to add the analyzer via ProjectReference. It's to workaround a bug of a csproj referenced from sqlproj throwing that the target doesn't exist

@ErikEJ
Copy link
Contributor

ErikEJ commented Aug 6, 2024

Is it really necessary ti support that?

@zijchen
Copy link
Member Author

zijchen commented Aug 6, 2024

This is very cool, looking forward.

I already publish a couple of NuGet packages with analyzer rules, targeting both .net standard 2.1 and netfx 4.6.2: https://www.nuget.org/packages?q=erikej.dacfx

So very interested in this topic.

The packages that I publish are currently used for consumption by a project, that use DacFX, not for consumption in a .sqlproj project. But it would be very nice to be able to support both scenarios in the same NuGet package, IMHO.

We are introducing a new property SqlCodeAnalysisAssemblyPaths in the next version of DacFx that you can use to hook up additional analyzer paths. I imagine you can use this for MSBuild.Sdk.Sqlproj too

@ErikEJ
Copy link
Contributor

ErikEJ commented Aug 6, 2024

Amazing!

@jmezach
Copy link

jmezach commented Aug 7, 2024

@ErikEJ That would make it a lot easier to implement rr-wfm/MSBuild.Sdk.SqlProj#569.

@zijchen zijchen marked this pull request as ready for review September 11, 2024 17:31
@ErikEJ
Copy link
Contributor

ErikEJ commented Sep 13, 2024

@zijchen I can see there is a preview release now with SqlCodeAnalysisAssemblyPaths I assume

In which namespace do I find SqlCodeAnalysisAssemblyPaths ?

@zijchen
Copy link
Member Author

zijchen commented Sep 13, 2024

@zijchen I can see there is a preview release now with SqlCodeAnalysisAssemblyPaths I assume

In which namespace do I find SqlCodeAnalysisAssemblyPaths ?

SqlCodeAnalysisAssemblyPaths is a property of Microsoft.Data.Tools.Schema.Tasks.Sql.SqlStaticCodeAnalysisTask. This hooks into a new AssemblyLookupPath property in Microsoft.SqlServer.Dac.CodeAnalysis.CodeAnalysisServiceSettings if you're using the library directly.

@zijchen zijchen merged commit ac9e6e2 into main Sep 17, 2024
13 checks passed
@zijchen zijchen deleted the zijchen/code-analyzer branch September 17, 2024 17:07
@ErikEJ
Copy link
Contributor

ErikEJ commented Sep 21, 2024

@zijchen I actually just did this to pack my rules:

    <None Include="..\SqlServer.Dac\bin\$(Configuration)\netstandard2.1\SqlServer.Dac.dll" 
             Pack="true"
             PackagePath="analyzers\dotnet\cs"
             Visible="false" />
    <None Include="bin\$(Configuration)\netstandard2.1\SqlServer.Rules.dll" 
        Pack="true"
        PackagePath="analyzers\dotnet\cs"
        Visible="false" />

@ErikEJ
Copy link
Contributor

ErikEJ commented Sep 25, 2024

Sadly, the property in DacFX ended up being called: SqlCodeAnalysisAssemblyPath 😢 - s is missing

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.

Add code analyzers support as nuget-packages to new SDK
5 participants