Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implicit top & bottom SDK imports #1392

Closed
cdmihai opened this issue Nov 23, 2016 · 18 comments
Closed

Implicit top & bottom SDK imports #1392

cdmihai opened this issue Nov 23, 2016 · 18 comments
Assignees
Labels
needs-design Requires discussion with the dev team before attempting a fix. triaged
Milestone

Comments

@cdmihai
Copy link
Contributor

cdmihai commented Nov 23, 2016

SDK imports represent the extension mechanism for MSBuild's build framework model: a certain build framework (e.g. UWP, .net core, web, etc) specifies what a build means by using an entry-point .props file and an entry-point .targets file.

A particular project implements a specific framework by putting its logic between and around the framework's entry-point imports. Its logic usually particularizes framework values, like the project name, references, etc. A project can implement multiple build frameworks by inserting their specific entry-point SDK imports (e.g. an F# based .net core web project).

With the move to .net core's human editable csproj files, the legacy top and bottom imports are no longer desirable, since they feel like magic values that are hard to remember and type from scratch:

<Project>
  <Import Project="$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props" Condition="Exists('$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props')" />
...
  <Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />

</Project>

Goals

  1. Remove SDK PackageRef
  2. Remove entry-point SDK imports
  3. Remove nuget restore from sdk acquisition

Impact on Templates

With the change in place, the .NET Core Library csproj template would become:

<Project ToolsVersion="15.0" Sdk="Microsoft.NET.Sdk/1.0.0-rc">
  <PropertyGroup>
    <TargetFramework>netstandard1.4</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <Compile Include="**\*.cs" />
    <EmbeddedResource Include="**\*.resx" />
  </ItemGroup>

  <ItemGroup>
    <PackageReference Include="NETStandard.Library" Version="1.6" />
  </ItemGroup>
</Project>

Risks

Implicit <Imports> should not:

  • get persisted to disk
  • copied during cloning (except if they have an internal flag to mark them as syntactic sugar)
  • Potential cache misses in ProjectCollection and ProjectRootElement caches
  • Potential weird interaction with ProjectRootElement.Version

Design

A new attribute is added to <Project> for representing SDKs: Sdk="Microsoft.NET.Sdk/1.0.0-RC".
In future releases the element is used to assist in the package acquisition experience. In the next release it is used to identify a pre-installed framework to be loaded. Specifically, when MSBuild discovers one of these elements it will inject implicit Imports before and after the current .proj file.

This example .csproj file:

<Project Sdk="Microsoft.NET.Sdk/1.0.0-RC" />
  <Target Name="SayHi">
    <Message Text="Hello, World" />
  </Target>
</Project>

Is interepreted by MSBuild as:

<Project Sdk="Microsoft.NET.Sdk/1.0.0-RC" />
  <!-- Import all, in order listed -->
  <Import Project="$(MSBuildSDKsPath)\%(___MSBuildSDK.Name)\%(___MSBuildSDK.Version)\build\InitialImport.props" 
          Condition="Exists('$(MSBuildSDKsPath)\%(___MSBuildSDK.Name)\%(___MSBuildSDK.Version)\build\InitialImport.props')" />
  
  <Target Name="SayHi">
    <Message Text="Hello, World" />
  </Target>
  
  <!-- Import all, in order listed -->  
  <Import Project="$(MSBuildSDKsPath)\%(___MSBuildSDK.Name)\%(___MSBuildSDK.Version)\build\FinalImport.targets"
          Condition="Exists('$(MSBuildSDKsPath)\%(___MSBuildSDK.Name)\%(___MSBuildSDK.Version)\build\FinalImport.targets')" />
</Project>

If users want to override elements (properties, items, targets, etc) in the implicit imports, they would have to use the explicit form of SDK imports.

The implicit SDK imports

  <Project Sdk="Microsoft.FSharp.Web/1.0.0.0" />  <!-- allows multiple SDK -->
  </Project>

are made explicit via the equivalent:

<Project>
  <user prop here>
  <Import Project="Sdk.props" Sdk="Microsoft.NET.Sdk/1.0.0.0" />
...
  <Import Project="Sdk.targets" Sdk="Microsoft.NET.Sdk/1.0.0.0" />
  <user target here>
</Project>

Usage Examples

1. .NET Core Console App

<Project Sdk="Microsoft.NET.Sdk/1.0.0-RC">
  ...
</Project>

2. .NET Core Web App

<Project Sdk="Microsoft.NET.Sdk/1.0.0-RC;
              Microsoft.Web.Sdk/1.0.0-RC">
  ...
</Project>

Potential MSBuildSDKsPath Locations for next release

  1. In Visual Studio: $(MSBuildExtensionsPath)\.dotnet\
  2. In CLI, [ProgramFiles]\dotnet\sdk\{cli_version}\Extensions\

In future releases we would use a package manager to tell use where to get them from based on their name & version

@rainersigwald
Copy link
Member

@rainersigwald
Copy link
Member

Splitting out the <Import Sdk= part from the implicit part, because for the next prerelease we can fake the final behavior without the SDK-import syntax.

@Sarabeth-Jaffe-Microsoft Sarabeth-Jaffe-Microsoft added this to the Visual Studio 15 RTM milestone Nov 28, 2016
@dsplaisted
Copy link
Member

We had a discussion about the version numbers in the Sdk attribute, and decided not to include them in the templates for the next preview. So the template for a .NET Core Console app would look like this:

<Project Sdk="Microsoft.NET.Sdk">
  ...
</Project>

Our plan is to later add support for a "lineup" which can specify which versions of Sdks to use. (This lets it be specified for all projects in a build instead of each project potentially specifying different versions.) Originally our thinking was that the lineup file would be used if you wanted to specify the exact version of the Sdks to use, while the version in the project file would be a minimum version and only be used to generate an error if the version of an Sdk that was selected was too low. Now we are thinking that the the lineup file could specify either an exact version or a minimum version.

Current POR is to still allow the version number to be specified in the Sdk attribute, but to ignore it.

Separately, we don't want the project file to have to list both the Web and .NET SDK, especially if the ordering between them matters. In the next preview, our plan is to have the Web SDK import Sdk.props and Sdk.targets from the .NET SDK using a relative path. Thus, the .NET Core Web App template becomes:

<Project Sdk="Microsoft.Web.Sdk">
  ...
</Project>

After the next preview we should consider how we can support dependencies between SDKs more generally.

@davidfowl
Copy link
Member

We had a discussion about the version numbers in the Sdk attribute, and decided not to include them in the templates for the next preview.

That's pretty concerning. Why not start with a version number and find then when the lineup concept lands remove it...

How do these things compose? What does that look like? Does the transitive closure of dependencies need to be splat into the project or can the SDK declare them? Also related, how do we go about composing language specific SDKs:

Microsoft.Web.Sdk -> Microsoft.Net.Sdk

<Project Sdk="Microsoft.Web.Sdk;Microsoft.Net.Sdk">
  ...
</Project>

F#?

<Project Sdk="Microsoft.Web.Sdk;Microsoft.Net.FSharp.Sdk;Microsoft.Net.Sdk">
  ...
</Project>

I'm ignoring the lineup and versioning for now (but I'm also concerned about that).

@davidfowl
Copy link
Member

I'm generally worried about having to reinvent semantics where they already work and exist today. E.g.

  • dependency conflict resolution
  • dependency version unification (if any)
  • asset resolution
  • (insert thing nuget solves here)...

I also think that "SDK" is just another word for "development dependency" or most recently "build time" dependencies with the added feature that it can be "pre-installed". I think we're glossing over acquisition but we'll immediately hit it as soon as we need to ship something outside of the set of pre-installed things that come with the .NET SDK.

  • SDKs depending on other SDKs
  • SDKs that want to bring in packages
  • packages that would like to bring in SDKs

BTW I'm using SDK to mean, props and targets (maybe tasks too), because that's all it really is. This still doesn't solve the fact that props and targets coming from nuget packages via PackageReference end up in the same dependency graph as the runtime graph (amongst other problems)...

@AndyGerlicher AndyGerlicher modified the milestones: MSBuild 15.1 RC.3, Visual Studio 15 RTM Dec 2, 2016
@danmoseley
Copy link
Member

danmoseley commented Dec 3, 2016

Seems the /preprocess output would be unaffected, except you have to decide what it shows in the comments where it currently shows the resolved import tags. What the user typed, what MSBuild inferred, or both?
edit: I just saw #1428

@rainersigwald
Copy link
Member

I tried to be as explicit as possible in the preprocessed output: 350e21b. Suggestions welcome!

@rainersigwald rainersigwald modified the milestones: MSBuild 15.1 RC.3, MSBuild Sdks Dec 4, 2016
@danmoseley
Copy link
Member

@rainersigwald looks good!

@enricosada
Copy link

I'd like if possibile to have defined some strict and documented points of extensibility (CoreCompile, contents, publish) instead of just trying to guess the implementation of another sdk. because that's may change over time.

Another concert for me is because the sdk list will grow really fast, the .csproj was cleaned up just to have lot's of lines at start? on multiple lines because that's required to make id readable.
Like @davidfowl said, the sdk are just build time dependencies. Is really not possibile to put these in an ItemGroup? at least can be put on bottom. Just read it as normal xml, to find a specific ItemGroup element, not used by msbuild itself.

About languages a im less worried, because we can just define the target for another language (f# for example) BEFORE the microsoft.net.sdk, and prepopulate a property (works already, ref dotnet/cli#4922 ) to hook the language target (who defines CoreCompile)

@rainersigwald
Copy link
Member

Sdks definitely cannot be represented in an ItemGroup, because items are considered well after imports. We could introduce a new top-level XML element to be considered before properties and imports. @jeffkl and I preferred this approach, but many other folks didn't like it.

I think the main reasons to prefer the attribute were:

  • It looks cleaner for simple cases.
  • It's intuitive that property expansions (for example for versions) are not allowed (which is required because we have to consider this before properties are defined).

@enricosada
Copy link

enricosada commented Dec 5, 2016

@rainersigwald i understandard are considered after, but cannot the csproj just be read as normal xml? not with an msbuild evalutation.
Doing it clean for simple cases, it's going to break soon, mytool.package.sdk/1.0.0-beta-version is a big string, plus there are multiple sdk (and more to come hopefully), so is going to become split on multiple lines, just at start of project file (really noisy).

<Project Sdk="Microsoft.NET.Sdk/1.0.0-RC;
              FSharp.NET.Sdk/1.0.0-RC;
              Paket.Sdk/1.0.0-RC;
              UWP.Sdk/1.0.0-alpha4;
              LocalizeMyResources.Sdk/1.0.0-beta;
              My.CI.Integraton.With.Jeknins.Sdk/1.0.0-RC">
  ...
</Project>

I think property expansion is not an issue, because $ is not valid for names and version, so can be a normal error reading as normal xml file.

The goal is also have a readable project file. Important parts commonly read and changed are compiles (for f# at least), defines, resouces, nuget package attributes, package references.. not the sdks

@dsplaisted
Copy link
Member

dsplaisted commented Dec 5, 2016

@enricosada I don't think in common scenarios there will be more than one or two Sdks referenced. NuGet packages are still a valid way of providing props and targets files that extend the build. The benefit of Sdks is that they help get rid of the imports that previously had to go at the top and bottom of your project file, and that the Sdks imports are available before package restore has completed (which lets us avoid situations where you create a project in Visual Studio and see a bunch of error messages until the restore operation completes).

From your example, localization and CI integration sound like they should probably be NuGet packages. I would expect that the FSharp and UWP Sdks would depend on the base .NET one, so you wouldn't need to list Microsoft.NET.Sdk either. And finally, we are still considering whether the version number should be expressed in each project file or whether it should be left out and defined elsewhere globally for your build. So your project is more likely to look something like this:

<Project Sdk="FSharp.NET.Sdk;
              Paket.Sdk;
              UWP.Sdk">
  ...
  <ItemGroup>
    <PackageReference Include="LocalizeMyResources" Version="1.0.0-beta" />
    <PackageReference Include="My.CI.Integration.With.Jenkins" Version="1.0.0-RC" />
  </ItemGroup>
</Project>

We do need to figure out when we recommend using Sdks versus NuGet packages, and write up guidance about it (#1439).

I would also like to consider using simpler names for the Sdks, such as described in dotnet/sdk#436. This would enable the Sdk references to look something like this:

<Project Sdk="F#;Paket;UWP">
  ...
</Project>

EDIT: I said that you shouldn't need to refer to the .NET Sdk if you used FSharp or UWP, but then I left it in the samples I gave. I've taken it out now.

@enricosada
Copy link

@dsplaisted 👍 about aliases + per repo (i hope per repo, not global) sdk versions. That's enough to fix my issues. Thank for the answer!

@gulshan
Copy link

gulshan commented Dec 7, 2016

My two cents-

  • Build time dependencies SDKs or tools can use same nuget mechanism, but use a separate myget-like repository maintained by MSBuild.
  • The difference between SDKs and tools need not to be explicit.
  • All build time dependencies should be installed globally.
  • Dotnet cli tools can merge with MSBuild tools in future, given similar API support in MSBuild.

We should take a look at other successful ecosystems and build frameworks for ideas and inspiration. In particular, I have some reservation for gradle- #1289

@Mike-E-angelo
Copy link

Holy moses I was about lay the smackdown on whoever tagged this issue as "needs design" but then I realize that it was the author. 😆 What an incredible issue/post, @cdmihai! Well done. It should be used as a template going forward for all new suggestions, if it isn't already. 👍

@enricosada
Copy link

All build time dependencies should be installed globally

Let's not repeat msbuild errors. Build time deps are per repository. Noone remember the pain of install msbuild deps on ci server or machine?

Also sdk vs normal package doesnt need a special nuget server, packages can be filtered on search with attributes

@rainersigwald
Copy link
Member

As of #1400 / #1492, the <Project Sdk=""> experience is as described in this issue.

@Mike-E-angelo
Copy link

Really great work, everyone. Very impressed with the improvements and how those files are looking these days. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-design Requires discussion with the dev team before attempting a fix. triaged
Projects
None yet
Development

No branches or pull requests