search

fsprojects / FAKE

FAKE - F# Make
https://fake.build
Other
1.28k stars 581 forks source link

SdkAssemblyResolver: Use dotnet --version to resolve the correct SDK #2665

Closed mclark1129 closed 2 years ago

mclark1129 commented 2 years ago

Description

This PR updates the SdkAssemblyResolver to use dotnet --info in order to resolve the most appropriate runtime for a project.

If available, link to an existing issue this PR fixes. For example:

TODO

Feel free to open the PR and ask for help

cataggar commented 2 years ago

Looks like this may fix #2648 as well.

mclark1129 commented 2 years ago

@yazeedobaid Any idea when someone might be able to take a look at this PR? I logged this issue over a month ago and a proposed fix over a week ago. We have several projects using FAKE in my organization and #2641 is causing a fair amount of headaches with our build agents.

yazeedobaid commented 2 years ago

@mclark1129 Thanks for the PR and sorry for the later response. As I understand this will make the global.json unnecessary and it will resolve the DotNet SDK from whatever the DotNet host is using. And this opens the possibilities to resolve any SDK version, for example, v5. And we don't want that since we have pinned the reference assemblies to Net6 since it is the LTS release.

Can you please confirm/add tests for cases in which if DotNet host is using for example v5 then what output will be? I think we can provide a descriptive message that Fake runner works with Net6 SDK? Or do you have any other ideas?

Also, why not provide the two options? From a global.json and DotNet host?

mclark1129 commented 2 years ago

@yazeedobaid No problem, just want to help out getting this issue resolved!

As far as I understand it, the current code would not allow an SDK other than 6 to resolve its runtime, due to https://github.com/fsprojects/FAKE/blob/202d04e5c0c463b5d46ec52f8d13fa98ee0c1be0/src/app/Fake.Runtime/SdkAssemblyResolver.fs#L62

Since this filters out only releases with a product version of 6.0, the runtime assembly for an SDK other than 6 would fail to resolve.

Also, why not provide the two options? From a global.json and DotNet host?

The issue is that trying to manually read from the global.json doesn't contain enough logic to properly resolve an appropriate local SDK. By using dotnet --version we get to reuse the CLI's logic without having to re-implement it, and it also effectively provides both options. The CLI can resolve from a global.json if present and if not will fall back to the most appropriate for the specified runtime. If that happens to be less than .NET 6, then the build will fail as no runtimes could be found.

I'll confirm the output in the event only .NET 5 or lower is installed, and add test cases. I'm also happy to clean up a bit more in that area (boyscout rule) to help ensure the appropriate error messages are returned with resolution fails. I didn't want to come in with my first PR changing everything around without asking first :).

yazeedobaid commented 2 years ago

@mclark1129 Thanks a lot

mclark1129 commented 2 years ago

@yazeedobaid I've cleaned this up, and I'm overall happy with the solution. However when writing an integration test for rollForward and found another issue here https://github.com/fsprojects/FAKE/blob/release/next/src/app/Fake.DotNet.Cli/DotNet.fs#L666

This code blows up if the global.json SDK doesn't match exactly, which prevents us from using rollForward at all. This doesn't make sense to me, it seems as if we are fighting against the framework itself. If a user wanted to force an exact match of an SDK in their build, they could do so by specifying rollForward: "disable" in global.json. There's no reason I can see for FAKE to enforce that.

On a related note, the whole exercise makes me question why we are doing any of this custom resolution at all. Seems like we could just use System.Environment.Version.ToString() and call it a day. Much like with global.json, a user could pin an exact runtime if they wanted using runtimeconfig.json. I can put in a different PR for that as an example, once I return from vacation.

Currently my integration does not pass due to the issue outlined above, there is also an unrelated Nuget Unit Test failing because it looks like the underlying nuget repo is returning different results from before. I believe this was failing on the previous PR as well. Let me know what you'd like me to do, but I think we need to remove the error in the DotNet module in order for this solution to move forward.

/cc @baronfel @matthid

mclark1129 commented 2 years ago

@yazeedobaid I went ahead and removed the global.json check. As far as I can tell, all tests are passing with the exception of the Fake.DotNet.NuGet.Tests/Search by title returns results for matching packages by provided title. This one is also failing in https://github.com/fsprojects/FAKE/pull/2667 so I'm inclined to believe it's not a result of any of my changes. Do I need to try and fix up that test as a part of my PR in order to move this one forward?

yazeedobaid commented 2 years ago

@mclark1129 if you could help in fixing that failing test that would be great. Thanks!

mclark1129 commented 2 years ago

@yazeedobaid I fixed the failing NuGet test and then ran into another issue with DotNet.uninstallTemplate. Looks like the code was incorrectly throwing an error if the template was not already installed, despite returning exit code 0. I updated the check to first look if the exit code is 0, and if not then it can check to see if the messages indicate that the template was not found. Not sure if that was a behavior change between SDK versions, but I'm not sure why the test couldn't find the message to begin with.

Anyways, dotnet fake build is returning 100% success locally, so hopefully the workflow should complete successfully the next time it runs. No idea yet if we'll see any issues during the linux builds. If you wouldn't mind triggering that as soon as you're able I would appreciate it.

mclark1129 commented 2 years ago

@yazeedobaid TIL I learned how to run the checks from my forked repo! 🚀 There is at least one test still failing due to the fact that dotnet --version changed stdout slightly from what the test was expecting. Doesn't seem to impact windows locally, so possibly some environmental difference on linux. Once I get the checks passing on my fork I'll get you to run them again. Thanks!

mclark1129 commented 2 years ago

@yazeedobaid Ok I have a green run on my fork https://github.com/mclark1129/FAKE/pull/1/checks, would you mind re-running the workflow when you're available?

yazeedobaid commented 2 years ago

Thanks a lot for the PR. I'll try to publish a release as soon as I can.

mclark1129 commented 2 years ago

@yazeedobaid If you can cut a pre-release first I can pull it in and test it out on our builds.

yazeedobaid commented 2 years ago

@mclark1129 It seems the build is failing on macOS. Could you please check?

https://github.com/fsprojects/FAKE/pull/2668