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

[mclark1129]

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:

• fixes SdkAssemblyResolver does not properly resolve locally installed runtime #2641

TODO

• Tests, but want to make sure the solution is acceptable first.

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 (or short reasoning why it doesn't make sense)

  Note: Consider using the CreateProcess API which can be tested more easily, see https://github.com/fsharp/FAKE/pull/2131/files#diff-4fb4a77e110fbbe8210205dfe022389b for an example (the changes in the DotNet.Testing.NUnit module)

• 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

[cataggar]

Looks like this may fix #2648 as well.

[mclark1129]

@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]

@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]

@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

FAKE/src/app/Fake.Runtime/SdkAssemblyResolver.fs

Line 62 in 202d04e

|> List.find (fun product -> product.ProductVersion.Equals(this.SdkVersionRaw))

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]

@mclark1129 Thanks a lot

[mclark1129]

@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]

@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 #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]

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

[mclark1129]

@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]

@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]

@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]

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

[mclark1129]

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

[yazeedobaid]

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

#2668