Target netcore on editor and bootstrapper projects

[glopesdev]

This is an experiment to take one more step towards .NET modernization. Following the changes introduced to support loading embedded resources and building with the .NET SDK for #1873 we can now compile and run WinForms applications on the modern .NET stack.

This PR aims to explore the amount of change required to run the existing IDE stack on .NET core on Windows.

Fixes #1932

[glopesdev]

@PathogenDavid It seems the only way atm to have a sane full CI build is to declare a single TargetFramework so that no folder names need to change, either when repacking or in python scripts, etc.

I was able to achieve this by configuring Bonsai.csproj like so:

<TargetFrameworks Condition="'$(ContinuousIntegrationBuild)' != 'true'">net48;net6.0-windows</TargetFrameworks>
<TargetFramework Condition="'$(ContinuousIntegrationBuild)' == 'true'">net48</TargetFramework>

It turns out the idiosyncrasies of the build process require this condition to be evaluated against properties which bind really early in the build. The only ones I was able to use successfully were either environment variables (which are set before the build even starts), or $(Configuration). Everything else seems to bind too late, including properties set from the dotnet command line.

Initially I thought the above approach might be fine, but it does omit an important part of CI which is to actually try to build the multi-targeting bootstrapper to ensure we haven't broken anything accidentally when targeting .NET core.

So I started thinking it might actually be preferable to create a totally separate build configuration for this (e.g. Pack), which would be used just for release artifacts. If we do this, then Bonsai.csproj could be configured like so:

<TargetFrameworks Condition="'$(Configuration)' != 'Pack'">net48;net6.0-windows</TargetFrameworks>
<TargetFramework Condition="'$(Configuration)' == 'Pack'">net48</TargetFramework>

We would then need to adjust the CI build matrix to include this extra configuration, and possibly modify other CI scripts downstream. It doesn't seem too hard but would be great to hear your thoughts on whether this seems like a reasonable approach before digging too deeply into this.

[PathogenDavid]

For the whole PR: .NET 6 is going out of support in November of this year.

We should skip straight to .NET 8 (which will be supported until November 2026.) We already require the .NET 8 SDK, so I don't think this should be a problem.

[PathogenDavid]

@glopesdev Regarding the issue with the bootstrapper and paths: An easier fix is to just explicitly specify the artifact pivot without the TFM. This avoids any weird behavior differences on CI.

I pushed a fix to your fork as glopesdev@585d0ea which also includes some other related changes as well.

(I've never actually used that feature of GitHub before so hopefully it doesn't anger the branch protection rules 😅)

[glopesdev]

@PathogenDavid thanks for the fix, this might indeed work better. I did notice that the .NET 6.0 build ends up in the package, but at least it's not the repackaged version so it doesn't make a big difference in size at the moment.

That did make me wonder though whether for future .NET Framework / .NET Core bootstrapper builds we want to keep things in the same package or have different packages per platform? For example let's say in .NET Framework we use ILRepack and in .NET Core we use single file publishing, if they both end up in the package it would make it significantly inflated.

Anyway, it might not be too relevant right now, just flagging it to remind us to have a discussion on different possibilities later on.

[glopesdev]

As a related note for later, it is possible to call the pack command with a specific target framework, so for example

dotnet pack Bonsai /p:TargetFrameworks=net48

will pack the project with just the net48 target. Again, there is probably no need for this, and if it turns out we really want to split into different packages we could also just have different .csproj files sharing the same source code.

[glopesdev]

For the whole PR: .NET 6 is going out of support in November of this year.

We should skip straight to .NET 8 (which will be supported until November 2026.) We already require the .NET 8 SDK, so I don't think this should be a problem.

In theory I would completely agree. In practice I've spent the last several hours dealing with breaking code, dependency, and behavioral changes for a full .NET 8.0 migration.

It probably is not wasted time anyway since we are anticipating breakage, but all my form tweaks are broken so the editor doesn't scale correctly anymore as it did on .NET 6.0, new dependencies have been added since old APIs continue to be deprecated and removed, even on Windows platforms (e.g. System.Drawing.Common seems to now be a required dependency on net8.0-windows for some reason; CodeBase and a bunch of other properties have been made obsolete in AssemblyName, etc.)

Will try to digest everything but it may be hard to stomach given all the other stuff we have on our plate.

P.S.: One area where the extra bootstrapper dependencies are particularly annoying is the repackaging process, since we need to be careful deciding and explicitly listing which assemblies get internalized, which makes it much harder to audit package updates.

[PathogenDavid]

I did notice that the .NET 6.0 build ends up in the package

That's typical of mutli-targeted packages.

For example let's say in .NET Framework we use ILRepack and in .NET Core we use single file publishing, if they both end up in the package it would make it significantly inflated.

Yeah let's burn that bridge when we cross it. My gut feeling is to just eat the extra bandwidth during the transition period.

Alternatively we could just remove the binaries from the bootstrapper package entirely. They're only really needed for the janky self-updating feature we've previously talked about removing.

As a related note for later, it is possible to call the pack command with a specific target framework, so for example

I'm not sure that this is an intended feature. Packing is supposed to be a target framework-agnostic operation.

If we want to go down the route of only including .NET 4.8 binaries in the bootstrapper package we should just set IncludeBuildOutput and IncludeSymbols to false for the modern .NET target framework.

I've spent the last several hours dealing with breaking code, dependency, and behavioral changes for a full .NET 8.0 migration.

I'm surprised to hear that much was broken by it. We can merge .NET 6 for now then and I can deal with the .NET 8 issues.

P.S.: One area where the extra bootstrapper dependencies are particularly annoying is the repackaging process, since we need to be careful deciding and explicitly listing which assemblies get internalized, which makes it much harder to audit package updates.

Not totally sure I follow here since any new dependencies should be able to be modern .NET-only and repacking only applies to .NET Framework.

[glopesdev]

Alternatively we could just remove the binaries from the bootstrapper package entirely. They're only really needed for the janky self-updating feature we've previously talked about removing.

This is a good idea and I agree most likely the most sensible way to move forward, especially when NuGet version constraints are working properly, it's fine to leave it as is, no need to worry about custom pack targets.

I'm surprised to hear that much was broken by it. We can merge .NET 6 for now then and I can deal with the .NET 8 issues.

I think I've now been able to nail down most of it, will push soon. The only thing I don't currently have a solution for right now is AssemblyName.ProcessorArchitecture got obsoleted and the workaround turns out to be really messy where previously it was a one-liner. To be fair, I think the solution might lead us to resolve other blockers for proper Roslyn integration with reference frameworks, but maybe for now I will just pragma ignore the warning.

Not totally sure I follow here since any new dependencies should be able to be modern .NET-only and repacking only applies to .NET Framework.

That's correct, but unfortunately when upgrading some packages like NuGet client they also included new dependencies for .NET framework, but we can just decide not to upgrade right now.

[PathogenDavid]

That's correct, but unfortunately when upgrading some packages like NuGet client they also included new dependencies for .NET framework, but we can just decide not to upgrade right now.

Yeah updating the NuGet client stuff should probably be out of scope for this PR if it isn't strictly necessary.

[glopesdev]

@PathogenDavid ok ready for review again. It was challenging but all main .NET Core issues have been resolved now, and I guess it was good that these were preempted now as it makes it much more clear what needs to be resolved, likely better than getting these the first time when transitioning to 3.0.

[PathogenDavid]

On .NET 8 the gear button for opening the package source dialog is invisible as it's getting out of its box:

This possibly only happens at 100% DPI scale. (#1080pGang)

Easiest fix is probably to reduce the label's width a little bit, but I didn't dig super far.

[glopesdev]

On .NET 8 the gear button for opening the package source dialog is invisible as it's getting out of its box:

I fixed it by reducing the right margin, since changing the label size made the text overflow into two lines on .NET Framework at 100% scale.

In general I already noticed many scale factors which need to be reviewed and tweaked for .NET 8 since it seems like they are using different rules which I don't quite understand yet. This was similarly painful to figure out just for scaling different DPI rules in .NET Framework, I always had to test at 3 or 4 different scaling levels to make sure it was working, so now it will be that times two for .NET Core.

I'm leaving all cosmetic issues which do not have functional implications like this one for the next milestone, since we are not officially releasing this editor version yet.