phmonte / Buildalyzer

A utility to perform design-time builds of .NET projects without having to think too hard about it.
MIT License
589 stars 92 forks source link

Update Roslyn based libraries to 4.0.1 and use .NET 6 SDK #189

Closed slang25 closed 2 years ago

daveaglick commented 2 years ago

I'm going to sit on this one for a little while. I'm hesitant to bump the target because I know of at least a few consumers that haven't moved off of .NET Core 3.1 yet (including Statiq). Unless there's a very compelling need to do so, I'll plan to leave Buildalyzer at .NET Core 3.1 for the foreseeable future...probably until early next year some time. I guess multi-targeting could be an option too, but honestly it's hard enough finding time to maintain one version of this library :).

slang25 commented 2 years ago

That's fair, this PR only updates the tests to .NET 6, I'm happy to put them back, the only thing I'm interested in is updating the Roslyn libraries so that they don't trip up on C# 10 features like global usings & namespace declarations.

I'll do a bit of testing here to see if we even need to do anything here, it may be that I can just update the transitive dependency and it all "Just Works" (although when I created the PR, I'm sure there was a good reason for it, I should have made a note in the description 🤦).

On the maintenance side of things, let me know if I can help, I appreciate investigating MSBuild weirdness is a niche skill set and not one most people want to learn 😆

0xced commented 2 years ago

Another benefit of supporting .NET 6 is that Buildalyzer will properly run on macOS (and probably Linux too). Currently, on .NET Core 3.1, all the GetsSourceFilesFromBinaryLog tests fail with the following exception:

System.PlatformNotSupportedException : COM Interop is not supported on this platform.
  Stack Trace:
     at Microsoft.Build.Shared.BuildEnvironmentHelper.get_Instance()
   at Microsoft.Build.Utilities.Traits.get_Instance()
   at Microsoft.Build.Logging.BinaryLogger.Initialize(IEventSource eventSource)
   at Buildalyzer.Logging.EventProcessor..ctor(AnalyzerManager manager, ProjectAnalyzer analyzer, IEnumerable`1 buildLoggers, IEventSource eventSource, Boolean analyze) in ~/Buildalyzer/src/Buildalyzer/Logging/EventProcessor.cs:line 43
   at Buildalyzer.ProjectAnalyzer.BuildTargets(BuildEnvironment buildEnvironment, String targetFramework, String[] targetsToBuild, AnalyzerResults results) in ~/Buildalyzer/src/Buildalyzer/ProjectAnalyzer.cs:line 162
   at Buildalyzer.ProjectAnalyzer.Build(String targetFramework, BuildEnvironment buildEnvironment) in ~/Buildalyzer/src/Buildalyzer/ProjectAnalyzer.cs:line 140
   at Buildalyzer.ProjectAnalyzer.Build(String targetFramework, EnvironmentOptions environmentOptions) in ~/Buildalyzer/src/Buildalyzer/ProjectAnalyzer.cs:line 132
   at Buildalyzer.ProjectAnalyzer.Build(EnvironmentOptions environmentOptions) in ~/Buildalyzer/src/Buildalyzer/ProjectAnalyzer.cs:line 150
   at Buildalyzer.Tests.Integration.SimpleProjectsFixture.GetsSourceFilesFromBinaryLog(EnvironmentPreference preference, String projectFile) in ~/Buildalyzer/tests/Buildalyzer.Tests/Integration/SimpleProjectsFixture.cs:line 181

It took me about two hours to figure it out: the Microsoft.Build package is restored using .NETFramework instead of the project target framework (i.e. .NETCoreApp,Version=v3.1). There's a warning for that (NU1701) but the Buildalyzer project has <NoWarn>NU1701</NoWarn> everywhere. 😭

I think the safest way to go is to multi-target Buildalyzer with the same supported target frameworks as the Microsoft.Build NuGet package instead of targeting .NET Standard 2.0. This way we can get rid of <NoWarn>NU1701</NoWarn> and all the problems that restoring the .NET Framework dlls on .NET Core can cause.

For reference: Microsoft.Build version Supported target frameworks
16.9.0 .NETFramework 4.7.2 and .NETCoreApp 2.1
16.10.0 .NETFramework 4.7.2 and .NET 5.0
16.11.0 .NETFramework 4.7.2 and .NET 5.0
17.0.0 .NETFramework 4.7.2 and .NET 6.0
slang25 commented 2 years ago

I will change this PR to reflect your suggestion. It's a shame that Buildalyzer needs to depend on Microsoft.Build at all, but I get it.

slang25 commented 2 years ago

I really wish this was a thing: https://github.com/mhutch/designs/blob/3be5566ee2af610fcf37c72b004b254e45427882/accepted/2021/experimental-package-shading/experimental-package-shading.md

daveaglick commented 2 years ago

It took me about two hours to figure it out: the Microsoft.Build package is restored using .NETFramework instead of the project target framework (i.e. .NETCoreApp,Version=v3.1). There's a warning for that (NU1701) but the Buildalyzer project has NU1701 everywhere. 😭

Aw, crap. That's my bad. I ended up yanking my Azure DevOps cross-platform build checks a little while ago with the intent to set up GitHub Actions and then...totally forgot. So I've been thinking it's building and running tests on Linux/Mac this whole time and all was well.

This whole versioning thing is a mess now. We have to upgrade to MSBuild >= 16.10.0 to support some of the newer project stuff. Even though we shell out to whatever MSBuild is on the system, the log format changed and we need the newer MSBuild to handle the incoming piped log messages. But the newer MSBuild targets Framework 4.7.2 and .NET 5.0 - even with multi-targeting that leaves cross-platform .NET Core 3.1 consumers out in the cold. In and of itself that doesn't hugely bother me, it is what it is, except I'm on of those .NET Core 3.1 consumers with Statiq. And since that's the whole reason Buildalyzer was created, I need to keep supporting it.

So...

Ugh. No good answers. At this point though, I'm very tempted to bump Buildalyzer to target .NET 6 along with a major version increment to indicate such and retarget Statiq to .NET 6 as well. I guess it's time to leave the old targets behind. Any reason I should not go this route? Speak now or...

slang25 commented 2 years ago

In my project I'm using MSBuild.Locator so have excluded the msbuild packages, which is probably why stuff is working on macOS for me.

What you are saying makes sense, I think it should be a minimum of 16.10 of the msbuild packages, v4 of the Roslyn workspace packages (to support C# 10), and targeting net6.0, it's an LTS and relativity easy to adopt from .NET Core 3.1

AdaskoTheBeAsT commented 2 years ago

Hi there is some way of keeping also things compatible with .net core 3.1 https://github.com/daveaglick/Buildalyzer/pull/197

please consider this way

daveaglick commented 2 years ago

Closing in favor of #197. I think we've finally got this under control - @AdaskoTheBeAsT figured out how to use a separate version of the StructuredLogging library to keep our MSBuild packages compatible with .NET Core 3.1, so we'll lose .NET Framework support, but I can continue targeting .NET Core 3.1 without multi-targeting. Seems like a good compromise.

Thanks for all the rubber ducking and work on this!