Msbuild: <Sdk> element does not respect Conditions

[dfederm]

Steps to reproduce

Project file

<Project>
  <Sdk Name="DoesNotExist" Condition="false" />
  <Target Name="Build" />
</Project>

Command line

Expected behavior

Successful build

Actual behavior

Error similar to:

E:\tmp\test\Test.proj : error : C:\Program Files\dotnet\sdk\2.1.601\Sdks\DoesNotExist\Sdk not found. Check that a recent enough .NET Core SDK is installed and/or increase the version specified in global.json.

Environment data

msbuild /version output:

Microsoft (R) Build Engine version 16.0.450+ga8dc7f1d34 for .NET Framework
Copyright (C) Microsoft Corporation. All rights reserved.

16.0.450.56488

OS info: Win10

[dfederm]

Note that a workaround is to extract out a targets file that does nothing but use the Sdk, and then conditionally import that targets file.

It's slightly semantically different though, but could work in many cases.

IE:
Foo.proj

<Project>
  <Import Project="Sdk.targets" Condition="false" />
  <Target Name="Build" />
</Project>

Sdk.targets:

<Project>
  <Sdk Name="DoesNotExist" />
</Project>

[rainersigwald]

This feels "by design" to me. I wish we errored when the condition was detected, but I don't know if anyone has used this construct (and thus would be broken by the change from "silent weird behavior" to "invalid project").

[rainersigwald]

Sdk.targets:

<Project>
  <Sdk Name="DoesNotExist" />
</Project>

This doesn't throw an error? 😬

Don't do this: use an Sdk only in your entry-point project.

[dfederm]

This doesn't throw an error? 😬

It doesn't because it doesn't end up getting imported since the condition is false :)

Don't do this: use an Sdk only in your entry-point project.

It can be helpful to be in Directory.Build.targets if all your projects need to use a particular SDK, eg Microsoft.Build.CentralPackageVersions.

Another workaround as per our offline discussion is to do:

<Import Sdk="SomeSdk" Project="Sdk.props" Condition="$(SomeCondition)" />
<!-- Rest of the stuff -->
<Import Sdk="SomeSdk" Project="Sdk.targets" Condition="$(SomeCondition)" />

[jeffkl]

I think we could transfer the condition from the <Sdk /> element to the implicit import:

So

<Project>
  <Sdk Name="DoesNotExist" Condition="false" />
  <Target Name="Build" />
</Project>

Would become:

<Project>
  <Import Project="Sdk.props" Sdk="DoesNotExist" Condition="false" />
  <Target Name="Build" />
  <Import Project="Sdk.targets" Sdk="DoesNotExist" Condition="false" />
</Project>

We can't actually apply conditions to <Sdk /> elements because the implicit imports are added way before property evaluation. But transferring the conditions to the <Import /> elements would make this work. A single <Sdk /> element does become an import at the top and bottom so people would have to understand that the condition might change.

So this:

<Project>
  <Sdk Name="DoesNotExist" Condition="'$(Something)' == 'true'" />
  <PropertyGroup>
    <Something>True</Something>
  </PropertyGroup>
  <Target Name="Build" />
</Project>

Would get expanded to this:

<Project>
  <Import Project="Sdk.props" Sdk="DoesNotExist" Condition="'$(Something)' == 'true'" />
  <PropertyGroup>
    <Something>True</Something>
  </PropertyGroup>
  <Target Name="Build" />
  <Import Project="Sdk.targets" Sdk="DoesNotExist" Condition="'$(Something)' == 'true'" />
</Project>

And the Sdk.props would not be imported but the Sdk.targets would be. Just something to consider.

[rainersigwald]

That feels like a change that's not worth the risk to me:

• possiblity of torn imports that you point out (condition changes between props and targets)
• paying attention to the condition could break someone (Condition="false" was ignored before, would be consumed now).

I wish it could have been that way from the beginning, though.