Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dotnet build with multi-targeting #3932

Merged
merged 1 commit into from Nov 27, 2023

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Nov 24, 2023

Motivations

Since #2820, we have had Debug_NetCore and Release_NetCore build configurations that compiled the Core and Test projects targeting .NET Core (now ".NET 7"; I'm not going to try to explain the difference between .NET Core/Standard/5/6/7/etc. and .NET Framework here because it drives me batty). These build options were run by our GitHub build workflows, but not used, and it was easy to forget they existed and thus submit a PR that broke them. They worked by setting the BuildFramework conditionally inside the .csproj files based on the Configuration, which is not great because it tightly couples the framework and configuration when in principle these could be independent. It would be nice to integrate this support better into the main build.

In addition, see #3929 (review), our .NET 7 builds currently emit about 100 warnings about obsolete code, platform-specific code reachable on all platforms, etc.

https://github.com/KSP-CKAN/CKAN/actions/runs/6916657258?pr=3929

image

Changes

  • Now the Debug_NetCore and Release_NetCore configurations are replaced by a NoGUI configuration that builds everything except GUI and AutoUpdater (because both depend on WinForms), which is independent of the target framework
    • This configuration is not needed on Windows, because it is able to build all of the projects for all target frameworks, but it is used on Linux to build the parts that can be built for .NET 7
  • Now each .csproj file's <TargetFramework> is replaced by <TargetFrameworks>net48;net7.0-windows</TargetFrameworks> if it uses WinForms (which .NET 7 considers specific to the Windows platform), or <TargetFrameworks>net48;net7.0;net7.0-windows</TargetFrameworks> otherwise, which will make them always build for both .NET Framework 4.8 and .NET 7.0. Tests and CmdLine use #if checks to include and exclude code that depends on those projects.
  • Now build.cake always builds for both frameworks, and ./build test and .\build.ps1 test run tests for both frameworks
    • On Windows it simply runs dotnet build because it can successfully build all projects for all target frameworks, but on Linux it uses dotnet build for the .NET 7 part of the build and the old Mono msbuild to build for .NET Framework 4.8, because it is the only way to build WinForms code on Linux. Ideally in the future we would find a way to make dotnet build work for everything on Linux as well, but that would require finding a way to install non-Mono WinForms support libraries.
  • The verbosity of the build output is reduced to make it easier to browse and reduce the chance of losing important output to terminal scrollback limits
  • Now build.yml no longer uses the *_NetCore configurations because the main build covers what they used to do
  • Now GUI's images are moved from the .resx file to embedded resources because .NET 7 can't handle non-string resources anymore (some of the devs claim otherwise in various issues on GitHub, but I think their solutions depend on not using .NET Framework anymore)
  • Now all of our Resources.Designer.cs files are deleted and automatically generated from the .resx files during the build, as VS intended, which will simplify translation maintenance
  • Now calls to the obsolete Uri.EscapeUriString are replaced by Uri.EscapeDataString
  • Now calls that .NET 7 believes are platform-specific are guarded by #if checks or check Platform.IsWindows, which is now marked with the SupportedOSPlatformGuard attribute to indicate that it is safe to run platform-specific code if it returns true
  • Now a few usages of WebClient and WebRequest are switched to the newer HttpClient
  • Now the PermissionSet attribute isn't used in .NET 7
  • Now we use SHA1.Create() and SHA256.Create() instead of calling the constructors of SHA1CryptoServiceProvider and SHA256CryptoServiceProvider
  • RedirectWebClient was unused and is removed
  • Now most of GUI is marked with the SupportedOSPlatform attribute so it can use WinForms in .NET 7

This addresses all of those .NET 7 build warnings and brings CKAN another step closer to using a more modern framework. With all our projects now able to build on .NET 7, we have the future opportunity to investigate switching over to it fully. There are many obstacles in the way (cross-platform WinForms, packing dependencies into an EXE, etc.), but all of the unnecessary ones that our build system had created are cleared.

@HebaruSan HebaruSan added Enhancement Build Issues affecting the build system Cake Issues affecting Cake labels Nov 24, 2023
@HebaruSan HebaruSan force-pushed the fix/dotnet-warnings branch 5 times, most recently from 7cb2264 to eca8868 Compare November 25, 2023 19:21
@HebaruSan

This comment was marked as resolved.

Copy link
Member

@techman83 techman83 left a comment

Choose a reason for hiding this comment

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

This is awesome @HebaruSan - yes, every time I got looking at that stuff, my head hurts. Nice to see the warnings gone!

@HebaruSan HebaruSan merged commit 5495c5b into KSP-CKAN:master Nov 27, 2023
8 checks passed
@HebaruSan HebaruSan deleted the fix/dotnet-warnings branch November 27, 2023 04:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build Issues affecting the build system Cake Issues affecting Cake Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants