Add assemblies and packages for new code style analyzer distribution #18285

sharwell · 2017-03-29T16:55:00Z

Fixes #18278.

New analyzer packages:

Microsoft.CodeAnalysis.CSharp.CodeStyle
Microsoft.CodeAnalysis.VisualBasic.CodeStyle

New projects containing code distributed with the above packages:

Microsoft.CodeAnalysis.CodeStyle (shared code for analyzers, no workspace reference)
Microsoft.CodeAnalysis.CodeStyle.Fixes (shared code for fixers)
Microsoft.CodeAnalysis.CSharp.CodeStyle (C# code style analyzers, no workspace reference)
Microsoft.CodeAnalysis.CSharp.CodeStyle.Fixes (C# code style fixes)
Microsoft.CodeAnalysis.VisualBasic.CodeStyle (VB code style analyzers, no workspace reference)
Microsoft.CodeAnalysis.VisualBasic.CodeStyle.Fixes (VB code style fixes)

New test projects:

Microsoft.CodeAnalysis.CodeStyle.UnitTests
Microsoft.CodeAnalysis.CodeStyle.CSharp.UnitTests
Microsoft.CodeAnalysis.CodeStyle.VisualBasic.UnitTests

Notes for specific reviewers:

@srivatsn: Please review the inclusion of install.ps1 and uninstall.ps1 in the new analyzer NuGet packages. Are these still necessary (or does NuGet automatically add references now), and if so, is this how you would include them?

@jaredpar I know you care about infrastructure, and also thanks for all the help leading up to this! 👏

@dpoeschl Is this enough of a foundation for us to work in parallel on the migration?

@Pilchie Please review the proposed NuGet package names and manner of distribution, etc.

@tmat Please review the change to BuildNuGets.csx.

jaredpar

Overall structure / infra looks good!

srivatsn · 2017-03-29T20:18:26Z

src/NuGet/Microsoft.CodeAnalysis.CSharp.CodeStyle.nuspec

+    <file src="$thirdPartyNoticesPath$" target="" />
+
+    <!-- Scripts -->
+    <file src="..\..\src\Setup\PowerShell\install.ps1" target="tools\" />


Yes you need these if someone's installing the package to a packages.config-based project which is still the majority of projects out there.

Pilchie

👍 Overall, this looks good.

My big concern is about how we do this transition with incurring perf/dll load issues in the default VS cases.

One approach might be including files in these projects as links/extracting the code out to a shared project. Not sure if people have other ideas. Otherwise, we're looking at 2dll loads for each language, which I'm not sure we'll get approval for without winning them back somewhere else.

Pilchie · 2017-03-29T20:57:32Z

src/NuGet/BuildNuGets.csx

@@ -122,6 +124,8 @@ string[] TestPackageNames = {
 var PreReleaseOnlyPackages = new HashSet<string>
 {
    "Microsoft.CodeAnalysis.Build.Tasks",
+    "Microsoft.CodeAnalysis.CSharp.CodeStyle",
+    "Microsoft.CodeAnalysis.VisualBasic.CodeStyle",


why are these pre-release only? When we ship a final release, we'll want to upload these?

I literally took a guess and went with it, and then added @tmat as a required reviewer. 😆

As the comment above says, are we not going to publish these on nuget?

In reply to: 108894557 [](ancestors = 108894557)

We are planning to publish these on NuGet. Does that mean I should remove them from PreReleaseOnlyPackages from the start (i.e. before they would actually be considered "ready for public release")?

Also, your comment only shows up in the Diff view for some reason. May I ask what client you used to create that comment?

Yes

CodeFlow.

In reply to: 109054195 [](ancestors = 109054195)

Pilchie · 2017-03-29T20:58:20Z

src/NuGet/Microsoft.CodeAnalysis.CSharp.CodeStyle.nuspec

+  </metadata>
+  <files>
+    <file src="Dlls\CSharpCodeStyle\Microsoft.CodeAnalysis.CSharp.CodeStyle.dll" target="analyzers\dotnet\cs" />
+    <file src="Dlls\CSharpCodeStyle\Microsoft.CodeAnalysis.CSharp.CodeStyle.xml" target="analyzers\dotnet\cs" />


Not sure if it makes sense to distribute these for analyzers.

I was thinking the same thing. I'm pretty sure the answer is we don't need them, so I'll go ahead and remove them now.

dpoeschl

Looks like a great starting point. Thanks.

sharwell · 2017-03-30T10:48:33Z

@Pilchie

My big concern is about how we do this transition with incurring perf/dll load issues in the default VS cases.

Oh, I'm worried about this too, but the big perf hit isn't going to be the DLL load. The switch from IDE- to NuGet-based analyzers will cause the code style analyzers to be regularly run against entire code bases, as opposed to only running them against files open in the IDE. I would not be surprised to find this leads to required perf work in both the code style analyzers themselves and the analyzer driver. On the bright side, improvements to the latter could lead to a big perf win for external analyzer projects which are already being shipped as NuGet packages.

jaredpar · 2017-03-30T18:09:38Z

retest ubuntu_14_debug_prtest please

See dotnet/msbuild#1661

alrz · 2017-04-03T07:13:32Z

@sharwell Is there any plan to move fxcop analyzers to these projects? e.g. #16621

https://github.com/dotnet/roslyn-analyzers/blob/569bd373a4831d3035597197e02980b57602a7f2/src/Microsoft.QualityGuidelines.Analyzers/Core/MarkMembersAsStatic.cs

I'm asking because I had a fix for this (#16902) and it's already implemented in roslyn-analyzers repo. I'm not clear if I'm going to implement an analyzer where exactly it should go.

sharwell · 2017-04-03T12:11:45Z

@alrz It was not my original intent for the FxCop analyzers to move to these packages. The main short-term goal is to move several of the existing IDExxxx analyzers away from being IDE-specific to being able to run during command line builds (and thus on CI servers).

We're facing some big hurdles right now related to the fact that some of these analyzers use APIs that are only available while working in the IDE (mostly related to configurable options). I wanted to get the first step of creating these projects out of the way so the 50 changed files you see here don't interfere with our ability to review upcoming proposals related to exposing options to analyzer authors in a uniform way.

ErikSchierboom · 2017-04-08T07:30:30Z

@sharwell I have two questions, which you hopefully can answer:

Are these new packages published on NuGet yet? I cannot seem to find them.
Will these new packages work with .NET Core projects?

jmarolf · 2017-04-08T09:17:35Z

@ErikSchierboom

they have not been published yet, but they should go on https://dotnet.myget.org/gallery/roslyn eventually
They are netstandard1.3 so yes

ErikSchierboom · 2017-04-09T17:10:00Z

@jmarolf Okay, thanks for the response! Looking forward to them being released.

sharwell requested review from srivatsn, tmat, jaredpar, dpoeschl and Pilchie March 29, 2017 16:55

dnfclas added the cla-already-signed label Mar 29, 2017

jaredpar approved these changes Mar 29, 2017

View reviewed changes

srivatsn reviewed Mar 29, 2017

View reviewed changes

Pilchie added the Area-IDE label Mar 29, 2017

Pilchie approved these changes Mar 29, 2017

View reviewed changes

dpoeschl approved these changes Mar 29, 2017

View reviewed changes

sharwell force-pushed the code-style-projects branch from 16d7b40 to f2ed1cc Compare March 30, 2017 17:52

tannergooding mentioned this pull request Mar 30, 2017

Almost empty Strings.resx fails GenerateResources in .NET Core MSBuild dotnet/msbuild#1661

Closed

sharwell force-pushed the code-style-projects branch 6 times, most recently from ede889b to fd14e14 Compare March 30, 2017 20:48

sharwell added 5 commits March 30, 2017 19:53

Add projects for new CodeStyle assemblies

0de615f

Add CodeStyle unit test projects

83fc506

Sign new production assemblies

fdeb0b5

Create NuGet packages for new code style analyzers

ee8d39c

Work around MSBuild bug regarding empty resource files

fadf696

See dotnet/msbuild#1661

sharwell force-pushed the code-style-projects branch from 45db511 to fadf696 Compare March 31, 2017 00:56

sharwell added this to the 15.3 milestone Mar 31, 2017

sharwell merged commit 940f119 into dotnet:master Mar 31, 2017

sharwell deleted the code-style-projects branch March 31, 2017 03:13

sharwell mentioned this pull request Apr 24, 2017

Compilers.sln does not build #18653

Closed

sharwell mentioned this pull request Jul 22, 2017

Mark CodeStyle nugets as prerelease-only until they are truly ready to ship #21021

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add assemblies and packages for new code style analyzer distribution #18285

Add assemblies and packages for new code style analyzer distribution #18285

sharwell commented Mar 29, 2017 •

edited by Pilchie

Loading

jaredpar left a comment

srivatsn Mar 29, 2017

Pilchie left a comment

Pilchie Mar 29, 2017

sharwell Mar 30, 2017

tmat Mar 30, 2017

sharwell Mar 30, 2017 •

edited

Loading

tmat Mar 30, 2017

Pilchie Mar 29, 2017

sharwell Mar 30, 2017

dpoeschl left a comment

sharwell commented Mar 30, 2017

jaredpar commented Mar 30, 2017

alrz commented Apr 3, 2017 •

edited

Loading

sharwell commented Apr 3, 2017

ErikSchierboom commented Apr 8, 2017

jmarolf commented Apr 8, 2017

ErikSchierboom commented Apr 9, 2017

Add assemblies and packages for new code style analyzer distribution #18285

Add assemblies and packages for new code style analyzer distribution #18285

Conversation

sharwell commented Mar 29, 2017 • edited by Pilchie Loading

Notes for specific reviewers:

jaredpar left a comment

Choose a reason for hiding this comment

srivatsn Mar 29, 2017

Choose a reason for hiding this comment

Pilchie left a comment

Choose a reason for hiding this comment

Pilchie Mar 29, 2017

Choose a reason for hiding this comment

sharwell Mar 30, 2017

Choose a reason for hiding this comment

tmat Mar 30, 2017

Choose a reason for hiding this comment

sharwell Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

tmat Mar 30, 2017

Choose a reason for hiding this comment

Pilchie Mar 29, 2017

Choose a reason for hiding this comment

sharwell Mar 30, 2017

Choose a reason for hiding this comment

dpoeschl left a comment

Choose a reason for hiding this comment

sharwell commented Mar 30, 2017

jaredpar commented Mar 30, 2017

alrz commented Apr 3, 2017 • edited Loading

sharwell commented Apr 3, 2017

ErikSchierboom commented Apr 8, 2017

jmarolf commented Apr 8, 2017

ErikSchierboom commented Apr 9, 2017

sharwell commented Mar 29, 2017 •

edited by Pilchie

Loading

sharwell Mar 30, 2017 •

edited

Loading

alrz commented Apr 3, 2017 •

edited

Loading