Add support for SDK Resolvers #2002

AndyGerlicher · 2017-04-22T08:05:58Z

Sending the PR for some initial feedback. I haven't reviewed it as carefully as I'd like (I'm sure there are nit items), and things I know I still have left to do:

Test on CoreCLR (haven't tried it yet).
Further testing within Visual Studio.
Load resolver(s) in a unit test (didn't have a good way to do this, may have to come later).

rainersigwald

Partial feedback on tests + interfaces only = "design feedback". Continuing to review for implementation.

rainersigwald · 2017-04-24T14:14:06Z

ref/net46/Microsoft.Build.Framework/Microsoft.Build.Framework.cs

@@ -360,6 +360,17 @@ public partial class ProjectStartedEventArgs : Microsoft.Build.Framework.BuildSt
        public string ToolsVersion { get { throw null; } }
    }
    public delegate void ProjectStartedEventHandler(object sender, Microsoft.Build.Framework.ProjectStartedEventArgs e);
+    public sealed partial class ReferencedSdk : System.IEquatable<Microsoft.Build.Framework.ReferencedSdk>


I think this should be named SdkReference to read more clearly.

rainersigwald · 2017-04-24T14:15:28Z

ref/net46/Microsoft.Build/Microsoft.Build.cs

@@ -334,6 +335,13 @@ public partial class ProjectRootElement : Microsoft.Build.Construction.ProjectEl
        public static Microsoft.Build.Construction.ProjectRootElement TryOpen(string path, Microsoft.Build.Evaluation.ProjectCollection projectCollection) { throw null; }
        public static Microsoft.Build.Construction.ProjectRootElement TryOpen(string path, Microsoft.Build.Evaluation.ProjectCollection projectCollection, System.Nullable<bool> preserveFormatting) { throw null; }
    }
+    public partial class ProjectSdkElement : Microsoft.Build.Construction.ProjectElementContainer


Did we get conceptual buy off on this from the people that vetoed it before?

rainersigwald · 2017-04-24T14:17:05Z

src/Build.OM.UnitTests/Construction/ProjectSdkImplicitImport_Tests.cs

-                Assert.Equal(6, children.Count);
+
+                // <Sdk> style will have an extra ProjectElment.
+                var expected = projectFormatString.Contains("Sdk=") ? 6 : 7;


Make this an input instead? (Can wait for a cleanup PR)

rainersigwald · 2017-04-24T14:20:24Z

src/Build.UnitTests/BackEnd/MockLoggingService.cs

@@ -24,6 +24,13 @@ namespace Microsoft.Build.UnitTests.BackEnd
    /// </summary>
    internal class MockLoggingService : ILoggingService
    {
+        private Action<string> _writter;


Typo: writer

rainersigwald · 2017-04-24T14:23:01Z

src/Build.UnitTests/BackEnd/MockLoggingService.cs

@@ -24,6 +24,13 @@ namespace Microsoft.Build.UnitTests.BackEnd
    /// </summary>
    internal class MockLoggingService : ILoggingService
    {
+        private Action<string> _writter;
+
+        public MockLoggingService(Action<string> writter = null)


Not a fan of this approach. I'll send a commit to use xunit output capture.

This can wait.

rainersigwald · 2017-04-24T14:27:27Z

src/Build.UnitTests/BackEnd/SdkResolution_Tests.cs

+        {
+            // 2sdkName will cause MockSdkResolver1 to fail with an error reason. The error will not 
+            // be logged because MockSdkResolver2 will succeed.
+            var log = new StringBuilder();


Comment copypasta

rainersigwald · 2017-04-24T14:40:48Z

src/Build.UnitTests/BackEnd/SdkResolution_Tests.cs

+
+            // Resolver2 gives a warning on success or failure.
+            Assert.Contains("WARNING2", logResult);
+            Assert.DoesNotContain("ERROR", logResult);


Can wait, but: I don't think this is the right logging strategy. We should look at how NuGet handles multiple package sources and learn from their experience/mistakes. I think we should log every message from all resolvers but demote ERROR to INFO--otherwise, how will we diagnose the situation "the first resolver was supposed to resolve this but didn't"?

rainersigwald · 2017-04-24T14:43:38Z

src/Build.UnitTests/BackEnd/SdkResolverLoadingStrategy_Tests.cs

+                File.WriteAllText(f3, string.Empty);
+                File.WriteAllText(f4, string.Empty);
+
+                var strategy = new SdkResolverLoadingStrategy();


I'd probably name this thing SdkResolverLoader.

rainersigwald · 2017-04-24T14:43:56Z

src/Build.UnitTests/BackEnd/SdkResolverLoadingStrategy_Tests.cs

+                File.WriteAllText(f4, string.Empty);
+
+                var strategy = new SdkResolverLoadingStrategy();
+                var files = strategy.FindPotentialSdkResolvers(root);


FindResolvers()

Ah, never mind. This is an internal implementation detail and is appropriately named.

rainersigwald

Seems reasonable overall.

rainersigwald · 2017-04-24T14:45:48Z

src/Build/BackEnd/Components/SdkResolution/DefaultSdkResolver.cs

+{
+    /// <summary>
+    ///     Default SDK resolver for compatibility with VS2017 RTM.
+    /// </summary>


Add a <remarks> describing what that means (searching in env var, next to MSBuild)?

rainersigwald · 2017-04-24T14:46:39Z

ref/net46/Microsoft.Build.Framework/Microsoft.Build.Framework.cs

+    {
+        protected SdkResolver() { }
+        public abstract string Name { get; }
+        public abstract int Priority { get; }


I really hope we can come up with a better implementation here but we can always ignore this field in the future so it should be reasonably harmless.

rainersigwald · 2017-04-24T14:50:11Z

src/Build/BackEnd/Components/SdkResolution/SdkResolution.cs

+{
+    /// <summary>
+    ///     Component responsible for resolving an SDK to a file path. Loads and coordinates
+    ///     with <see cref="SdkResolver" /> plug-ins.


Oh, I get the name now. But I still don't like it. SdkResolvers?

rainersigwald · 2017-04-24T14:59:21Z

src/Build/BackEnd/Components/SdkResolution/SdkResolution.cs

+                    var resultFactory = new SdkResultFactoryImpl(sdk);
+                    try
+                    {
+                        var result = (SdkResultImpl)sdkResolver.Resolve(sdk, context, resultFactory);


Casting to ...Impl seems pretty wrong here. Why is it necessary?

Same. All usages of result in this method should be of the supertype members. The Resolver could even choose not to use the resultFactory and use its own result implementation :)

In reply to: 112969534 [](ancestors = 112969534)

Still concerned about this. Does it represent a design flaw, or is it just an implementation fix we can deal with later?

rainersigwald · 2017-04-24T15:00:28Z

src/Build/BackEnd/Components/SdkResolution/SdkResolution.cs

+                    }
+                    catch (Exception e)
+                    {
+                        logger.LogFatalBuildError(buildEventContext, e, new BuildEventFileInfo(importElementLocation));


This catch looks redundant with the overall one, except it doesn't rethrow? Just to avoid onion-peeling on resolver exceptions?

rainersigwald · 2017-04-24T15:22:28Z

src/Build/Evaluation/Evaluator.cs


+            // Evaluate the "top" implicit imports as if they were the first entry in the file.


Ah, so that makes the order I complained about above less relevant. Cool.

rainersigwald · 2017-04-24T15:26:01Z

src/Build/Evaluation/Evaluator.cs

+                if (string.IsNullOrEmpty(sdkRootPath))
+                {
+                    ProjectErrorUtilities.ThrowInvalidProject(importElement.SdkLocation, "CouldNotResolveSdk", importElement.ParsedReferencedSdk.ToString());
+                }


I was a bit surprised to see this; I expected GetSdkPath to throw on failure.

rainersigwald · 2017-04-24T15:28:34Z

src/Framework/Sdk/ReferencedSdk.cs

+                return false;
+            if (ReferenceEquals(this, other))
+                return true;
+            return string.Equals(Name, other.Name) && Equals(Version, other.Version);


We decided to be case-sensitive for Sdk names? I think we did as a side effect of the initial implementation on case-sensitive file systems, but I don't remember for sure. Should be doc'ed either way.

rainersigwald · 2017-04-24T15:36:01Z

src/Framework/Sdk/ReferencedSdk.cs

+        {
+            unchecked
+            {
+                return ((Name?.GetHashCode() ?? 0) * 397) ^ (Version?.GetHashCode() ?? 0);


Every time I see this pattern I weep a bit. Where'd you get 397 from?

@jeffkl not sure. I think Resharper generated all of the IEquatable stuff.

rainersigwald · 2017-04-24T15:36:52Z

src/Framework/Sdk/SdkResolver.cs

+        /// <summary>
+        ///     Resolves the specified SDKs.
+        /// </summary>
+        /// <param name="referencedSdk">A <see cref="ReferencedSdk" /> containing the referenced SDKs be resolved.</param>


typo: plurality disagreement

cdmihai · 2017-04-24T16:46:23Z

src/Build/BackEnd/Components/SdkResolution/DefaultSdkResolver.cs

+    {
+        public override string Name => "DefaultSdkResolver";
+
+        public override int Priority => 10000;


We'll have to come up with a better prioritization scheme for 3rd party support. Even if we do not expect many resolver implementations, there still will be some, and we can't go with a global registry of priorities. :)

cdmihai · 2017-04-24T16:59:59Z

src/Build/BackEnd/Components/SdkResolution/SdkResolution.cs

+
+        private void Initialize(ILoggingService logger, BuildEventContext buildEventContext, ElementLocation location)
+        {
+            lock (_lockObject)


Why not a thread safe Lazy?

cdmihai · 2017-04-24T17:02:28Z

src/Build/BackEnd/Components/SdkResolution/SdkResolution.cs

+        /// <param name="sdk">SDK referenced by the Project.</param>
+        /// <param name="logger">Logging service.</param>
+        /// <param name="buildEventContext">Build event context for logging.</param>
+        /// <param name="importElementLocation">Location of the element within the project which referenced the SDK.</param>


importElementLocation makes it sound like SDKs can only come from msbuild import elements. Maybe SDKReferenceLocation would be a better name?

livarcocc · 2017-04-24T17:30:04Z

@nguerrera

cdmihai · 2017-04-24T17:58:19Z

src/Build/BackEnd/Components/SdkResolution/SdkResolution.cs

+
+            foreach (var result in results)
+            {
+                LogWarnings(logger, buildEventContext, importElementLocation, result);


doesn't this lead to re-logging the success results again? They also get logged at line 68

Never mind, did not see the return on success.

In reply to: 113014254 [](ancestors = 113014254)

cdmihai · 2017-04-24T18:06:57Z

src/Build/BackEnd/Components/SdkResolution/SdkResolution.cs

+            {
+                LogWarnings(logger, buildEventContext, importElementLocation, result);
+
+                if (result.Errors != null)


I'd log errors only when:

there was no successful resolver

there was at least one successful resolver and the log verbosity is diagnostic. And in this case I'd maybe convert the errors to warnings / messages so they don't fail the build.

Never mind, did not see the return on success.

In reply to: 113016412 [](ancestors = 113016412)

However, I'd still log errors when verbosity is diagnostic. To help users debug all resolvers and understand why a certain one did not work.

In reply to: 113016807 [](ancestors = 113016807,113016412)

Though I'd still convert the errors to messages if the log verbosity is diagnostic.

cdmihai · 2017-04-24T18:29:08Z

src/Build/BackEnd/Components/SdkResolution/SdkResolution.cs

+            lock (_lockObject)
+            {
+                if (_resolvers != null) return;
+                _resolvers = _sdkResolverLoadingStrategy.LoadResolvers(logger, buildEventContext, location);


Wonder if it's worth logging the resolver order when the log is diagnostic. Could be really helpful when debugging the resolver chain.

cdmihai · 2017-04-24T18:35:12Z

src/Framework/Sdk/SdkResult.cs

+        /// <summary>
+        ///     Indicates the resolution was successful.
+        /// </summary>
+        public bool Success { get; protected set; }


Since the SdkResultFactory mentions warnings and errors, maybe the SdkResult should do to. Either via Warning / Error properties, or a Diagnostic list, where the Diagnostic.Type is either Warning / Error. This way you could avoid casting to the implementation.

cdmihai · 2017-04-24T18:49:06Z

src/Build.UnitTests/BackEnd/SdkResolverLoadingStrategy_Tests.cs

+    public class SdkResolverLoadingStrategy_Tests
+    {
+        [Fact]
+        public void AssertDefaultLoadingStretegyReturnsDefaultResolver()


Stretegy typo

cdmihai · 2017-04-24T18:53:05Z

src/Build/BackEnd/Components/SdkResolution/SdkResolverLoadingStrategy.cs

+            ElementLocation location)
+        {
+            // Always add the default resolver
+            var resolvers = new List<SdkResolver> {new DefaultSdkResolver()};


Should the default resolver only get added when there are no explicit resolvers set?

If the default is there all the time, it means it might work when the build should actually fail. Maybe the SdkResult should contain a ShouldFailTheBuild member.

cdmihai · 2017-04-24T19:07:48Z

src/Framework/Sdk/ReferencedSdk.cs

+
+            if (!string.IsNullOrWhiteSpace(sdk))
+            {
+                var parts = sdk.Split('/').Select(i => i.Trim()).ToArray();


What about the range syntax (e.g.FooSDK/min:3.4)? I guess MSBuild should be doing that, otherwise all resolvers would duplicate the logic.

cdmihai · 2017-04-24T19:11:34Z

src/Build/Construction/ProjectImportElement.cs

@@ -91,7 +91,12 @@ public string Sdk
        /// added because of the <see cref="ProjectRootElement.Sdk"/> attribute and the location where the project was
        /// imported.
        /// </summary>
-        public ImplicitImportLocation ImplicitImportLocation { get; internal set; } = ImplicitImportLocation.None;
+        public ImplicitImportLocation ImplicitImportLocation { get; internal set; }


Who sets the default to None? The ProjectParser could do it when it hits real import elements.

cdmihai · 2017-04-24T19:26:26Z

src/Build/Construction/ProjectRootElement.cs

+                nodes.Add(ProjectImportElement.CreateImplicit("Sdk.targets", currentProjectOrImport, ImplicitImportLocation.Bottom, referencedSdk));
+            }
+
+            foreach (var sdkNode in Children.OfType<ProjectSdkElement>())


What if, the ProjectSdkElement has a condition on it? The ProjectElementContainer allows for it. Then, imports that are placed above an Sdk element might affect that condition via conditions / file system side effects.

rainersigwald

Only remaining major concern is the Impl cast, if it's a design flaw.

rainersigwald · 2017-04-25T15:47:36Z

src/Build/BackEnd/Components/SdkResolution/DefaultSdkResolver.cs

@@ -9,6 +9,12 @@ namespace Microsoft.Build.BackEnd
 {
    /// <summary>
    ///     Default SDK resolver for compatibility with VS2017 RTM.
+    /// <remarks>
+    ///     Default Sdk folder will to:


rainersigwald · 2017-04-25T15:50:43Z

src/Framework.UnitTests/SdkReference_Tests.cs

+        [Fact]
+        public void VerifySdkReferenceParseWithWhitespace()
+        {
+            string sdkString = "       Name            /   min=Version    ";


Can you make one of these hunks a \t, just for good measure?

rainersigwald · 2017-04-25T16:01:27Z

src/Build/BackEnd/Components/SdkResolution/SdkResolution.cs

+                    var resultFactory = new SdkResultFactoryImpl(sdk);
+                    try
+                    {
+                        var result = (SdkResultImpl)sdkResolver.Resolve(sdk, context, resultFactory);


Still concerned about this. Does it represent a design flaw, or is it just an implementation fix we can deal with later?

rainersigwald · 2017-04-25T16:29:22Z

src/Framework/Sdk/SdkResolver.cs

@@ -28,6 +28,9 @@ public abstract class SdkResolver
        /// <returns>
        ///     An <see cref="SdkResult" /> containing the resolved SDKs or associated error / reason
        ///     the SDK could not be resolved.
+        ///     <remarks>
+        ///         Note: You must use the <see cref="SdkResult" /> to return a result.


SdkResultFactory?

* Adds support for MSBuild to discover plug-in based resolvers for the 'SDK' import feature. A resolver assembly can be placed in: MSBuild\SdkResolver\(ResolverName)\(ResolverName).dll MSBuild will create an instance of any class that implements SdkResolver and attempt to resolve all SDK references with those resolver (sorted by SdkResolver.Priority). * Adds support for MSBuild to consume an Sdk reference as element (as an alternative to a property on the Project element). Both versions of the syntax will add an implicit Import element at the top and the bottom of the file. Related to dotnet#1493

nguerrera · 2017-04-25T19:48:54Z

Let me know when packages are available and what version to use.

* This feature was regressed in dotnet#2002 * Update unit tests to verify functionality * Update ProjectParse to parse values and construct an SdkReference object to be used by the evaluator. Closes dotnet#2034

* This feature was regressed in dotnet#2002 * Update unit tests to verify functionality * Update ProjectParser to parse values and construct an SdkReference object to be used by the evaluator. Closes dotnet#2034

* This feature was regressed in dotnet#2002 * Update unit tests to verify functionality * Update ProjectParser to parse SDK name/version values and construct an SdkReference object to be used by the evaluator. Closes dotnet#2034

* This feature was regressed in #2002 * Update unit tests to verify functionality * Update ProjectParser to parse SDK name/version values and construct an SdkReference object to be used by the evaluator. Closes #2034

dnfclas added the cla-already-signed label Apr 22, 2017

Sarabeth-Jaffe-Microsoft added the Needs Review label Apr 22, 2017

AndyGerlicher force-pushed the sdkresolvers branch 4 times, most recently from 2297ba3 to b9aa944 Compare April 23, 2017 06:49

rainersigwald reviewed Apr 24, 2017

View reviewed changes

cdmihai reviewed Apr 24, 2017

View reviewed changes

AndyGerlicher force-pushed the sdkresolvers branch 3 times, most recently from 6749ad4 to c74c8e5 Compare April 25, 2017 02:34

rainersigwald approved these changes Apr 25, 2017

View reviewed changes

rainersigwald reviewed Apr 25, 2017

View reviewed changes

AndyGerlicher changed the base branch from master to preview/vs15.3 April 25, 2017 18:16

AndyGerlicher force-pushed the sdkresolvers branch from d8d7501 to 3d1fa75 Compare April 25, 2017 18:24

AndyGerlicher force-pushed the sdkresolvers branch from 3d1fa75 to b0a6d07 Compare April 25, 2017 18:25

AndyGerlicher changed the base branch from preview/vs15.3 to master April 25, 2017 18:34

AndyGerlicher merged commit 9e22548 into dotnet:master Apr 25, 2017

cdmihai removed the Needs Review label Apr 25, 2017

rainersigwald mentioned this pull request Apr 28, 2017

Tracking: .NET Core Sdk in Build Tools Installer #1697

Closed

AndyGerlicher mentioned this pull request May 1, 2017

Fix issue importing an SDK via Import element. #2038

Merged

natemcmaster mentioned this pull request May 11, 2017

Proposal: implement a SdkResolver for NuGet packages NuGet/Home#5220

Closed

AndyGerlicher deleted the sdkresolvers branch June 26, 2018 20:35


		// Evaluate the "top" implicit imports as if they were the first entry in the file.

Add support for SDK Resolvers #2002

Add support for SDK Resolvers #2002

Conversation

AndyGerlicher commented Apr 22, 2017 • edited Loading

rainersigwald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainersigwald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdmihai Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdmihai Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

livarcocc commented Apr 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdmihai Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdmihai Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainersigwald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguerrera commented Apr 25, 2017

AndyGerlicher commented Apr 22, 2017 •

edited

Loading

cdmihai Apr 24, 2017 •

edited

Loading

cdmihai Apr 24, 2017 •

edited

Loading

cdmihai Apr 24, 2017 •

edited

Loading

cdmihai Apr 24, 2017 •

edited

Loading