-
Notifications
You must be signed in to change notification settings - Fork 69
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
WIP: Update net and netstandard versions #261
base: master
Are you sure you want to change the base?
Conversation
@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:
|
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi and apologies for only getting back to you on this now, and thanks for helping to keep RProvider up to date!
I've had a look and while I do not think there is any harm in updating the core libraries from netstandard2.0 to netstandard2.1, there are a couple of issues that I think explain the problems you saw trying to run the tests.
- 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.
NB The dotnet version will need reviewing again at the end of this year, with a move to .NET 8 then if possible - or abandomnent of FAKE.)
Again, thanks for contributing!
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.