Open EverybodyKurts opened 1 year ago
@EverybodyKurts I think we'd better make you maintainer on this repo
@dsyme I could become a maintainer :D. I'd need help with some of the inner workings of msbuild syntax.
I do not know much of the inner workings of this repo either, and while i get bumping .NET versions i am not sure about increasing the netstandard version. Isn't the rationale behind targetting netstandard2.0 trying to be as backwards-compatible as possible?
via https://learn.microsoft.com/en-us/dotnet/standard/net-standard?tabs=net-standard-2-0:
However, if you want to share code between .NET Framework and any other .NET implementation, such as .NET Core, your library should target .NET Standard 2.0
Hi all, I am currently on paternity leave so have been slow to reply but will look through this and review today.
The RProvider supports net5.0 as the minimum version to run on. As part of the conversion from .net Framework to .net core, the minimum version we could support was .net v5.0.
I am not aware of any changes that would require us updating the framework versions beyond this (or netstandard2.0 where that is used in some projects). Keeping on v5 maintains backwards compatibility with .net 5 and .net 6.
I will look through the PR later today
So I'm re-examined this pull request and tried to build the tests solution:
$ dotnet build RProvider.Tests.sln
MSBuild version 17.8.3+195e7f5a3 for .NET
Determining projects to restore...
All projects are up-to-date for restore.
RProvider.Runtime -> /Users/mueller.128/Developer/comrit/RProvider/src/RProvider.Runtime/bin/Debug/net8.0/RProvider.Runtime.dll
RProvider.Server -> /Users/mueller.128/Developer/comrit/RProvider/src/RProvider.Server/bin/Debug/net8.0/osx-x64/RProvider.Server.dll
RProvider.DesignTime -> /Users/mueller.128/Developer/comrit/RProvider/src/RProvider.DesignTime/bin/Debug/net8.0/RProvider.DesignTime.dll
RProvider -> /Users/mueller.128/Developer/comrit/RProvider/src/RProvider/bin/Debug/net8.0/RProvider.dll
error FS3031 : The type provider '/Users/mueller.128/Developer/comrit/RProvider/src/RProvider/obj/Debug/net8.0/ref/RProvider.dll' reported an error : Assembly attribute 'TypeProviderAssemblyAttribute' refers to a designer assembly 'RProvider.DesignTime' which cannot be loaded from path '/Users/mueller.128/Developer/comrit/RProvider/src/RProvider/obj/Debug/net8.0/ref/RProvider.DesignTime.dll'. The exception reported was: System.IO.FileNotFoundException - Could not load file or assembly '/Users/mueller.128/Developer/comrit/RProvider/src/RProvider/obj/Debug/net8.0/ref/RProvider.DesignTime.dll'. The system cannot find the file specified. [/Users/mueller.128/Developer/comrit/RProvider/tests/Test.RProvider/Test.RProvider.fsproj]
FSC : warning FS3005: Referenced assembly '/Users/mueller.128/Developer/comrit/RProvider/src/RProvider/obj/Debug/net8.0/ref/RProvider.dll' has assembly level attribute 'Microsoft.FSharp.Core.CompilerServices.TypeProviderAssemblyAttribute' but no public type provider classes were found [/Users/mueller.128/Developer/comrit/RProvider/tests/Test.RProvider/Test.RProvider.fsproj]
Again, for some reason it looks for src/RProvider/obj/Debug/net8.0/ref/RProvider.DesignTime.dll
which does not exist. I don't know exactly where to look in the build script to the dll placed in that directory.
* We don't use `dotnet build` directly to build and test, but use FAKE, which is currently only supported on .NET 5 (FAKE v5) and .NET 6 (FAKE v6).
If you are still interested in following up on this PR, I suggest the following may be the way forward:
* Keep the update of all core libraries from netstandard2.0 to netstandard2.1, as we are no longer supporting full .NET framework anyway (which would be the reason to stick to 2.0). * Instead of .NET 7, we move to target the current oldest supported LTS version of .NET, which is currently .NET 6. * Update FAKE to the latest (.NET 6) version and get the build script - build.fsx - working and testing successfully. * Make sure github actions are using the .NET 6 sdk.
I wonder if we could move Fake to a "Build" project, like how Ionide does it. It seems like that's the standard convention nowadays. That way, we aren't necessarily beholden to .net6
https://github.com/ionide/ionide-vscode-fsharp/tree/main/build
Hi thanks again for the ideas. Moving to a proj file sounds like a good step to prepare for using future SDK versions that I think would be fine to implement.
Having thought more about the framework targets, the discussion about netstandard is not relevant, as the RProvider relies on running the built-in executable that requires .NET 5 or greater to be installed. I think the best plan is therefore to move all projects in the solution to target net6.0 (as the current oldest supported LTS) to maintain compatability, but move to net8.0 (enabled by the above proposed FAKE project change) later in the year.
Proposed Changes
Bump net & netstandard versions... specifically from net5.0 to net7.0 and netstandard2.0 to netstandard2.1
Types of changes
What types of changes does your code introduce to RProvider? Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Currently, I can not get tests to even run... this is why this pull request is a work-in-progress. Heres what happens when I currently attempt to build the test solution:
Basically, the build process is looking for
RProvider.DesignTime.dll
inRProvider/src/RProvider/obj/Debug/net7.0/ref/
and since the dll doesn't exist there, it throws an error.Unfortunately, I don't know enough about msbuild syntax to figure out how to troubleshoot this myself.
Further comments
This is a relatively minor, yet important, change... especially for newcomers.