Allow sdk resolvers to query global msbuild properties

[nguerrera]

Today the context available to sdk resolvers is fixed by https://github.com/Microsoft/msbuild/blob/master/src/Framework/Sdk/SdkResolverContext.cs#L9

Seeing that SolutionFilePath comes from $(SolutionPath),

I wonder if we can just have an extra method on SdkResolverContext that lets resolvers query any propety:

public abstract class SdkResolverContext {
   // ...
   public abstract string GetProperty(string name);
}

This would allow resolvers to document arguments that are passed as plain msbuild properties.

Without this, we're currently using environment variables to override some of our resolver's behavior:

https://github.com/dotnet/cli/blob/09dd14bfe467e1cd264740af6ed4a8a243ccb53a/src/Microsoft.DotNet.MSBuildSdkResolver/MSBuildSdkResolver.cs#L20-L24
https://github.com/dotnet/cli/blob/09dd14bfe467e1cd264740af6ed4a8a243ccb53a/src/Microsoft.DotNet.MSBuildSdkResolver/MSBuildSdkResolver.cs#L94-L98

Our immediate use case was to have a test hook, but there are production scenarios too. The main one is pulling down the .NET Core SDK to an arbitrary, non-global location as part of the build and signaling to the resolver to choose the appropriate sdk from there and not from program files. (Now, that actually overlaps with another feature that's evolving and we will likely land in a place where you can edit global.json to get this behavior without setting any environment variables, but the mechanism here applies more generally to arbitrary resolvers with arbitrary input.)

cc @jaredpar @AndyGerlicher

EDIT (by @dsplaisted): We probably want to do this, but only for global properties. This will allow resolvers to know whether they are running in VS, for example.

[jaredpar]

Very much in favor of this approach. Today virtually anything about the MSBuild experience can be customized via properties. True there are a few cases where you need to be careful about ordering (<UsingTask> for example). But overall you can completely configure the build via properties.

Having SDK deviate from that would be a bit painful.

[rainersigwald]

I'm strongly opposed to breaking the encapsulation of the resolver in this way. Plus, it would be very, very confusing: only global properties would be available in the normal case (<Project Sdk="" />), because at the time of SDK resolution none of the project file has been parsed.

[jaredpar]

The normal case though couldn't really care about this at all. They have no interest in customizing their build process this way. It's only likely going to be interesting to the more complex customers who spend a lot of time customizing their build.

This still seems very analogous to <UsingTask> from my view as a customer. If I know what I'm doing and understand the ordering implication I can safely override values which would normally be loaded.

[rainersigwald]

Clearly we have very different ideas about what this would imply. Can you give an example of a setup that used this feature?

[nguerrera]

I have another use case for passing down custom context to the resolver: VS wanting to say, "Please give me at least version X even if project specifies min=Y and Y < X because I can't work right without X". See dotnet/cli#6585.

I am struck by the simplicity of this approach. We could invent some other mechanism for passing data down to the resolver, but being able to just use properties will make it very easy to plumb the IDE -> resolver case.

I can live with the semantics that it has to be global in order to impact the shorthand syntax. For the IDE->resolver communication, it would be global. And for the advanced build customization scenario, you can refactor the imports to the longer syntax (probably via shared targets/props of your build).

We'd have to document it carefully and while it could be confusing, I don't think it's that hard to explain/justify.

[jaredpar]

@rainersigwald

Can you give an example of a setup that used this feature?

Roslyn. For build sanity reasons we do not want to use the version of the SDK that comes with MSBuild. Every new preview, update, etc ... has some breaking behavior associated with it. This means using the SDK that comes with MSBuild will produce different results that our official builds.

This is both frustrating to developers who get different behavior based on what VS they are using. And frustrating to our infrastructure team as we have to rationalize it all.

Instead we'd like to control the SDK that is used to ensure a consistent build experience in all environments. This is the approach we take for many other parts of our build: reference assemblies, compilers, toolsets, etc ...

From our perspective we want to be able to set a property saying "find SDK here". Otherwise I'm not sure what the developer experience is supposed to be.

[rainersigwald]

@jaredpar No, I mean what goes in what project files/props. I don't understand how you think that'll work.

[jaredpar]

@rainersigwald

I imagine I would set a property specifying the locations from which to resolve SDK imports. Expecting this would override the default locations.

Here is another perspective I look at this problem from. SDK imports are supposed to be a way for SDKs to opt into the MSBuild process. Yet I can't control where the come from in my build. Don't see the logic in that.

[rainersigwald]

@jaredpar Can you please give me some XML? I think I understand your scenario. I don't understand how it could work with the MSBuild evaluation model.

[jaredpar]

@rainersigwald

<SdkResolversPaths>$(NugetPackageRoot)\Microsoft.DotNet.SDK\$(MicrosoftDotNetSdkVersion)\tools</SdkResolversPath>

[rainersigwald]

@jaredpar Where is that in relation to the SDK reference? Is that per-project? In some import?

[jaredpar]

@rainersigwald

It would be in our central properties file. Not sure that should be relevant though. The only item that should be relevant is that it's logically before the <Import ... Sdk=... />

[rainersigwald]

The "central properties file" is discovered only by that import, so it seems like you're in a pickle already.

[rainersigwald]

(at least by default. That's why I'm asking for details here)

[jaredpar]

@rainersigwald

The "central properties file" is discovered only by that import, so it seems like you're in a pickle already.

No. The central properties file is a hard coded path on our machine.

  <Import Project="..\..\..\..\build\Targets\SettingsSdk.props" />

The content of SettingsSdk.props is essentially

<PropertyGroup>

<SdkResolversPaths>$(NugetPackageRoot)\Microsoft.DotNet.SDK\$(MicrosoftDotNetSdkVersion)\tools</SdkResolversPath>
</PropertyGroup>

<!-- lots of other stuff -->
<Import ... Sdk=... />

I feel like we must be talking past each other in some way here. The setup I'm describing of a centralized props / targets file for a repo is well understood. It's a staple of virtually every project I've worked on at Microsoft. Not sure where our disconnect is.

[nguerrera]

We don't need this for dotnet/cli#6585 after all. We'll instead place a data file next to the resolvers installation in VS indicating the minimum sdk version VS needs.

[rainersigwald]

Ok, I've now pieced together that you expect this situation, which is very different from the default SDK use case:

• Some kind of external-to-msbuild setup that populates an SDK location
• Explicit <Import Sdk="" /> in every csproj
• . . . explicitly after an explicit import to common config

And what you want is to have a project instance MSBuild property defined in the common config that points to the private SDK location (what ensures consistency there?).

It sounds like you're ok with someone who creates a new project from template being subtly different/broken.

---

As you mention, this is a road we've gone down many times and it's a huge source of frustration for end-user developers in systems like this designed by build engineers. If possible, I'd like to see us design a system that avoids the required-in-every-project customization, in favor of a more pleasant user experience.

I think that your goal could be better met by something like https://github.com/dotnet/cli/issues/6589.

[jaredpar]

And what you want is to have a project instance MSBuild property defined in the common config that points to the private SDK location (what ensures consistency there?).

Correct.

It sounds like you're ok with someone who creates a new project from template being subtly different/broken.

Broken because they didn't include our standard header? Yep.

This presents basically no fear to us because we have a Jenkins job to verify our common <Import /> is used in every project. If it's missing the PR seeking to add the project will fail.

If possible, I'd like to see us design a system that avoids the required-in-every-project customization, in favor of a more pleasant user experience.

Not sure how that is going to be possible. The Directory.Build.props / targets solution seems to be an attempt at doing this: central settings that are automatic. But as I've pointed out in other threads it misses a number of key scenarios that make it impossible to use in a sufficiently complex project given the current state of the SDK.

[nguerrera]

Hit another use case for this for a product feature of allowing VS to specify whether it accepts previews of the SDK. I agree that https://github.com/dotnet/cli/issues/6589 would be better for some of the things discussed above, but it wouldn't apply to this case.

@rainersigwald @AndyGerlicher @jeffkl How would you feel if the proposal was amended to:

public abstract class SdkResolverContext {
   // ...
   public abstract string GetGlobalProperty(string name);
}

And it would only ever query global properties?

cc @davkean @abpiskunov