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

NuGet-based SDK resolver design spec #2803

Closed
5 of 7 tasks
jeffkl opened this issue Dec 13, 2017 · 34 comments
Closed
5 of 7 tasks

NuGet-based SDK resolver design spec #2803

jeffkl opened this issue Dec 13, 2017 · 34 comments
Assignees
Labels
Milestone

Comments

@jeffkl
Copy link
Contributor

jeffkl commented Dec 13, 2017

Overview

At the moment, there are two SDK resolvers, both of which assume that the SDKs are on disk. It would be very useful to have SDKs resolved as NuGet packages as well which would allow third party SDKs to be acquired.

Related issues:

User Experience

Users would have to opt into the new behavior by specifying a version of the SDK to use. The resolver would never automatically get the latest because this would introduce very non-deterministic builds.

Users could control the version on a per-project basis:

<Project Sdk="My.Custom.Sdk/2.3.4">
  ...
</Project>

In this example, a NuGet package named My.Custom.Sdk version 2.3.4 would be retrieved and imported.

It would scale much better if users could specify a version per SDK for the whole repository. They could do this in their global.json:

{
  "sdk" : {
    "version:" "1.0.0"
  },
  "msbuild-sdks": {
    "My.Custom.Sdk" : "2.3.4"
  }
}

The sdk object controls which version of the .NET CLI to use and the msbuild-sdks object controls which versions of the MSBuild SDKs to use. If the resolver detected that multiple versions of the same SDK are requested for the same build episode, a warning would be logged and the first version specified would be used.

MSB4528: warning: A version of the SDK 'My.Custom.Sdk' was already specified in 'D:\Foo\global.json'.  The version specified at 'C:\Foo\bar.csproj (1.7)' was ignored.

In Visual Studio, users would see projects in a Loading state while SDKs are resolved. Any errors that occur during acquisition would be reported in the Output window just like other package restore or build errors. Projects would fail to load if an SDK could not be resolved. Users could fix the issue and reload the project in Visual Studio.

image

Since the SDKs would be NuGet packages, the standard caching mechanisms and configurations would be in place. Users would control which feeds to use and other NuGet settings in their NuGet.config.

Implementation

  • 1. The current SdkResolvers infrastructure within MSBuild would need to be improved to support caching and parallelization. Currently, the same SDK is resolved multiple times per build episode which can slow down evaluations. Make SDK resolution a service #2847
  • 2. Add functionality so that resolvers can maintain state in between resolutions. This would allow resolvers to only do expensive operations once per build. Allow SDK resolvers to preserve state #2849
  • 3. Implement a new logging message to indicate to callers when an SDK is done resolving. This would allow UI bound threads to wait for the Project constructor to return and be notified when its ready. We already added a ProjectEvaluationFinished event.
  • 4. NuGet needs to implement an API for requesting a package to be downloaded. This work is tracked here: NuGet Package Download API NuGet/Home#5919
  • 5. We would make a new package type named Sdk or MSBuildSdk. This could help differentiate the packages so that users don't attempt to add a PackageReference to something like Microsoft.NET.Sdk. Create new package type for MSBuild project SDKs NuGet/Home#6484
  • 6. Develop an SDK resolver that uses the NuGet object model to download packages into the global package cache and return the path to them. If the package was already on disk, it would not connect to any remote resource. Enough information would be logged that a user could diagnose problems like package feeds being offline, packages not existing, etc. Implement a NuGet-based SDK resolver #2850
    a. The resolver would ship with MSBuild so that the functionality is always available since it would be a core piece of project evaluation. It could be a new assembly or be in Microsoft.Build.dll
  • 7. Document how to take advantage of the resolver and how to make SDK packages. This documentation should include the new msbuild-sdks section of global.json. Add documentation for using MSBuild project SDKs MicrosoftDocs/visualstudio-docs#487

If time permits, I'd like to add a command-line argument like /resolver which would allow users to specify custom resolvers to address #2278. Repository owners could control resolvers with Directory.Build.rsp. Resolvers would not be able to be specified in an import because resolvers load before projects are evaluated.

@jeffkl jeffkl added the needs-design Requires discussion with the dev team before attempting a fix. label Dec 13, 2017
@jeffkl jeffkl self-assigned this Dec 13, 2017
@jeffkl
Copy link
Contributor Author

jeffkl commented Dec 13, 2017

FYI @nguerrera

@jaredpar
Copy link
Member

. It would be very useful to have SDKs resolved as NuGet packages as well which would allow third party SDKs to be acquired.

Think this design should encompass bringing in the .NET Core SDK via a NuGet package. Today it must be installed SxS with MSBuild and that creates a lot of friction in our initial build experience. Literally everything about the build except the .NET Core SDK can be bootstrapped. This is true even though the .NET Core SDK has supported xcopy distributions.

@jeffkl jeffkl added this to the MSBuild 15.6 milestone Dec 13, 2017
@jp2masa
Copy link
Member

jp2masa commented Jan 7, 2018

I'm working on 2 project systems, and I'd like to be able to create MSBuild SDKs which can be consumed by the project systems on both Visual Studio and the dotnet CLI. Installing the SDKs to the SDKs folder is not really easy (as it requires admin permission, and for the dotnet CLI it's even harder, as it would have to be installed on all the dotnet SDK installations). As one of the project systems is an extension for managed project systems, I use PackageReference, but it causes some problems on MEF parts import.

I see that the milestone for this feature is MSBuild 15.6, is it planned for the 15.6 release?

@jeffkl
Copy link
Contributor Author

jeffkl commented Jan 8, 2018

Yes I'm working to get this out in the next release

jeffkl added a commit that referenced this issue Jan 8, 2018
A singleton instance handles all resolution. A service hosted in the main node handles build related evaluation on the main node and a service hosted in the out-of-proc node sends requests to the main node for processing.

A cache exists on the main node so that requests are only resolved once per build. The out-of-proc node also caches responses so that an SDK is only resolved once.

Non-build evaluations are not cached and could be slow since there is no way to know if two evaluations are related.

Address the first step in #2803

* Log a warning if different versions of the same SDK are specified
* Log a warning if the SDK version specified does not match what was resolved
* Improve Microsoft.Build.UnitTests.Preprocessor.Preprocessor_Tests.SdkImportsAreInPreprocessedOutput by using TestEnvironment to mock SDK resolution
* Add escape hatch to disable caching the result on the main node `MSBUILDDISABLESDKCACHE`
jeffkl added a commit that referenced this issue Jan 9, 2018
SDK resolvers can get and set the `State` property on the `SdkResolverContext` they receive.  This state is preserved for the same resolver during the same build submission.  When there is no build submission, the state is not preserved.  Resolvers will need to take account for these considerations.

This will allow the NuGet-based SDK resolver to only look for and parse `global.json` once per build.

Address item 2 in #2803
@jeffkl
Copy link
Contributor Author

jeffkl commented Jan 10, 2018

PR is out for a NuGet-based SDK resolver... #2850

jeffkl added a commit that referenced this issue Jan 10, 2018
An SDK resolver that activates only when a user has specified a version.  This resolver is ordered to come before the DotNetSdkResolver but if a user does not specify a version (the default), then the NuGet SDK resolver is skipped.

The NuGet SDK resolver can be disabled with an environment variable just in case its causing issues or a user wants to override its functionality.

NuGet assemblies are loaded at runtime from their installed location.  They either come come [VisualStudioInstallRoot\Common7\IDE\CommonExtensions\Microsoft\NuGet or next to MSBuild.exe.  There is also an environment variable that can be set to specify a folder to use instead.

I've also placed references to NuGet into private subclasses so that assemblies are not loaded unless they are needed.  This means that if you have no version specified and no `global.json` with versions, `Newtonsoft.Json` and `NuGet.*.dll` are not loaded.  You only pay JIT penalty if you're using the functionality and hopefully we'll make up that time later in the build through caching.

Addresses item 6 in #2803 
Related to #2278
@jeffkl jeffkl removed the needs-design Requires discussion with the dev team before attempting a fix. label Jan 24, 2018
@natemcmaster
Copy link
Contributor

A few questions.

  1. What is the expected location for the Sdk.props/targets files inside a NuGet package?
  2. What takes precedence when setting the version of the NuGet SDK package to download, project or global.json? If version is absent from the <Sdk> element/attribute in a project, will the version in global.json be applied?
  3. /target:Restore supports using MSBuild properties for restore settings, such as adding feeds via RestoreSources, changing the NuGet cache folder with RestorePackagesPath or the location of the NuGet.config file.. This is something we must use because Nuget.config does not support conditionals. (Example in aspnet/Kestrel: build/sources.props). Can these MSBuild properties be used to control how an SDK package is downloaded by the NuGet resolver?

@jeffkl
Copy link
Contributor Author

jeffkl commented Jan 25, 2018

@natemcmaster I've been working on documentation and some of the answers are in there: https://github.com/MicrosoftDocs/visualstudio-docs/blob/master/docs/msbuild/how-to-use-project-sdk.md

What is the expected location for the Sdk.props/targets files inside a NuGet package?

Sdk.props and Sdk.targets must be in a root folder in the package named Sdk

MyPackage
└───Sdk
        Sdk.props
        Sdk.targets

What takes precedence when setting the version of the NuGet SDK package to download, project or global.json? If version is absent from the element/attribute in a project, will the version in global.json be applied?

The order is:

  1. Version specified in a project's XML
  2. Version specified in global.json

If no version exists in the project, then the one in global.json applies. We recommend you only put versions in global.json just because its easier to maintain.

/target:Restore supports using MSBuild properties for restore settings, such as adding feeds via RestoreSources, changing the NuGet cache folder with RestorePackagesPath or the location of the NuGet.config file.. This is something we must use because Nuget.config does not support conditionals. (Example in aspnet/Kestrel: build/sources.props). Can these MSBuild properties be used to control how an SDK package is downloaded by the NuGet resolver?

At the moment, no properties are used for the SDK package restores. When resolving an SDK, project properties haven't been read yet. Global properties could be used but only generally come into play with command-line builds. So for now you can only configure SDK restore with a NuGet.config. We'll have to get feedback if that's not going to work and then come up with a solution if needed.

@nguerrera
Copy link
Contributor

Sdk.props and Sdk.targets must be in a root folder in the package named Sdk

nuget package folders are all camelCased. We shouldn't break that convention

@jeffkl
Copy link
Contributor Author

jeffkl commented Jan 25, 2018

@nguerrera the folder name comes from what the SDK resolvers use:

https://github.com/dotnet/cli/blob/master/src/Microsoft.DotNet.MSBuildSdkResolver/MSBuildSdkResolver.cs#L96
https://github.com/Microsoft/msbuild/blob/master/src/NuGetSdkResolver/NuGetSdkResolver.cs#L207
https://github.com/Microsoft/msbuild/blob/master/src/Build/BackEnd/Components/SdkResolution/DefaultSdkResolver.cs#L32

Interestingly, the Sdk folder is the only one that's proper cased:

C:\Program Files\dotnet\sdk\15.5.0-preview-007044\Sdks\Microsoft.NET.Sdk
├───build
├───buildCrossTargeting
├───Sdk
└───tools

So the ship might have already sailed. The NuGetSdkResolver could use lower case names but I'm not sure if we can get that change in? What do you think @AndyGerlicher ? Maybe we should even use a different name in the NuGet packages like just build or msbuildsdk?

@nguerrera
Copy link
Contributor

Yeah, I told everyone the folder should be 'sdk' when we first introduced these, but the feedback did not get applied. :(

build already exists as a nuget concept for PackageName.targets/PackageName.props, but I think build/Sdk.props build/Sdk.targets would work.

@nguerrera
Copy link
Contributor

Maybe we just need to check both on case-sensitive file systems and document 'sdk' as the good way?

@natemcmaster
Copy link
Contributor

@jeffkl thanks for clarifying.

I totally get why, but it's too bad we can't use MSBuild settings to influence the restore inside the NuGet SDK resolver. NuGet.config isn't quite expressive enough, and MSBuild project evaluation has to come after SDKs are resolved. Looks like we'll have to go back to using a console tool to manipulate NuGet.config files before invoking MSBuild.

@jeffkl
Copy link
Contributor Author

jeffkl commented Jan 25, 2018

@natemcmaster can you open an issue so I can track the request and investigate a solution?

@natemcmaster
Copy link
Contributor

Ok. #2914

@clairernovotny
Copy link
Member

What is the ordering works w.r.t. multiple SDK's. First one goes outer (first props/last targets?)

One big issue I have with MSBuild.Sdk.Extras is that I want my props to be after the .NET SDK props and my targets before the .NET SDK targets.

I wonder if this would do that: <Project Sdk="Microsoft.NET.Sdk;MSBuild.Sdk.Extras/1.2.1">

Second, it would be really useful if NuGet could add/update these from both the project location and the global.json location. How are people supposed to know these packages exist (and get installed in the "right" way?) I imagine that people would use the NuGet UI or dotnet add gestures just like they do now. Except instead of a PackageReference, it'll add to the Sdk= attribute. Also for updates, how will the user know there's a newer version? Having it display and be updatable in the NuGet UI would be helpful.

Should also show version's/updates for global.json entries since that is the recommended place to put these.

@natemcmaster
Copy link
Contributor

it would be really useful if NuGet could add/update these from both the project location and the global.json location. How are people supposed to know these packages exist (and get installed in the "right" way?)

@onovotny IMO using packageType metadata is a good first step. If we agree to do NuGet/Home#6484, NuGet could use this to add tooling experiences for these types of packages (and by tooling I mean VS, nuget.org search, command-line experiences, etc). I don't think any of those experiences are planned yet, though.

@kzu
Copy link
Contributor

kzu commented Jan 31, 2018

so is this shipping already in current 15.6 previews?

@jeffkl
Copy link
Contributor Author

jeffkl commented Jan 31, 2018

@kzu yes it went out in 15.6 Preview 3 and we have some minor improvements coming to it in Preview 4. I've started working on SDKs: https://github.com/Microsoft/MSBuildSdks

@clairernovotny
Copy link
Member

@jeffkl What's the right compat story to support both usages? Just add an Sdks directory and the sdk props/targets?

I'd like to migrate my MSBuild.Sdk.Extras to it, but I can't break existing users using as a PackageReference.

@jeffkl
Copy link
Contributor Author

jeffkl commented Jan 31, 2018

@onovotny yes at the moment you can just add add an Sdk folder with Sdk.props and Sdk.targets which just imports ..\build\MSBuild.Sdk.Extras.props and ..\build\MSBuild.Sdk.Extras.targets (unless you need do so something more fancy).

SDK packages don't honor dependencies yet and MSBuild considers them to be pretty much standalone when it adds the implicit imports. We don't currently support an SDK depending on an SDK and having multiple imports in dependency order. But I might work on that if the need arises.

@dasMulli
Copy link
Contributor

dasMulli commented Jan 31, 2018

What happens if a sdk project file also has an sdk import? #inception

@clairernovotny
Copy link
Member

How about ordering? Like if mine needs to be after the .net sdk ones, so that I get their props and my targets run before the .net sdk targets?

This is partially a tooling issue; if/when NuGet installs it, how does it know to prepend/append. I need to be able to specify the .net sdk goes before mine.

@dasMulli
Copy link
Contributor

if that works (sdk import in sdk), @onovotny you could in theory make the extras package the single sdk reference and then add an <Import Project="Sdk.targets" Sdk="Microsoft.NET.Sdk" /> in the middle of your targets just where you need it..

@jeffkl
Copy link
Contributor Author

jeffkl commented Jan 31, 2018

@onovotny I think this would work best:

<Project Sdk="Microsoft.NET.Sdk">
  <Sdk Name="MSBuild.Extras.Sdk" Version="1.0.0" />
  <PropertyGroup>
      <TargetFramework>net46</TargetFramework>
  </PropertyGroup>
</Project>

In that case, the implicit imports should be like:

<Project Sdk="Microsoft.NET.Sdk">
  <Import Project="Sdk.props" Sdk="Microsoft.NET.Sdk" />
  <Import Project="Sdk.props" Sdk="MSBuild.Extras.Sdk" Version="1.0.0" />
  <PropertyGroup>
      <TargetFramework>net46</TargetFramework>
  </PropertyGroup>
  <Import Project="Sdk.targets" Sdk="MSBuild.Extras.Sdk" Version="1.0.0" />
  <Import Project="Sdk.targets" Sdk="Microsoft.NET.Sdk" />
</Project>

If you need to be imported after Microsoft.NET.Sdk, you probably just want to stick with <PackageReference />. SDK packages are really meant to be the top-level set of imports or if you need to modify how Restore works (which can't be done via a PackageReference).

@clairernovotny
Copy link
Member

clairernovotny commented Jan 31, 2018

@jeffkl I don't want to be a PackageReference, as I do want to control how restore works (and include additional metapackages, like UWP).

Also, PackageReference does not have the right extensibility points for the build imports, it pulls me in too late for the targets. I have to tell people to add this to the end of their project file to get in earlier:

<Import Project="$(MSBuildSDKExtrasTargets)" Condition="Exists('$(MSBuildSDKExtrasTargets)')" />

Does it have to be <Sdk Name="MSBuild.Extras.Sdk" Version="1.0.0" /> as a separate element for this? Why can't I say <Project Sdk="Microsoft.NET.Sdk;MSBuild.Extras.Sdk"> where the global.json defines the version? That seems much cleaner, I just need ordering guarantees, and it'd be nice if the NuGet tooling could know that's what should happen.

@clairernovotny
Copy link
Member

Better yet would be <Project Sdk="MSBuild.Extras.Sdk"> where my SDK can do the right thing and declare that Microsoft.NET.Sdk has to be "first".

@rainersigwald
Copy link
Member

As long as what you're referencing is not another NuGet-delivered SDK, @dasMulli's suggestion #2803 (comment) should enable <Project Sdk="MSBuild.Extras.Sdk">.

@clairernovotny
Copy link
Member

clairernovotny commented Jan 31, 2018

@rainersigwald Neat, that looks like it could work then, assuming that Microsoft.NET.Sdk will always be in-box.

What happens if I do that but the user still has <Project Sdk="Microsoft.NET.Sdk;MSBuild.Extras.Sdk">? What wins the import battle? Are there warnings of multiple imports? Just thinking though cases where people do the wrong thing.

@jeffkl
Copy link
Contributor Author

jeffkl commented Jan 31, 2018

@onovotny you do not have to declare a version in the project, global.json is definitely the better way to go. I just keep mentioning it to remind people that if no version is specified in the project or global.json, the NuGet SDK resolver won't do anything.

Your suggestion of doing both SDKs as a list is probably what you want. I tend to use the <Sdk /> element if I want to reference and SDK in an import.

So this:

<Project Sdk="MSBuild.Extras.Sdk;Microsoft.NET.Sdk">
  <PropertyGroup>
      <TargetFramework>net46</TargetFramework>
  </PropertyGroup>
</Project>

Should expand to this:

<Project>
  <Import Project="Sdk.props" Sdk="MSBuild.Extras.Sdk" />
  <Import Project="Sdk.props" Sdk="Microsoft.NET.Sdk" />
  <PropertyGroup>
      <TargetFramework>net46</TargetFramework>
  </PropertyGroup>
  <Import Project="Sdk.targets" Sdk="Microsoft.NET.Sdk" />
  <Import Project="Sdk.targets" Sdk="MSBuild.Extras.Sdk" />
</Project>

You will be able to confirm the ordering in the preprocessor in 15.6 preview 4 (I had to fix a bug)

@clairernovotny
Copy link
Member

clairernovotny commented Jan 31, 2018

I just keep mentioning it to remind people that if no version is specified in the project or global.json, the NuGet SDK resolver won't do anything.

Is an error raised then, since no matching SDK would be found? A silent error seems bad...?

@jeffkl
Copy link
Contributor Author

jeffkl commented Jan 31, 2018

You get an error that the SDK could not be found. The NuGet SDK resolver notices that no version is specified so MSBuild continues on in the list of resolvers:

  1. NuGet SDK resolver finds nothing because no version is specified
  2. .NET CLI resolver finds nothing because the SDK is not installed with .NET CLI
  3. MSBuild default resolver finds nothing because the SDK is not installed with MSBuild

@clairernovotny
Copy link
Member

So what that leaves is what the best practice/recommendation should be?

I would need <Project Sdk="Microsoft.NET.Sdk;MSBuild.Extras.Sdk"> as the ordering to work properly.

Do I rely on that, or as @dasMulli suggested, put the <Import Sdk="Microsoft.NET.Sdk" ... in my Sdk props/targets? That would enable <Project Sdk="MSBuild.Extras.Sdk">, but I question what happens if people do <Project Sdk="Microsoft.NET.Sdk;MSBuild.Extras.Sdk"> anyway in that case (double import warning/error?)

Finally, it's about the tooling doing the right thing with the package on install (correct order or replacement of the existing sdk item).

@jeffkl
Copy link
Contributor Author

jeffkl commented Jan 31, 2018

In my opinion, having MSBuild.Extras.Sdk include an SDK reference to Microsoft.NET.Sdk would be a bad choice. It limits what the project owner can do and having them specify <Project Sdk="MSBuild.Extras.Sdk;Microsoft.NET.Sdk" /> doesn't seem like too much overhead. That said, if MSBuild.Extras.Sdk is designed to only extend Microsoft.NET.Sdk and you really want it to be a replacement, then having it include an SDK reference might make sense.

I imagine you'll want to play around with it a little bit and decide what gives your users the best experience when consuming MSBuild.Extras.Sdk. And of course please circle back and let me know if you have any feedback. You should install 15.6 Preview 3 and try it out if you haven't already.

@jeffkl
Copy link
Contributor Author

jeffkl commented Feb 1, 2018

Closing this since we've completed all work on our end and remaining items have associated in other repos.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

10 participants