<Sdk> element does not respect Conditions

[dfederm]

Steps to reproduce

Project file

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

Command line

msbuild

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.

[AraHaan]

I find it a pain that it's considered a risk to break someone. I think in this case it should be ok to break someone as a vast majority of users would probably expect the Condition part of the element to be expanded to the <Import Project=... Sdk=... Condition=... />.

Why is it you ask? Because let's use Microsoft.NET.Sdk and Microsoft.NET.Sdk.Web for example, while Sdk.props defines a property when it's imported, Sdk.targets in those sdk's do not so there is the possibility that the Sdk.targets for those sdks can be imported multiple times within one's custom msbuild sdk that imports one (or both) of those sdks conditionally. That is why one would want to use the <Sdk> element inside of their custom .NET SDK's only to find that they now run into this issue (which I feel should be fixed).

[KalleOlaviNiemitalo]

MSBuild has change waves now, so the warning about Condition not being supported in Sdk could be made conditional on that.