chore: enable PublicApiAnalyzers on MacOS X #1071
Conversation
The PublicApiAnalyzers is currently set to run only on Windows. I suspect it is so that, during the build process, one would have the Linux builds proceed and provide better insight to other types of failures. But the current setting leaves people using a Mac out in the cold. This commit switches up the condition to be `$(OS) != 'Linux'`, reducing the blast radius. (In reality, a better fix would be to skip these checks if the build process is running in CI, as they also occur during the Code Coverage step. Or maybe they should be a step on its own?)
|
Could you please confirm that it is also working for packages referencing .NET Framework, like OpenTeleetry.Exporter.Geneva? |
|
@Kielek long story short: looks good to me. Against code as in $ dotnet build src/OpenTelemetry.Exporter.Geneva
MSBuild version 17.4.0+18d5aef85 for .NET
Determining projects to restore...
All projects are up-to-date for restore.
OpenTelemetry.Exporter.Geneva -> /Users/mmanciop/git/opentelemetry-dotnet-contrib/src/OpenTelemetry.Exporter.Geneva/bin/Debug/net6.0/OpenTelemetry.Exporter.Geneva.dll
OpenTelemetry.Exporter.Geneva -> /Users/mmanciop/git/opentelemetry-dotnet-contrib/src/OpenTelemetry.Exporter.Geneva/bin/Debug/netstandard2.0/OpenTelemetry.Exporter.Geneva.dll
Build succeeded.
0 Warning(s)
0 Error(s)
Time Elapsed 00:00:02.59When breaking on purpose the PublicAPI.Shipped.txt for dotnet build src/OpenTelemetry.Exporter.Geneva
MSBuild version 17.4.0+18d5aef85 for .NET
Determining projects to restore...
All projects are up-to-date for restore.
OpenTelemetry.Exporter.Geneva -> /Users/mmanciop/git/opentelemetry-dotnet-contrib/src/OpenTelemetry.Exporter.Geneva/bin/Debug/net6.0/OpenTelemetry.Exporter.Geneva.dll
/Users/mmanciop/git/opentelemetry-dotnet-contrib/src/OpenTelemetry.Exporter.Geneva/ExceptionStackExportMode.cs(21,5): warning RS0016: Symbol 'Drop' is not part of the declared API [/Users/mmanciop/git/opentelemetry-dotnet-contrib/src/OpenTelemetry.Exporter.Geneva/OpenTelemetry.Exporter.Geneva.csproj::TargetFramework=netstandard2.0]
/Users/mmanciop/git/opentelemetry-dotnet-contrib/src/OpenTelemetry.Exporter.Geneva/ExceptionStackExportMode.cs(22,5): warning RS0016: Symbol 'ExportAsString' is not part of the declared API [/Users/mmanciop/git/opentelemetry-dotnet-contrib/src/OpenTelemetry.Exporter.Geneva/OpenTelemetry.Exporter.Geneva.csproj::TargetFramework=netstandard2.0]
OpenTelemetry.Exporter.Geneva -> /Users/mmanciop/git/opentelemetry-dotnet-contrib/src/OpenTelemetry.Exporter.Geneva/bin/Debug/netstandard2.0/OpenTelemetry.Exporter.Geneva.dll
Build succeeded.
/Users/mmanciop/git/opentelemetry-dotnet-contrib/src/OpenTelemetry.Exporter.Geneva/ExceptionStackExportMode.cs(21,5): warning RS0016: Symbol 'Drop' is not part of the declared API [/Users/mmanciop/git/opentelemetry-dotnet-contrib/src/OpenTelemetry.Exporter.Geneva/OpenTelemetry.Exporter.Geneva.csproj::TargetFramework=netstandard2.0]
/Users/mmanciop/git/opentelemetry-dotnet-contrib/src/OpenTelemetry.Exporter.Geneva/ExceptionStackExportMode.cs(22,5): warning RS0016: Symbol 'ExportAsString' is not part of the declared API [/Users/mmanciop/git/opentelemetry-dotnet-contrib/src/OpenTelemetry.Exporter.Geneva/OpenTelemetry.Exporter.Geneva.csproj::TargetFramework=netstandard2.0]
2 Warning(s)
0 Error(s)
Time Elapsed 00:00:01.38 |
| @@ -24,7 +24,7 @@ | |||
| <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets> | |||
| <PrivateAssets>all</PrivateAssets> | |||
| </PackageReference> | |||
| <PackageReference Include="Microsoft.CodeAnalysis.PublicApiAnalyzers" Version="$(MicrosoftPublicApiAnalyzersPkgVer)" Condition=" $(OS) == 'Windows_NT' and '$(EnablePublicApi)'!='false'" PrivateAssets="All" /> | |||
| <PackageReference Include="Microsoft.CodeAnalysis.PublicApiAnalyzers" Version="$(MicrosoftPublicApiAnalyzersPkgVer)" Condition=" $(OS) != 'Linux' and '$(EnablePublicApi)'!='false'" PrivateAssets="All" /> | |||
There was a problem hiding this comment.
I would consider even removing system condition completely. It can impact on build time, but should cover scenario when someone is using linux to develop.
@utpilla, any opinions on that?
There was a problem hiding this comment.
I think there is value in letting a platform (Linux) go ahead in the build process and not get stuck at the same step as Windows. It may help iterate faster when PRs have more than one issue.
However, if the public api check were to become a separate step in CI... After all, from what I understand, that analyzer is platform independent.
There was a problem hiding this comment.
This was discussed in the SIG meeting yesterday. We were in favor of removing this condition completely and have it run for all the platforms. It shouldn't add much to CI time as the checks for different platforms should be run in parallel.
However, if the public api check were to become a separate step in CI... After all, from what I understand, that analyzer is platform independent.
Yeah that's a possibility we could explore.
The PublicApiAnalyzers is currently set to run only on Windows. I suspect it is so that, during the build process, one would have the Linux builds proceed and provide better insight to other types of failures.
But the current setting leaves people using a Mac out in the cold.
Changes
This commit switches up the condition to run the PublicApiAnalyzers to be
$(OS) != 'Linux', making it work ootb with Mac OS X.(In reality, a better fix would be to skip these checks if the build process is running in CI, as they also occur during the Code Coverage step. Or maybe they should be a step on its own?)