MSBuild SDKs version property is not expanded

[andrew-boyarshin]

Steps to reproduce

Directory.Build.props file:

<Project>

  <PropertyGroup>
    <LocalPackageVersion>2.0.0-local</LocalPackageVersion>
  </PropertyGroup>

  <Import Project="Sdk.props" Sdk="SharpGenTools.Sdk" Version="$(LocalPackageVersion)" Condition="'$(MSBuildProjectExtension)' == '.csproj'" />

</Project>

Expected behavior

SharpGenTools.Sdk/2.0.0-local is resolved and imported.

Actual behavior

SharpGenTools.Sdk/$(LocalPackageVersion) fails to be resolved by NuGet resolver.

Log:

Resolving SDK 'SharpGenTools.Sdk/$(LocalPackageVersion)'...

Environment data

Process = "C:\Program Files\dotnet\dotnet.exe"
MSBuild executable path = "C:\Program Files\dotnet\sdk\5.0.100-preview.5.20261.11\MSBuild.dll"
Command line arguments = ""C:\Program Files\dotnet\sdk\5.0.100-preview.5.20261.11\MSBuild.dll" -maxcpucount -verbosity:m /t:Restore;Build /bl "-distributedlogger:Microsoft.DotNet.Tools.MSBuild.MSBuildLogger,C:\Program Files\dotnet\sdk\5.0.100-preview.5.20261.11\dotnet.dll*Microsoft.DotNet.Tools.MSBuild.MSBuildForwardingLogger,C:\Program Files\dotnet\sdk\5.0.100-preview.5.20261.11\dotnet.dll""
MSBuild version = "16.7.0-preview-20258-02+26f6d1d87"

Source

ProjectImportElement.ParsedSdkReference contains unexpanded properties from XML element, but Evaluator.ExpandAndLoadImportsFromUnescapedImportExpressionConditioned doesn't expand them before passing to SDK resolvers.

---

Cross-referencing my SharpGen branch.

[jeffkl]

This is not supported as SDKs are parsed before any part of the project like properties, items, etc. I have an open issue to log an error if you try to put expandable characters in there.

#1518

[andrew-boyarshin]

@jeffkl

This is not supported as SDKs are parsed before any part of the project like properties, items, etc.

Even explicit imports? I was under impression these were evaluated simultaneously with the properties during the 1st pass. That's why you can put a Condition on an Import.

I have an open issue to log an error if you try to put expandable characters in there.

Well, logging is great, but isn't it better to just expand the properties (and log error only when evaluation has failed)?

I use explicit imports, without this working I have to force PackageVersion to always be sth like 42.42.42, and even then, this issue makes it impossible to run tests after building a tagged release.
I can put together a PR for that, if MSBuild team considers it okay.

[jeffkl]

Yes technically explicit imports would be possible to make work. But we also need to consider the pros and cons of making it work in one scenario (<Import />) but not in another (<Project Sdk="" /> and <Sdk />). I'm a little worried that it would cause confusion. That said, <Import /> elements have conditions and are different from <Project /> but <Sdk /> looks the same but is treated a lot differently under the covers.

[andrew-boyarshin]

@jeffkl but is this consistency worth more than feature-completeness? From my PoV, <Import /> is already inconsistent. Some properties are evaluated, some are not. It's the choice between 2 inconsistencies, one is more feature-complete than the other, and that one actually blocks some legitimate scenarios.

[japj]

As an alternative you can put the version info in a global.json as described at https://docs.microsoft.com/en-us/visualstudio/msbuild/how-to-use-project-sdk?view=vs-2019#how-project-sdks-are-resolved

[andrew-boyarshin]

@japj but that doesn't allow referencing MSBuild properties, does it? This is kind of the whole point of this issue. Imagine there is a lot of logic to compute version property. There is no way to use the computed value to resolve SDK. There is no benefit from moving from explicit versioned SDK imports to (explicit non-versioned SDK imports)+(global.json).

[rainersigwald]

The version of an imported SDK must match across the entire build session, so I'm not sure it makes sense to support property expansion there.

that one actually blocks some legitimate scenarios.

Can you expand on that, please?

[andrew-boyarshin]

@rainersigwald SharpGen repo has a custom versioning logic. If repo is built on local PC (not via Azure DevOps CI), version string is $(VersionPrefix)-local (sth like 2.0.0-local), whereas on CI it can either be a release build (version string is taken verbatim from build environment variable), or a nightly build (with VersionSuffix generated by AzDO, like 2.0.0-ci.1024). This logic is, naturally, in Directory.Build.props. The repo dev workflow steps are:

• Build
• Pack
• Copy built *.nupkgs to SdkTests\LocalPackages
• Restore & Build & Run SdkTests\SdkTests.sln

SdkTests must know the just-built package version to restore. This is only possible if these properties become expandable. I have implemented a workaround for now (change all non-release builds to use 42.42.42 as a version), but that still makes it impossible to run SdkTests when building a tagged release.

I have already fixed this issue in my local MSBuild branch and I've refactored ProjectImportElement along the way. It still needs some work (add proper tests, for instance).

By the way, one can inject properties visible in <Project Sdk="..." /> and <Sdk /> via command line or via Directory.Build.rsp. It's just a matter of knowing how flexible MSBuild really is (or how to make it so).

[Nirmal4G]

one can inject properties visible in <Project Sdk="..." /> and <Sdk /> via command line or via Directory.Build.rsp.

This is what I wanted to do. See: #1518 (comment)

#5378 (comment)

I personally welcome this change but if we're allowing versions then we can move the MinimumVersion/MaximumVersion representation to global json. Thus we don't need to add additional version attributes. Either we specify a version or don't, which will infer from global json. For SDK packages from NuGet, specifying dependencies of required base SDK packages should be more than enough for resolving dependent SDKs transitively. NuGetSdkResolver should be able to do just that!