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

upgrade paket.core to support net5.0 #2587

Conversation

yazeedobaid
Copy link
Collaborator

@yazeedobaid yazeedobaid commented Apr 13, 2021

Description

This PR upgrades Paket.Core to the latest beta release 6.0.0-beta8 to support .NET 5 in dotnet core build of FAKE. The legacy project is still using Paket.Core version 5.257

During the upgrade, the following has been done:

  1. Removed the Expecto.testAdapter since it is archived by the maintainer (repo link) and conflicts with FSharp.Core in which it needs FSharp.Core < 5.0.0
  2. FSharp.Core has been pinned to 5.0.0 since Packet.Core has FSharp.Core pinned to 5.0.0, please see the following link

I have tested the changes on the two cases for the two issues mentioned below. The new build of fake-cli which uses Paket.Core latest beta build, will parse the lock files generated from Paket CLI tool successfully.

TODO

Feel free to open the PR and ask for help

  • New (API-)documentation for new features exist (Note: API-docs are enough, additional docs are in help/markdown)
  • unit or integration test exists - Since it is an upgrade in the dependencies of FAKE, currently I cannot think of a way to add tests to this.
  • boy scout rule: "leave the code behind in a better state than you found it" (fix warnings, obsolete members or code-style in the places you worked in)
  • (if new module) the module has been linked from the "Modules" menu, edit help/templates/template.cshtml, linking to the API-reference is fine.
  • (if new module) the module is in the correct namespace
  • (if new module) the module is added to Fake.sln (dotnet sln Fake.sln add src/app/Fake.*/Fake.*.fsproj)
  • Fake 5 API guideline is honored

@matthid
Copy link
Member

matthid commented Apr 14, 2021

Since it is an upgrade in the dependencies of FAKE, currently I cannot think of a way to add tests to this.

Given this repository bootstraps itself in the release process. I'm usually not too worried. If I want to be 100% safe I release a prerelease and try to build and release with the prerelease (there were some occasions where I did this). But usually there is no easy way to get the test-suite green with something that is broken.

Btw if you ever need a private channel/chat/question, feel free to add me on twitter (@matthi__d) or send me a mail to the one on the github profile.

@yazeedobaid
Copy link
Collaborator Author

Thanks Matthias !
I Will try that. Do you have any clue why the intellisense.fsx and files that are auto-generated by FAKE in .fake directory got deleted after the run of the script. This is the issue that causes the build to fail after the upgrade in integration tests.

@matthid
Copy link
Member

matthid commented Apr 17, 2021

@yazeedobaid Nothing that pops to my mind, I can tell you that we use Paket.Core APIs to generate those scripts and those scripts are not actually used by FAKE for running the script - they are, as hinted by the name, only used for intellisense in IDE, and strictly speaking not even all IDEs as last time I remember VSCode/Ionide used FAKE APIs instead.
The idea was to (re-)create those fsx-files whenever you run a script, such that after the first run you can have intellisense for all FAKE packages you use.

@matthid
Copy link
Member

matthid commented Apr 17, 2021

I took a brief look, maybe there is some other underlaying issue, and the test Fake.Core.Globbing.Tools.Tests/Test try find tool folder in sub path is failing with a different message. We also don't write the intellisense file on failure and assert that in all integrations tests. So maybe the runner just fails with a different issue and therefore doesn't write the intellisense files (as expected). I'd suggest running those tests standalone (those integration tests just run fake.exe on a particular scenario and check if that works as expected).

@matthid
Copy link
Member

matthid commented Apr 17, 2021

And btw for running standalone I usually just setup the debugger in visual studio to the working directory of the test and the arguments accordingly and run it from there (and git clean the test directory when you are finished)

@yazeedobaid
Copy link
Collaborator Author

@matthid Thanks for the tips really helped
The issue was the System.Reactive package was updated from v4 to v5. I minimized the diffs and it was the cause in which auto-generated files in .fake directory were deleted after the fake runner finishes running the script.
For globbing tests, I also reverted the changes for them. I updated them to use the new process utility APIs but they cause the tests to fail.
I will keep this PR focused on the upgrade of Paket.Core. For globbing tests, In my opinion, I think new integration tests for new APIs in process utility should be added not replacing existing tests for deprecated APIs,. Do you have any other suggestions for them?

@halcwb
Copy link

halcwb commented May 4, 2021

Why is this so necessary update not merged??
@yazeedobaid did some great work, and now it seems that it is stalled.

@github-actions
Copy link

github-actions bot commented Aug 3, 2021

There has not been any activity in this pull request for the last 3 months so it will be closed in 14 days if there is no activity.

@github-actions github-actions bot added the stale label Aug 3, 2021
@kerams
Copy link
Contributor

kerams commented Aug 15, 2021

Should be worth updating to 6.0.0 proper now.

@CumpsD
Copy link

CumpsD commented Nov 9, 2021

@dsyme can you reopen this and have a look at it? without it FAKE will probably die quietly if it won't work for net6.0

@matthid matthid mentioned this pull request Jan 11, 2022
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants