Fallback on failed global.json SDK resolution in VS

[sfoslund]

Fixes #11238

Tested manually in VS:

[wli3]

Looks good so far. Any reason it is still in draft?

[sfoslund]

Any reason it is still in draft?

@wli3 I'm waiting until my MSBuild change (dotnet/msbuild#5562) flows here before I take it out of draft.

[dsplaisted]

Looks like there are some resources conflicts you'll need to resolver.

[dsplaisted]

Does this target run during design-time build? I think not, but can you make sure we don't prevent resolving framework references and NuGet packages so there's a decent intellisense experience.

[sfoslund]

I don't have a good understanding of design time builds, so I don't know. Right now this target runs before resolving framework references and packages in the build, does that interfere with intellisense? Should I hook it in later in the build?

[dsplaisted]

There are targets which gather the references to pass to the compiler for intellisense and other tooling. We don't want to prevent those targets from running successfully.

I think that this target probably isn't run for the design time build, but check to make sure that when the global.json resolution fails, that in VS you can add a PackageReference and get Intellisense for APIs from the package as well as from .NET Core.

If the target is run, then we can avoid the check in design time builds, something like this:

sdk/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets

Lines 312 to 316 in 9b6505e

	<!-- For design-time builds, don't generate an error if a targeting pack isn't available (ie because it hasn't been restored yet) -->
	<PropertyGroup Condition="'$(GenerateErrorForMissingTargetingPacks)' == ''">
	<GenerateErrorForMissingTargetingPacks>true</GenerateErrorForMissingTargetingPacks>
	<GenerateErrorForMissingTargetingPacks Condition="'$(DesignTimeBuild)' == 'true'">false</GenerateErrorForMissingTargetingPacks>
	</PropertyGroup>

[sfoslund]

I see, I did some manual testing and I was able to add a package and get intellisense for the package APIs, so we should be good here.

[dsplaisted]

Instead of creating these properties specifically, could you create a global.json pointing to an invalid version? Then you could also have to variants of the test, one with BuildingInsideVisualStudio true, and one false, and test the different behaviors.

This would be a full framework only test, since the resolver isn't used for .NET Core MSBuild. And I'm not sure if we hook up the SDKResolver for full framework tests or not, so it might not easily be possible. But if the SDKResolver already is hooked up, I'd go ahead and try to make the change.

[sfoslund]

It doesn't seem like the SDK resolver it hooked up, I haven't been able to write this type of test.

[dsplaisted]

Now I'm remembering more. There's not a good test hook for adding an SDK resolver. That prevents us from testing the MSBuild SDK resolver in end-to-end scenarios, and will also prevent us from testing the workload resolver on full framework.

I think as a follow-up, we should try to add a test hook in MSBuild for this. IE when looking for SDK resolvers, if an MSBUILDADDITIONALSDKRESOLVERSFOLDER environment variable is specified, then we should look there for SDK resolvers in addition to the SdkResolvers folder in the MSBuild bin directory. If there are folder names with the same name in both paths, the one from the test hook should override the built-in one (so we can test changes to a resolver that ships in MSBuild).

Could you file an MSBuild issue to add the test hook, and and SDK issue to add end-to-end tests for the global.json resolution once that's done?

[sfoslund]

That makes sense, I've filed dotnet/msbuild#5618 and #12810. I'll keep these in my backlog of work, I should be able to get to them in the next few weeks.

[dsplaisted]

Looks good to merge 👍