Skip to content
New issue

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

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

Already on GitHub? # to your account

Upgrade to fcs 39 #93

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

MangelMaxime
Copy link
Contributor

@MangelMaxime MangelMaxime commented Apr 13, 2021

Related to #88

Note: It seems like there was a lot of white spaces in the original files and because my IDEs remove them it make a lot of noise in the diff.

It is possible in GitHub to hide the white space to make it easier to read the diff:

image

If that's really a problem, I could redo all the changes from master but I would prefer to just remove the white spaces from the files and add a .editorconfig file to ensure the behaviour across the different IDEs/developer machine. .editorconfig is file understand by most of the IDEs/Editors allowing to configure things like tabs size, removal of white space, etc.


Current status:

  • Upgrade to FCS 39
  • Change the project to target net5.0
  • [ ] Change the path in the extension to use net5.0 instead of netcoreapp3.0 release
  • Fix warnings
    • Warnings due to Array decomposition

image

- "Possible incorrect indentation"
  • There is a regression in tests/FSharpLanguageServer.Tests. The tests testing is .fsx files works is failing but it is not failing on master branch
  • There is a failing test in tests/ProjectCracker.Tests but it is already failing in the master branch so not related to my changes. I will see if I can fix it or if I move directly to test Ionide/dotnet-proj library

This test starting failing when net5.0 has been introduced on the CI machine. On this build we can see that the Windows build is green while the Linux build is red.

The difference is Windows build doesn't have net5 while Linux does:

image

image

  • I have a new warning when compiling the projects
fsharp-language-server\src\FSharpLanguageServer\FSharpLanguageServer.fsproj : warning NU1605: Detected package downgrade: FSharp.Core from 5.0.1 to 5.0.0. Reference the package directly from the project to select a different version. 
fsharp-language-server\src\FSharpLanguageServer\FSharpLanguageServer.fsproj : warning NU1605:  FSharpLanguageServer -> FSharp.Compiler.Service 39.0.0 -> FSharp.Core (= 5.0.1)
fsharp-language-server\src\FSharpLanguageServer\FSharpLanguageServer.fsproj : warning NU1605:  FSharpLanguageServer -> FSharp.Core (>= 5.0.0)

I don't know why I have this problem. I tried forcing FSharp.Core version in the fsproj but still get the warning.

When executing dotnet add package FSharp.Core, I have this error:

$ dotnet add src/FSharpLanguageServer/ package FSharp.Core
  Determining projects to restore...
  Writing C:\Users\Maxime\AppData\Local\Temp\tmpFC1C.tmp
info : Adding PackageReference for package 'FSharp.Core' into project 'C:\Users\Maxime\Documents\Workspace\Github\fsprojects\fsharp-language-server\src\FSharpLanguageServer\FSharpLanguageServer.fsproj'.
info :   CACHE https://api.nuget.org/v3/registration5-gz-semver2/fsharp.core/index.json
info : Restoring packages for C:\Users\Maxime\Documents\Workspace\Github\fsprojects\fsharp-language-server\src\FSharpLanguageServer\FSharpLanguageServer.fsproj...
info : Package 'FSharp.Core' is compatible with all the specified frameworks in project 'C:\Users\Maxime\Documents\Workspace\Github\fsprojects\fsharp-language-server\src\FSharpLanguageServer\FSharpLanguageServer.fsproj'.
error: Error while performing Update for package 'FSharp.Core'. Cannot edit items in imported files - 
error:   Item 'PackageReference' for 'FSharp.Core' in Imported file 'C:\Program Files\dotnet\sdk\5.0.202\FSharp\Microsoft.FSharp.NetSdk.props'.

@georgewfraser
Copy link
Collaborator

Hey @MangelMaxime this is still marked WIP---is this ready for review? Can you rebase on master?

I agree with you about .editorconfig

@MangelMaxime
Copy link
Contributor Author

Hello @georgewfraser,

I guess it depends on what we want to ship. I think it should be possible to make a ready for review.

The only regression we have compared to the master branch is that .fsx support doesn't seems to work anymore.

Initially I wanted to also make the switch from custom project cracker to using dotnet-proj but I think I will make it in another PR. So we can have a first release for the FCS update.

I will try to make the required adjustment to the PR for a review this weekend.

@MangelMaxime MangelMaxime changed the title WIP: Upgrade to fcs 39 Upgrade to fcs 39 Apr 30, 2021
@MangelMaxime
Copy link
Contributor Author

Hello @georgewfraser,

I rebase this PR on master.

Compared to the master branch there is one failing which concern the .fsx support. I remove the WIP tag because I have no idea how to fix this issue and perhaps another contributor of this repo can send a PR to fix the issue.

When #92 is merged, it will be easier to read the diff because this PR depends on it.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants