Use hostfxr hosting layer API

[Forgind]

This makes the process of finding .NET SDK instances more efficient and fix dotnet/msbuild#7466

[jzabroski]

@rainersigwald I remain perplexed how you can review and/or test quality assurance of any of these changes.

[Forgind]

https://github.com/dotnet/sdk/blob/e3c0cf07006be3eb3d66e84125bb03a967672c33/src/Resolvers/Microsoft.DotNet.NativeWrapper/Interop.cs#L132-L201

[jzabroski]

Hi @forgind

I'm not trying to negatively criticize your improvements here. I just see a lot of changes to MSBuildLocator with no tests. There is a huge pile of assumptions that underlies how MSBuildLocator even works that isn't routinely proven and/or is legacy.

For example, if the goal of this commit is to improve performance, it would be nice to benchmark that.

I'm curious what other build tools like Bazel do for stuff like this.

[Forgind]

@jzabroski,

You're absolutely right that the lack of strong testing infrastructure is a major issue in this repo. My question is: how would you test it? Take this change, for example. This changes how we first find a list of sdks that might have MSBuild. We could try to mock the native call and verify that the mocked native call returns as expected, but that's really just testing the test, not testing the change. We could download a specific set of SDKs and verify that those and only those are found, but that's an end-to-end test, not a unit test; having a long list of those would be very expensive computationally. We could have a short list of end-to-end tests, but at that point, how different is it from manually testing a change?

If you have a good idea for how to write unit tests for MSBuildLocator or feel moved to write a few solid end-to-end tests, we would likely take them. Short of that, I think manual testing is sufficient. This is only issue/pr#161; if it would take our team a lot of effort to create good tests, I don't think it's worth it unless MSBuildLocator suddenly sees a lot more activity.

[jzabroski]

how would you test it?

Docker.

Short of that, I think manual testing is sufficient.

I don't even know how you do manual testing for this stuff. Where is the testing script?

[jzabroski]

To elaborate a bit more, there are two open source projects that allow creating ad-hoc docker containers using the Docker API:

1. https://github.com/Deffiss/testenvironment-docker -> https://www.nuget.org/packages/TestEnvironment.Docker
2. https://github.com/testcontainers/testcontainers-dotnet -> https://www.nuget.org/packages/DotNet.Testcontainers

I prefer the first one. I tried to get involved in the second one, but I found it was fairly difficult to communicate with the maintainers. TestContainers is a broader initiative supporting many different languages, so I imagine some of the change control process involves fitting the overall project's architecture. Each supported language doesn't really simply do a language binding, either, but a ground-up native re-implementation.

Despite the fact Deffiss's TestEnvironment.Docker project has about 4x fewer downloads, I think it is a lot cleaner, especially how they take advantage of C# 9 records' with syntax. The main disadvantage relative to TestContainers.Net is that Deffiss's project doesn't currently support Moby Ryuk for "resource reaping" (garbage collection for Docker test artifacts like images and containers) . In general, that shouldn't be that big of a deal.

Windows also now supports podman, so you can use that in some scenarios, but Docker is still better supported, I believe.

Cheers.

[Forgind]

Docker is certainly better than what I'd originally been imagining. It would let you run tests with different environments in a fairly clean way. On the other hand, there are still only two relevant parts of MSBuildLocator: does it find the MSBuilds you expect, and after you've picked one, does it let you use the API as expected. Using this change as an example, only the first half is relevant, so if I had great tests for it, they'd create an environment with some SDKs and verify that those SDKs are found. I'd also want to make sure that works on both linux and windows. So that's what I tested. I have several SDKs on my windows computer, and I made sure it found the ones I expected it to find and didn't find the ones I didn't expect it to find. I verified that it put the right one at the top, i.e., sorted by version in descending order. Then I tested it on Ubuntu. That testing is why I noticed the PATH variable is different on windows and Ubuntu, hence the latest change. I also noticed that the call to hostfxr_resolve_sdk_2 returned the path to the global.json rather than the path to the best MSBuild, though I haven't figured out an ideal solution to that yet.

The problem is: there aren't very many tests I can run for MSBuildLocator, and I do run those every time I make a change because they build off of each other. I just run them manually rather than automatically. As I said, I'd love to have it all automated; I'm just not sure if it's worth the time at the moment, given everything else we have to do.

[Forgind]

Edit: figured out that last part 🙂

[jzabroski]

I think it's pretty easy to create an image that installs a set of SDKs, and associate that image with negative/positive test cases. Certainly about as much time as manually checking it. I also think it would be cleaner in terms of catching issues, like returning multiple matches or duplicate matches.

Why do you think it would take longer?

I do agree, the initial investment/learning curve is a bit annoying, but once the guidewire architecture for writing tests is in place, the tests write themselves practically.

[Forgind]

What should the version impact of this be? It's theoretically a same-behavior bugfix but it "feels" bigger than that; I'd probably bump the minor version.

That sounds reasonable to me. If we were pushing to MSBuildLocator regularly, I might push back, but we haven't had a version bump in over a year. It's time. I'm thinking we can push a release version after this goes in and the target framework change goes in. Sound good to you?

[Forgind]

Forgot I'd already pushed the version update to the tf PR. Ah, well.