Skip to content

Conversation

tillig
Copy link
Contributor

@tillig tillig commented Apr 4, 2024

These were issues I had to fix to get Serilog building/testing on a Mac as part of the benchmarking for #1944.

  • The .vscode folder isn't a per-user setting the way .vs is. .vscode is how you provide a default build/test command for VS Code folks. I added that support so things like "Open Repo in VS Code" or other contributors can get going a little faster.
  • The Powershell build script would Push-Location at the start but never Pop-Location, so you'd end up with a stack of history entries. Added a try/finally so at the end - failure or success - it will still pop back to where you were.
  • The local branch build auto-generated semver in the Powershell build didn't like branches like feature/xyz with slashes. Added a fix for that so it'll replace slash with dash.
  • VS Code notoriously uses the first TFM listed in the .csproj file as the one for Intellisense and in-editor analyzers and such. Reordering the TFMs in the .csproj files to go newest-to-oldest instead of oldest-to-newest fixes some problems with Intellisense and contributing.
  • The benchmarks in the SourceContextMatchBenchmark fixture caused some problems due to the targeting specifically of netcoreapp2.1 and netcoreapp3.1. During the execution of the benchmarks, BenchmarkDotNet auto-generates an assembly that targets these frameworks so they can be run. This auto-generated assembly has TreatWarningsAsErrors turned on. In .NET 8, you get a NuGet warning when you try to target netcoreapp2.1 because Microsoft.NETCore.App v2.1.0 has known security issues. All of this adds up to the auto-generated assembly not compiling and the benchmarks not running. Even if you get the netcoreapp3.1 building, you still get issues because System.Diagnostics.DiagnosticSource writes warnings about how it hasn't been tested and isn't supported against netcoreapp3.1. The fix for this is to:
    • Target newer frameworks in the SourceContextMatchBenchmark so you don't get the security warnings.
    • Add the targeted frameworks to the benchmark project so you can catch any build warnings for that target framework up front before the auto-generated assembly gets created.

I'm fine rolling any or all of this back, but this is what it took for me, on a Mac, to get this all working smoothly. Hopefully it can help other devs/contributors to get on board faster.

tillig added 6 commits April 4, 2024 09:13
While this isn't documented as strictly necessary, a problem arises where libraries or runtimes aren't available for the auto-generated BenchmarkDotNet assemblies, which results in warnings and errors failing the benchmarks.

By adding the TFMs to the benchmark assembly, we'll get build errors/warnings up front about things that won't work.

These were also upgraded because libraries like System.Diagnostics.DiagnosticSource throw warnings about not being supported or tested on older platforms like netcoreapp3.1.
Copy link
Member

@nblumhardt nblumhardt left a comment

Choose a reason for hiding this comment

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

Lovely, thank you 👍

@nblumhardt nblumhardt merged commit f5383bc into serilog:dev Apr 4, 2024
@tillig tillig deleted the feature/case-insensitive-benchmarks branch April 5, 2024 01:10
@nblumhardt nblumhardt mentioned this pull request May 27, 2024
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.

2 participants