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

Update to faster sets and maps in FSharp.Core 5.0 #3918

Closed
wants to merge 6 commits into from
Closed

Conversation

forki
Copy link
Member

@forki forki commented Oct 22, 2020

The Paket package resolution process relies heavily on sets.

With FSharp.Core 5.0 we now have some performance improvements for sets and maps - so let's use them.

@forki forki force-pushed the fsharp5 branch 2 times, most recently from 0f429b4 to 66cdf2b Compare October 22, 2020 18:46
@forki forki force-pushed the fsharp5 branch 2 times, most recently from 66cdf2b to 5e65729 Compare October 23, 2020 16:44
@forki
Copy link
Member Author

forki commented Oct 23, 2020

@smoothdeveloper I need your help here. I tried to upgrade all the things to netstandard2.1 but now a test complains:

X fcs can type check [3s 195ms]
Error Message:
Expected is <System.Object[0]>, actual is <FSharp.Compiler.SourceCodeServices.FSharpErrorInfo[6]>
Values differ at index [0]
Extra: < <test.fsx (2,7)-(2,36) parse error Package manager key 'paket' was not registered in [C:\projects\paket\integrationtests\Paket.IntegrationTests\bin\Release\net461; C:\projects\paket\integrationtests\Paket.IntegrationTests\bin\Release\net461], [C:\projects\paket\integrationtests\Paket.IntegrationTests....\src\FSharp.DependencyManager.Paket\bin\Release\netstandard2.1]. Currently registered: >, <test.fsx (2,7)-(2,36) parse error Package manager key 'paket' was not registered in [C:\projects\paket\integrationtests\Paket.IntegrationTests\bin\Release\net461; C:\projects\paket\integrationtests\Paket.IntegrationTests\bin\Release\net461], [C:\projects\paket\integrationtests\Paket.IntegrationTests....\src\FSharp.DependencyManager.Paket\bin\Release\netstandard2.1]. Currently registered: >, <test.fsx (2,7)-(2,36) parse warning '' is not a valid assembly name>... >
Stack Trace:
at Paket.IntegrationTests.FsiExtension.fcs can type check () in C:\projects\paket\integrationtests\Paket.IntegrationTests\FsiExtension.fs:line 46

Do you know what's going wrong here?

@smoothdeveloper
Copy link
Contributor

@forki can you try if adjusting this hardcoded "netstandard2.0" thing works:

let pathToExtension = Path.Combine(__SOURCE_DIRECTORY__, "..", "..", "src", "FSharp.DependencyManager.Paket", "bin", configuration, "netstandard2.0")

@forki
Copy link
Member Author

forki commented Oct 24, 2020

I already changed that to nestandard2.1

@smoothdeveloper
Copy link
Contributor

Oh! the output folder for the project has changed for the release script, it is now under bin not src.

@forki
Copy link
Member Author

forki commented Oct 24, 2020

any chance you could send a PR against my branch? I'm lost. also why is it working on travis?

@smoothdeveloper
Copy link
Contributor

Checking the CI on #3922

@lcg-gauthier-segay
Copy link

The current paket FSI extension .dll in releases fails in VS, I assume because the version of fsharp.core used for compilation doesn't match the one that runs in VS.

For it to work out of the box in VS, it needs to reference the same FSharp.Core version as the nuget extension does.

potential fixes in this repository

  • change target net461 in Paket.IntegrationTests project, maybe upgrading it to net48 will allow it to load the netstandard2.0 assembly of the extension
  • change how the extension is built so it uses specific version of FSharp.Core (in case this PR is still blocking, consider a separate build step).

Work around VS & FSharp.DependencyManager.Paket extension

For the extension to load in VS: compiling it from this repository with dotnet build using latest dotnet sdk instead of the one currently locked in the repository.

@forki
Copy link
Member Author

forki commented Dec 2, 2020

@lcg-gauthier-segay @smoothdeveloper I think the problem with this pr is that I wanted to much in one step. Maybe you can try to just update the FSharp.Core in another PR and we keep this one for the next step when we want to go netstandard2.1

@forki forki closed this Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants