Add dotnet/roslyn-analyzers repo #1849
Conversation
| create mode 100644 src/Text.Analyzers/Setup/AnalyzerReleases.Unshipped.md | ||
|
|
||
| diff --git a/FxCopAnalyzers.sln b/FxCopAnalyzers.sln | ||
| deleted file mode 100644 |
There was a problem hiding this comment.
Is there some other way to selectively disable a .sln file?
There was a problem hiding this comment.
Maybe we can specify one in the build command... but I think we can defer this improvement.
| EndProject | ||
| Project("{778DAE3C-4631-46EA-AA77-85C1314464D9}") = "Roslyn.Diagnostics.VisualBasic.Analyzers", "src\Roslyn.Diagnostics.Analyzers\VisualBasic\Roslyn.Diagnostics.VisualBasic.Analyzers.vbproj", "{70BBA457-2CC1-4929-8FEE-359EBB7C398A}" | ||
| EndProject | ||
| -Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Roslyn.Diagnostics.Analyzers.UnitTests", "src\Roslyn.Diagnostics.Analyzers\UnitTests\Roslyn.Diagnostics.Analyzers.UnitTests.csproj", "{D90E7402-70E7-4A95-A292-47EA163D6309}" |
There was a problem hiding this comment.
I have removed all Unit Test projects. Not sure if we should remove fewer or more projects.
| +done | ||
| + | ||
| +scriptroot="$( cd -P "$( dirname "$source" )" && pwd )" | ||
| +"$scriptroot/eng/common/build.sh" --restore --build $@ |
There was a problem hiding this comment.
This script is basically a port of Build.cmd which really just calls eng/common/build.ps1 --restore --build.
| <_CscToolPath Condition="!HasTrailingSlash('$(_CscToolPath)')">$(_CscToolPath)\</_CscToolPath> | ||
| </PropertyGroup> | ||
|
|
||
| - <Exec Command='"$(_CscToolPath)csi.exe" "$(RepoRoot)eng\GenerateAnalyzerNuspec.csx" "$(NuspecFile)" "$(AssetsDir)\" "$(MSBuildProjectDirectory)" "$(Configuration)" "$(TargetFrameworksForPackage)" "@(_NuspecMetadata)" "@(AnalyzerNupkgFile)" "@(AnalyzerNupkgFolder)" "@(AnalyzerNupkgAssembly)" "@(AnalyzerNupkgDependency)" "@(AnalyzerNupkgLibrary)" "$(_GeneratedRulesetsDir)" "$(_GeneratedEditorconfigsDir)" "@(AnalyzerLegacyRuleset)" "$(ArtifactsBinDir)\" "$(AnalyzerDocumentationFileDir)" "$(AnalyzerDocumentationFileName)" "$(AnalyzerSarifFileDir)" "$(AnalyzerSarifFileName)" "$(AnalyzerConfigurationFileDir)" "$(AnalyzerConfigurationFileName)" "$(_GeneratedGlobalAnalyzerConfigsDir)"' /> |
There was a problem hiding this comment.
csi.exe is not available in the .NET 5 SDK that we are using for building. I ended up "porting" the .csx file to the .cs file. Where porting is just adding imports and wrapping the file contents in a public static void Main(string[] Args).
| + <PropertyGroup> | ||
| + <OutputType>Exe</OutputType> | ||
| + <TargetFramework>net5.0</TargetFramework> | ||
| + </PropertyGroup> |
There was a problem hiding this comment.
Is there anything I need to do to make sure I am not accidentally using prebuilts here?
There was a problem hiding this comment.
In general, even the Microsoft build wants to be able to build a basic app without downloading everything (simple "offline" scenarios) so a clean/basic proj like this should be ok. Looking at the annotated prebuilt report for the project.assets.json of this proj is the way to tell from the end results.
| Restore would conclude that there is a cyclic dependency between Microsoft.CodeAnalysis and Microsoft.CodeAnalysis.Analyzers. | ||
| --> | ||
| - <PackageId>*$(MSBuildProjectFullPath)*</PackageId> | ||
| + <PackageId>*$(MSBuildProjectFullPath.Replace('/', '\\'))*</PackageId> |
There was a problem hiding this comment.
As the comment in the original file says, they want to use the file path as the package name.
But when the "package name" (really the full file path with forward slashes on Linux) is put in the assets.json file, it ends up confusing the version parsing logic which thinks "*/home/foo/bar/baz*/4.5.0" has an invalid version along the lines of /home/foo/bar/baz*/4.5.0.
Changing the slashes back to Windows-style slashes makes this work.
There was a problem hiding this comment.
Bizarre... can you add this to the code comment? (I imagine this can be upstreamed for compat, and would be nice to have explanation inline. If they don't want to find some other way.)
| \ No newline at end of file | ||
| +</Project> | ||
| diff --git a/src/Microsoft.CodeAnalysis.Analyzers/Setup/AnalyzerReleases.Shipped.md b/src/Microsoft.CodeAnalysis.Analyzers/Setup/AnalyzerReleases.Shipped.md | ||
| new file mode 100644 |
There was a problem hiding this comment.
A standalone build of roslyn-analyzers was failing for me with messages like:
/home/omajid/devel/dotnet/roslyn-analyzers/src/Directory.Build.targets(45,5): error : Create a new empty file named 'AnalyzerReleases.Shipped.md' in the project directory [/home/omajid/devel/dotnet/roslyn-analyzers/src/Roslyn.Diagnostics.Analyzers/Setup/Roslyn.Diagnostics.Analyzers.Setup.csproj]
So I did what it asked 🤷
repos/roslyn-analyzers.proj
Outdated
| <RepositoryReference Include="arcade" /> | ||
| <RepositoryReference Include="command-line-api" /> | ||
| <RepositoryReference Include="xliff-tasks" /> | ||
| --> |
There was a problem hiding this comment.
I need to figure out the build dependencies of this. I am not sure how to do that.
There was a problem hiding this comment.
Basically, look at the prebuilt report and add any dependencies that would be resolved by other repos. (For non-nupkg dependencies you have to just know, but those are limited to a few repos.)
eng/Version.Details.xml
Outdated
| <!-- DOes this need a coherent parent dependency? --> | ||
| <Dependency Name="Microsoft.CodeAnalysis.NetAnalyzers" Version="5.0.0-rc1.20428.12"> |
There was a problem hiding this comment.
Always, if possible--sdk has the dependency on roslyn-analyzers:
<Dependency Name="Microsoft.CodeAnalysis.NetAnalyzers" Version="5.0.0-rc1.20428.12">
<Uri>https://github.com/dotnet/roslyn-analyzers</Uri>
<Sha>e856dc126950463c13226c7437fc7e9fb41442dc</Sha>
</Dependency>So here, we can do CoherentParentDependency="Microsoft.NET.Sdk" to link it up.
| create mode 100644 src/Text.Analyzers/Setup/AnalyzerReleases.Unshipped.md | ||
|
|
||
| diff --git a/FxCopAnalyzers.sln b/FxCopAnalyzers.sln | ||
| deleted file mode 100644 |
There was a problem hiding this comment.
Maybe we can specify one in the build command... but I think we can defer this improvement.
| + <PropertyGroup> | ||
| + <OutputType>Exe</OutputType> | ||
| + <TargetFramework>net5.0</TargetFramework> | ||
| + </PropertyGroup> |
There was a problem hiding this comment.
In general, even the Microsoft build wants to be able to build a basic app without downloading everything (simple "offline" scenarios) so a clean/basic proj like this should be ok. Looking at the annotated prebuilt report for the project.assets.json of this proj is the way to tell from the end results.
| Restore would conclude that there is a cyclic dependency between Microsoft.CodeAnalysis and Microsoft.CodeAnalysis.Analyzers. | ||
| --> | ||
| - <PackageId>*$(MSBuildProjectFullPath)*</PackageId> | ||
| + <PackageId>*$(MSBuildProjectFullPath.Replace('/', '\\'))*</PackageId> |
There was a problem hiding this comment.
Bizarre... can you add this to the code comment? (I imagine this can be upstreamed for compat, and would be nice to have explanation inline. If they don't want to find some other way.)
repos/roslyn-analyzers.proj
Outdated
| <RepositoryReference Include="arcade" /> | ||
| <RepositoryReference Include="command-line-api" /> | ||
| <RepositoryReference Include="xliff-tasks" /> | ||
| --> |
There was a problem hiding this comment.
Basically, look at the prebuilt report and add any dependencies that would be resolved by other repos. (For non-nupkg dependencies you have to just know, but those are limited to a few repos.)
omajid
force-pushed
the
add-roslyn-analyzers
branch
2 times, most recently
from
984c3c6 to
e0d0e80
Compare
| + <DotNetExecutible Condition="'$(OS)' == 'Windows_NT'">$(DotNetRoot)\dotnet.exe</DotNetExecutible> | ||
| + <DotNetExecutible Condition="'$(DotNetExecutible)' == ''">$(DotNetRoot)/dotnet</DotNetExecutible> |
There was a problem hiding this comment.
(Nit) DotNetExecutible => DotNetExecutable
But come to think of it, this might be provided already by $(DOTNET_HOST_PATH)... can confirm in the binlog.
No need to reset CI just for this though.
omajid
force-pushed
the
add-roslyn-analyzers
branch
4 times, most recently
from
cfcf7a4 to
4aea905
Compare
|
Thanks, but the CI wont be going green right now: this is dragging too many new prebuilts. It looks like roslyn-analzyers depends on multiple versions of roslyn. For example, from a previous CI run: I will post a longer rant^Wdescription when I have rebased this to master. |
omajid
force-pushed
the
add-roslyn-analyzers
branch
3 times, most recently
from
7729a74 to
9367c1a
Compare
|
I am a bit stumped. Here's the new prebuilts after switching things from One issue is that this repo produces packages like We also need packages (such as |
This seems reasonable--I saw this question on gitter before here and mentioned there:
The dependency on dotnet/roslyn packages makes sense--it'd want to use the newest versions of the Roslyn APIs to poke around the code and analyze it. About why they're showing up as prebuilts--dotnet/roslyn-analyzers might be missing the |
There's some of that, for sure. But it also appears like some rolsyn packages are not being built? |
Hmm. It: https://github.com/dotnet/roslyn/blob/4c195c3ac1974edcefa76774d7a59a2350ec55fa/src/NuGet/Microsoft.CodeAnalysis.Package.csproj But I don't know why. 😕 It seems to me like it'd be reasonable to try patching |
|
I can't see the version of the bincore dll, but the |
|
Opened #1869 to track down if that's an issue or not. (It doesn't seem (directly) related to this PR to me.) |
omajid
force-pushed
the
add-roslyn-analyzers
branch
2 times, most recently
from
df56774 to
fee6e79
Compare
|
The last commit brings the prebuilts down to: The versions are the dogfood versions.
|
patches/roslyn-analyzers/0003-Use-individual-Microsoft.CodeAnalysis-packages.patch
Outdated
Show resolved
Hide resolved
| - <MicrosoftCodeAnalysisVersion>3.8.0</MicrosoftCodeAnalysisVersion> | ||
| + <!-- Source build doesn't produce the Microsoft.CodeAnalysis super-assembly ; use individual packages instead | ||
| + <MicrosoftCodeAnalysisVersion>3.3.1</MicrosoftCodeAnalysisVersion> | ||
| + --> |
There was a problem hiding this comment.
"Metapackage" was the common term for this kind of package that has no content, only dependencies, when I was working with them in dotnet/runtime. And actually, since it doesn't have an assembly I'd expect it to be easy to add back. Filed #1870 for this.
|
It sounds like we need to break the build chain via SBRP at some point? How do we identify if it's at |
omajid
force-pushed
the
add-roslyn-analyzers
branch
from
fee6e79 to
ca8a857
Compare
|
Hmm, I would think that since (If we did need to execute this analyzer, we probably couldn't replace its dependencies with SBRP-built libraries anyway since they need to actually be runnable. I think in that case we'd be better off removing the dependency from the source and making whatever source removals we need so we can make sure it won't accidentally try to use an unavailable library.) |
omajid
force-pushed
the
add-roslyn-analyzers
branch
3 times, most recently
from
20a8874 to
c643b38
Compare
|
Okay, I have reverted my changes to build FxCop and friends. And I have rebased this on top of #1872. CI says we are now back to: What are the next steps here? Should I wait and rebase on top of dotnet/roslyn-analyzers#4419 (and any followups)? Or is this PR okay to go for now? |
|
The steps are:
No need to wait for upstream, the requested changes look nonfunctional to me so far, and we'll have to resolve the patch conflicts later either way. Can fix them up later to make it a more obvious patch removal but we shouldn't wait on that--we need these prebuilts gone. 😄 |
omajid
force-pushed
the
add-roslyn-analyzers
branch
from
c643b38 to
ff98cac
Compare
This repo doesn't build out of the box on Linux, so I have added various build scripts and hacks to get that working.
omajid
force-pushed
the
add-roslyn-analyzers
branch
from
ff98cac to
bb26721
Compare
This repo doesn't build out of the box on Linux, so I have added various build scripts and hacks to get that working.