Adding test hook for SDK resolver end to end tests

[sfoslund]

Fixes #5618

[benvillalobos]

/azp run

[azure-pipelines]

No pipelines are associated with this pull request.

[benvillalobos]

This was closed/reopened because it ran into #5646.

[sfoslund]

@dsplaisted I've updated this PR so it now uses the resolvers from RootFolder unless there is one with a matching name under the directory pointed to by the new environment variable, in which case the resolver from the environment variable wins.

[Forgind]

If it is not obvious what the type of a variable is, do not use var. That just makes it harder to read.

[sfoslund]

If it is not obvious what the type of a variable is, do not use var. That just makes it harder to read.

@Forgind is there anywhere in particular you'd prefer I not use var?

[Forgind]

My preference is to never use var unless you actually don't know the type of the variable. Most people are a little more lax and say you can/should use var if the right side of the equation is "new ___", since that's pretty obvious. If you assign a new variable to the return value of a function, and you know the type, I can't think of any reason to use var.

[dsplaisted]

I think there are strong opinions on both side of the var debate. I tend to like using it, especially for generic types which can get long.

[sfoslund]

@Forgind personally I'm with Daniel, I like using it pretty universally. That also seems to be the pattern in this file (there are very few places where it's not used) so I was trying to stick with precedence.

[sfoslund]

@dsplaisted does any of your current resolver work depend on this? In case it does, I've pushed the SDK side of this change (dotnet/sdk#12875) which should work once this flows. That way you won't be blocked in case this doesn't go in this week (as I'll be OOF mon-weds next week).

[dsplaisted]

@dsplaisted does any of your current resolver work depend on this? In case it does, I've pushed the SDK side of this change (dotnet/sdk#12875) which should work once this flows. That way you won't be blocked in case this doesn't go in this week (as I'll be OOF mon-weds next week).

It will be helpful to have this for my resolver work. I need the test hook for automated tests on full Framework MSBuild, and the code to locate workloads is different on Full Framework, so it would be very good to be able to test it.

[Forgind]

Can we compromise on type vs. var in the places I pointed to? I selected 4/21 as the most potentially confusing, whereas others (like the environment variables) aren't at all confusing; they just look weird to me.

[benvillalobos]

For the record I'm anti-var (outside of longer type names) 😁 I had to "go to definition" a few times to make sure I knew what was being returned. Consider the var changes as nits from me.

[cdmihai]

There already exists an environment variable based way of specifying sdk resolver paths: https://github.com/cdmihai/msbuild/blob/ba9a1d64a7abf15a8505827c00413156a3eb7f62/src/Shared/BuildEnvironmentHelper.cs#L580

Why add additional mechanisms instead of reusing / enhancing the existing one? (for enhancing it, we could change the existing env var to contain a semicolon delimited list of paths)

[dsplaisted]

There already exists an environment variable based way of specifying sdk resolver paths: https://github.com/cdmihai/msbuild/blob/ba9a1d64a7abf15a8505827c00413156a3eb7f62/src/Shared/BuildEnvironmentHelper.cs#L580

Why add additional mechanisms instead of reusing / enhancing the existing one? (for enhancing it, we could change the existing env var to contain a semicolon delimited list of paths)

The existing mechanism lets you set a path for SDKs. We want to be able to override an individual SDK Resolver so that we can specifically test the resolver behavior. For example, see dotnet/sdk#12875, and we'd also like to use the test hook to test the workload resolver.

[sfoslund]

@benvillalobos @Forgind I've updated with your feedback, thanks!

[Forgind]

LGTM

[benvillalobos]

LGTM!