Sdk: Project System unconditionally declares the AssemblyName and other properties.

[333fred]

_From @tannergooding on January 28, 2017 16:53_

Description

In MSBuild\Sdks\Microsoft.NET.Sdk\build\Microsoft.NET.Sdk.props there are several properties which are unconditionally defined:

  <!-- User-facing configuration-agnostic defaults -->
  <PropertyGroup>
    <OutputType>Library</OutputType>
    <FileAlignment>512</FileAlignment>
    <ErrorReport>prompt</ErrorReport>
    <AssemblyName>$(MSBuildProjectName)</AssemblyName>
    <RootNamespace>$(MSBuildProjectName)</RootNamespace>
    <Deterministic>true</Deterministic>
  </PropertyGroup>

  <!-- User-facing configuration-specific defaults -->

  <PropertyGroup Condition=" '$(Configuration)' == 'Debug' ">
    <DebugSymbols>true</DebugSymbols>
    <Optimize>false</Optimize>
  </PropertyGroup>
  <PropertyGroup Condition=" '$(Configuration)' == 'Release' ">
    <Optimize>true</Optimize>
  </PropertyGroup>
  <PropertyGroup Condition=" '$(Platform)' == 'x64' ">
    <PlatformTarget>x64</PlatformTarget>
  </PropertyGroup>
  <PropertyGroup Condition=" '$(Platform)' == 'x86' ">
    <PlatformTarget>x86</PlatformTarget>
  </PropertyGroup>

  <!-- Default settings for all projects built with this Sdk package -->
  <PropertyGroup>
    <DebugType>portable</DebugType>

These will override any values that the user may have defined before the parent Sdk.props file was imported.

Expected Behavior

For the properties which can be overridden by the user (such as AssemblyName) they should be conditioned to only set the default if it isn't already set.

For the properties which are not allowed to be overridden (possibly DebugType), at the very least, a warning should be given if it was already set to a different value.

_Copied from original issue: dotnet/roslyn-project-system#1360_

[srivatsn]

The general msbuild design is that props are where defaults are set unconditionally and targets are where defaults are set conditionally. This is because props are supposed to appear at the top of the project and the project file that follows can override whatever it wants.

[tannergooding]

Summary: I completely agree that the general design is that props are where values are set unconditionally and targets are where values are set conditionally. However, it is also the case that for the non-user props files, values are almost always conditionally set unless it would break the build by being a different value.

The default property values set by the Sdk.props should be just that defaults. That is they should only be set if a value does not already exist. Doing anything else means that they are not defaults and are instead enforced values.

@srivatsn, I am declaring props prior. For example, the following should work:

<Import Project="$(MSBuildThisFileDirectory)..\..\Imports\Common\RepoName.Common.props" />
<Import Project="Sdk.props" Sdk="Microsoft.NET.Sdk" />

If I declare some properties in RepoName.Common.props, they should not be overridden by Sdk.props.

It is not uncommon for a user to have something like the following:

<PropertyGroup>
  <SomeProperty>SomeValue</SomeProperty>
</PropertyGroup>
<Import Project="SomeImport.props" />

Where SomeImports.props has a conditional declaration such as:

<Choose>
  <When Condition="'$(SomeProperty)' == 'SomeValue'">
    <PropertyGroup>
      <!-- Property Declarations -->
    </PropertyGroup>
  </When>
  <Otherwise>
    <PropertyGroup>
      <!-- Property Declarations -->
    </PropertyGroup>
  </Otherwise>
</Choose>

Sdk.props even imports a file (Microsoft.Common.props) where the user may want to declare some properties beforehand. For example, the repository may want to set <ImportUserLocationsByWildcardBeforeMicrosoftCommonProps>false</ImportUserLocationsByWildcardBeforeMicrosoftCommonProps> as they want to completely ignore and user customizations which may impact the build (this is very similar to passing in -NoProfile -ExecutionPolicy Bypass when executing a powershell script as this frequently can cause issues, such as executing using the wrong tool).

Ensuring that BaseIntermediateOutputPath is set to required user value is also important, as this impacts the value of MSBuildProjectExtensionsPath

Declaring VisualStudioVersion may also be important, as it impacts the resulting value for VSToolsPath.

Users who need to consistently override these values for an entire repository, would generally do so by declaring something like a RepoName.Common.props file. They would then import this first and then the Sdk.props file, so as to ensure that any downstream properties end up getting set properly.

To ease infrastructure, one would generally declare other defaults in the same file (rather than creating two separate imports, one for before Sdk.props and one for after).

Further, Sdk.props even has some of its own conditionals that may require the user to specify their own properties first. These include Platform, Configuration, EnableDefaultItems, EnableDefaultCompileItems, EnableDefaultEmbeddedResourceItems, etc... For each of these, the props was specifically designed with the thought that the user may need/want to override that in some scenarios. By unconditionally setting some properties in these files, it increases the infrastructure for the end-user (first they need to know that import priority is important for the SDK when it was not previously and then they need to ensure that properties are explicitly split and setup for import before the SDK props and import after the SDK props).

[dsplaisted]

Reopening, as based on Tanner's comments it seems worth considering setting these conditionally.

[srivatsn]

Fair enough.

[jeffkl]

I can code this up if you're willing to accept the change.

[srivatsn]

Gladly but don't think it meets the bar for RTM. You can create a PR against our 'future' branch.

[tannergooding]

@jeffkl, in addition to the contents of Microsoft.NET.Sdk.props it looks there are some defaults in Microsoft.NET.Sdk.CSharp.props and Microsoft.NET.Sdk.VisualBasic.props that should be conditioned.

[jaredpar]

Wanted to give a customer perspective here. @tannergooding is absolutely right. in particular this statement:

Users who need to consistently override these values for an entire repository, would generally do so by declaring something like a RepoName.Common.props file

This is exactly what dotnet/roslyn does and the SDK 1.0 behavior of unconditionally setting these properties is a real blocker. It means global warning suppressions, constants, etc ... are all wiped out once we import Sdk.props. Moving our props file after the import of Sdk.props is a non-starter because there are other centralized values which must be set before importing.

Conditionally setting these properties is definitely the right approach here.

Now for the next question ... how can I get a build of the SDK with these fixes on my machine? Want to keep making progress on roslyn and need a solution that doesn't depend on a brand new instance of VS being on my (and every Roslyn developer and Jenkins) machine.

[dsplaisted]

For VS 15.0, the SDK is part of VS. The only way to use a different SDK would be to set the MSBuildSDKsPath environment variable to where an updated SDK was available. So you could install a version of the .NET CLI with the updated SDK, and set the environment variable to the corresponding path under the CLI, for example C:\Program Files\dotnet\sdk\2.0.0-alpha-005165\Sdks. I'm not sure whether requiring an environment variable like this is something you're willing to do for Roslyn, but you could use it to keep working on it locally until there is a version of VS available with the updated SDK.

For 15.3 we are planning to use the SDK that comes from the CLI for VS and full Framework MSBuild, and use the same logic to select the version as is used for the rest of the .NET CLI. So installing an updated CLI would update the version of the SDK that is used, and you could pin to a specific version using a global.json file. The SDK and VS side of that work is tracked in https://github.com/dotnet/cli/issues/6112.

[jaredpar]

@dsplaisted

The only way to use a different SDK would be to set the MSBuildSDKsPath environment variable to where an updated SDK was available.

Doesn't the SDK ship as a NuGet package? Why can't we install it and set MSBuildSDKsPath to the location of the NuGet package?

I'm not sure whether requiring an environment variable like this is something you're willing to do for Roslyn

Absolutely. Overriding MSBuild properties is how we bootstrap the Roslyn compiler. It's a very reasonable approach to patching / overriding MSBuild with builds.

[dsplaisted]

No, the SDK doesn't ship as a NuGet package. We moved away from that because there was too much VS integration in the SDK for a project to work well before the SDK was restored.

MSBuildSDKsPath needs to be set as an environment variable, not an MSBuild property. It's used by MSBuild to locate the SDKs before properties have been evaluated.

[jaredpar]

MSBuildSDKsPath needs to be set as an environment variable, not an MSBuild property.

It also doesn't seem to be absolutely necessary. After chatting with @nguerrera I just switched our <Import> to not have an Sdk attribute and use full paths instead. Seems to work just fine.

We moved away from that because there was too much VS integration in the SDK for a project to work well before the SDK was restored.

This seems fairly solvable. I agree it presents a couple challenges but was pretty easy to adapt to in our repo. Feels like a big price to pay to remove the ability to override the SDK without admin privs because some developers wouldn't want to work around this bootstrapping problem.

[jaredpar]

What release will this fix be a part of? I'm using 15.2 previews and I'm still seeing this behavior for NoWarn properties.

[nguerrera]

15.3

[jaredpar]

So does that mean Roslyn should give up on SDK until then? If so when we hit an error in 15.3 will we need to give up again until 15.6?