Create replacement API for Microsoft.DotNet.ProjectModel #180

natemcmaster · 2016-10-03T20:10:57Z

Creates a new package "Microsoft.Extensions.ProjectModel.Sources" which contains API for project evaluation to replace Microsoft.DotNet.ProjectModel.

Follows patterns from Mircrosoft.DotNet.ProjectModel

cc @NTaylorMullen @bricelam @prafullbhosale @muratg @Eilon

prafullbhosale · 2016-10-03T20:25:45Z

src/Microsoft.Extensions.ProjectModel.Sources/MsBuild/MsBuildProjectContextBuilder.cs

+
+namespace Microsoft.Extensions.ProjectModel
+{
+    internal class MsBuildProjectContextBuilder


Also consider adding the ability to specify TargetFramework.
Certain targets may only be available when a TargetFramework is specified.
ResolvePackageDependenciesDesignTime is one such target.

What format does MSBuild expect TargetFramework to be? e.g. .NETCoreApp,1.0 or netcoreapp1.0

AFAIK netcoreapp1.0

Added .WithTargetFramework

prafullbhosale · 2016-10-03T20:33:00Z

src/Microsoft.Extensions.ProjectModel.Sources/DotNet/DotNetProjectContext.cs

+        /// <param name="propertyName"></param>
+        /// <param name="propertyNameComparer"></param>
+        /// <returns></returns>
+        public string FindProperty(string propertyName, StringComparison propertyNameComparer)


Nit: move methods below properties and fields.

prafullbhosale · 2016-10-03T20:37:08Z

src/Microsoft.Extensions.ProjectModel.Sources/DotNet/ProjectContextExtensions.cs

+        private const string DefaultConfiguration = "Debug";
+        public static IProjectContext AsAbstract(this ProjectContext context)
+        {
+            return new DotNetProjectContext(context, DefaultConfiguration, outputPath: null);


Consider getting configuration as parameter with a default value of "Debug"

prafullbhosale · 2016-10-03T20:37:50Z

src/Microsoft.Extensions.ProjectModel.Sources/IProjectContext.cs

+        string PackagesDirectory { get; }
+        string TargetDirectory { get; }
+        string AssemblyFullPath { get; }
+        string FindProperty(string propertyName, StringComparison propertyNameComparer);


Move the method below properties?

prafullbhosale · 2016-10-03T20:44:13Z

src/Microsoft.Extensions.ProjectModel.Sources/Internal/DotNetCoreSdkResolver.cs

+            {
+                return null;
+            }
+            var t = new { sdk = new { version = "" } };


Is this being used?

Nope, will clean this up.

prafullbhosale · 2016-10-03T20:54:57Z

src/Microsoft.Extensions.ProjectModel.Sources/MsBuild/MsBuildProjectContextBuilder.cs

+            return this;
+        }
+
+        public virtual IProjectContext Build()


Consider Changing return type to MsBuildProjectContext.

prafullbhosale · 2016-10-03T21:00:08Z

src/Microsoft.Extensions.ProjectModel.Sources/MsBuild/MsBuildProjectContextBuilder.cs

+
+            if (logger.Errors.Count > 0)
+            {
+                throw new InvalidOperationException(logger.Errors[0].Message);


Why only the first one?

Good point. I'll aggregate all errors.

troydai · 2016-10-03T20:35:47Z

makefile.shade

@@ -16,6 +16,7 @@ k-standard-goals
        if (Directory.Exists("src") && !IsLinux)
        {
          Directory.CreateDirectory(BUILD_DIR_LOCAL);
+          DotnetPack("src/Microsoft.Extensions.ProjectModel.Sources/project.json", BUILD_DIR_LOCAL, E("Configuration"), E("KOREBUILD_DOTNET_PACK_OPTIONS") + " --no-build");


why do we need this?

Because this makefile overrides the default build target.

troydai · 2016-10-03T20:45:50Z

src/Microsoft.Extensions.ProjectModel.Sources/DotNet/DotNetProjectContext.cs

+                _rawProject = JObject.Load(jsonReader);
+            }
+            _project = wrappedProject;
+            _paths = wrappedProject.GetOutputPaths(configuration, /* buildBasePath: */ null, outputPath);


doesn't seem to be our convention to leave comment between parameter. i understand where it comes from but suggest against this approach. a named parameter is more flexible with tools when you rename parameter through IDE.

troydai · 2016-10-03T20:46:48Z

src/Microsoft.Extensions.ProjectModel.Sources/DotNet/DotNetProjectContext.cs

+{
+    internal class DotNetProjectContext : IProjectContext
+    {
+        private readonly ProjectContext _project;


rename to _projectContext to improve readability

troydai · 2016-10-03T20:47:17Z

src/Microsoft.Extensions.ProjectModel.Sources/DotNet/DotNetProjectContext.cs

+            using (var streamReader = new StreamReader(stream))
+            using (var jsonReader = new JsonTextReader(streamReader))
+            {
+                _rawProject = JObject.Load(jsonReader);


doesn't the project property on project context already have the project.json parsed?

if it is really necessary, make this process lazy.

I couldn't find API that exposes PJ as a grab bag of key/value pairs...but if it exists, happy to use it instead.

question is, do we need to. i know the FindProperty is defined on the interface, but all the information of a project.json is exposed through the project model. the project.json shouldn't be treated as a simple dictionary.

all the information of a project.json is exposed through the project model

Not user secrets id, or other custom properties...

troydai · 2016-10-03T20:51:01Z

src/Microsoft.Extensions.ProjectModel.Sources/DotNet/DotNetProjectContext.cs

+        public IEnumerable<string> CompilationItems
+            => _compilerOptions.CompileInclude.ResolveFiles();
+        public IEnumerable<string> EmbededItems
+            => _compilerOptions.EmbedInclude.ResolveFiles();


make both properties same line definition

and move them ahead of the first public method

nit: is the method ResolveFiles costy? like globbing files. if so make the properties to cache the results.

Yes, these are costly, but in theory can change over time. Callers should expect any evaluation of this yields the latest set of items and that each evaluation could yield different results.

Please still change the code style.

troydai · 2016-10-03T21:07:05Z

src/Microsoft.Extensions.ProjectModel.Sources/MsBuild/MsBuildContext.cs

+    internal class MsBuildContext
+    {
+        public string MsBuildExecutableFullPath { get; set; }
+        public string ExtensionsPath { get; set; }


can both properties be readonly?

troydai · 2016-10-03T21:08:18Z

src/Microsoft.Extensions.ProjectModel.Sources/MsBuild/MsBuildProjectContextBuilder.cs

+            WithProperty("_ResolveReferenceDependencies", "true");
+        }
+
+        protected virtual ProjectCollection CreateProjectCollection()


troydai · 2016-10-03T21:09:30Z

src/Microsoft.Extensions.ProjectModel.Sources/MsBuild/MsBuildProjectContextBuilder.cs

+        {
+            // don't to expensive things
+            return WithProperty("DesignTimeBuild", "true")
+                     .WithProperty("_ResolveReferenceDependencies", "true")


my ocd tells me that the W should be aligned.

and you fix that! thank you!

troydai · 2016-10-03T21:10:49Z

src/Microsoft.Extensions.ProjectModel.Sources/MsBuild/MsBuildProjectContextBuilder.cs

+        }
+
+        public MsBuildProjectContextBuilder WithBuildTargets(string[] targets)
+        {


check null?

troydai · 2016-10-03T21:14:49Z

src/Microsoft.Extensions.ProjectModel.Sources/MsBuild/MsBuildProjectContextBuilder.cs

+                var xml = ProjectRootElement.Create(xmlReader, projectCollection, preserveFormatting: true);
+                xml.FullPath = fileInfo.PhysicalPath;
+
+                return new Project(xml, globalProps, /*toolsVersion:*/ null, projectCollection);


named parameter instead of comment among parameters

NTaylorMullen

⌚

NTaylorMullen · 2016-10-03T21:45:31Z

src/Microsoft.Extensions.ProjectModel.Sources/DotNet/DotNetProjectContext.cs

+            _isExecutable = wrappedProject.ProjectFile.GetCompilerOptions(wrappedProject.TargetFramework, configuration).EmitEntryPoint
+                            ?? wrappedProject.ProjectFile.GetCompilerOptions(null, configuration).EmitEntryPoint.GetValueOrDefault();
+
+            _compilerOptions = _project.ProjectFile.GetCompilerOptions(TargetFramework, Configuration);


Set this before the _isExecutable bit and you can re-use. (Will need to set Configuration before as well)

NTaylorMullen · 2016-10-03T21:46:22Z

src/Microsoft.Extensions.ProjectModel.Sources/DotNet/DotNetProjectContext.cs

+            Configuration = configuration;
+        }
+
+        public bool IsClassLibrary => !_isExecutable;


{ get; } and set this in the ctor instead of having an _isExecutable variable. Can then just use this property.

NTaylorMullen · 2016-10-03T21:48:18Z

src/Microsoft.Extensions.ProjectModel.Sources/DotNet/DotNetProjectContext.cs

+        public string FindProperty(string propertyName, StringComparison propertyNameComparer)
+            => FindProperty<string>(propertyName, propertyNameComparer);
+
+        public T FindProperty<T>(string propertyName, StringComparison propertyNameComparer)


NTaylorMullen · 2016-10-03T21:50:39Z

src/Microsoft.Extensions.ProjectModel.Sources/DotNet/DotNetProjectContext.cs

+        public IEnumerable<string> EmbededItems
+            => _compilerOptions.EmbedInclude.ResolveFiles();
+
+        public ProjectContext Unwrap() => _project;


Do we really need this? It'd be nice to build our tools without the expectation that they can consume the original project context. Aka our IProjectContext should have sufficient information, if it doesn't we should add API.

Good point. I'll removing so we are forced improve the IProjectContext abstraction instead of falling back. We can revisit later if there is a read need for this.

NTaylorMullen · 2016-10-03T21:51:32Z

src/Microsoft.Extensions.ProjectModel.Sources/DotNet/DotNetProjectContextFactory.cs

+        {
+            var project = new ProjectContextBuilder()
+                .AsDesignTime()
+                .WithProject(ProjectReader.GetProject(filePath))


Break statements like these into multiple declarations. It's an additional line but it makes debugging 10x easier if it ever comes to it.

NTaylorMullen · 2016-10-03T21:58:50Z

src/Microsoft.Extensions.ProjectModel.Sources/Internal/DotNetCoreSdkResolver.cs

+            return new DotNetCoreSdk
+            {
+                BasePath = latest.path,
+                Version = latest.version.ToFullString()


Any value in ToStringing here? Why not just let the DotNetCoreSdk class have Version has a semantic version?

Seems that we don't use the version directly so it's wasteful/less informative to do this bit.

Would also let you not use an anonymous object in the logic you use to determine "latest" here and then in the "sdk" logic in the next method

NuGet.Versioning.SematicVersion is an implementation detail of how we figure out which version is "latest". In theory, SDK versions may not be valid SemanticVersions.

If they end up not being valid semantic versioning then doesn't our resolution logic fall apart? Reason being we order the versions based on how semantic versions are ordered.

NTaylorMullen · 2016-10-03T22:01:10Z

src/Microsoft.Extensions.ProjectModel.Sources/MsBuild/InMemoryLogger.cs

+
+namespace Microsoft.Extensions.ProjectModel.Internal
+{
+    internal class InMemoryLogger : ILogger


Any reason it shouldn't be a private class in the class it's used in?

NTaylorMullen · 2016-10-03T22:04:58Z

src/Microsoft.Extensions.ProjectModel.Sources/ProjectContextExtensions.cs

+
+namespace Microsoft.Extensions.ProjectModel
+{
+    internal static class ProjectContextExtensions


These extensions seem wasteful.

Reasons:

First is trying to avoid using ignorecase. Our dev guidelines state to always specify comparison.

Second is overly specific to user secrets. Let the projects who care about user secrets add this API; no need to pollute the IntelliSense list with extra info when it typically will not be needed.

Fair enough. I'm in favor of smaller APIs :)

NTaylorMullen · 2016-10-03T22:05:52Z

src/Microsoft.Extensions.ProjectModel.Sources/MsBuild/MsBuildProjectContextBuilder.cs

+
+        public MsBuildProjectContextBuilder()
+        {
+            Initialize();


lol, why do we need to specify a method that can be overridden (instead of inlining)? Makes me think the builder isn't sufficient as is.

Seems like a lot here is protected virtual. So same question applies there.

The intent is to make it possible to override builder defaults. Overriding methods is easier than unwinding work done in the base constructor.

NTaylorMullen · 2016-10-03T22:08:13Z

src/Microsoft.Extensions.ProjectModel.Sources/MsBuild/MsBuildProjectContextBuilder.cs

+                throw new ArgumentNullException(nameof(filePath));
+            }
+
+            return WithProjectFile(new PhysicalFileInfo(new FileInfo(filePath)));


break apart

natemcmaster · 2016-10-03T22:43:37Z

🆙 📅

Made lots of updates per PR feedback.

troydai

most of the comments in the first review are addressed. the PR looks good to me.

troydai · 2016-10-04T21:17:03Z

src/Microsoft.Extensions.ProjectModel.Sources/DotNet/DotNetProjectContext.cs

+        public IEnumerable<string> CompilationItems
+            => _compilerOptions.CompileInclude.ResolveFiles();
+        public IEnumerable<string> EmbededItems
+            => _compilerOptions.EmbedInclude.ResolveFiles();


troydai · 2016-10-04T21:17:34Z

src/Microsoft.Extensions.ProjectModel.Sources/DotNet/DotNetProjectContext.cs

+        public IEnumerable<string> CompilationItems
+            => _compilerOptions.CompileInclude.ResolveFiles();
+        public IEnumerable<string> EmbededItems
+            => _compilerOptions.EmbedInclude.ResolveFiles();


Please still change the code style.

troydai · 2016-10-04T21:18:22Z

src/Microsoft.Extensions.ProjectModel.Sources/DotNet/DotNetProjectContext.cs

+{
+    internal class DotNetProjectContext : IProjectContext
+    {
+        private readonly ProjectContext _project;


troydai · 2016-10-04T21:18:43Z

src/Microsoft.Extensions.ProjectModel.Sources/DotNet/DotNetProjectContext.cs

+            using (var streamReader = new StreamReader(stream))
+            using (var jsonReader = new JsonTextReader(streamReader))
+            {
+                _rawProject = JObject.Load(jsonReader);


troydai · 2016-10-04T21:20:14Z

src/Microsoft.Extensions.ProjectModel.Sources/Internal/DotNetCoreSdkResolver.cs

+            {
+                return null;
+            }
+            var t = new { sdk = new { version = "" } };


troydai · 2016-10-04T21:26:28Z

src/Microsoft.Extensions.ProjectModel.Sources/MsBuild/InMemoryLogger.cs

+
+        public string Parameters { get; set; }
+        public LoggerVerbosity Verbosity { get; set; }
+        public void Initialize(IEventSource eventSource)


troydai · 2016-10-04T21:27:03Z

src/Microsoft.Extensions.ProjectModel.Sources/MsBuild/MsBuildContext.cs

+    internal class MsBuildContext
+    {
+        public string MsBuildExecutableFullPath { get; set; }
+        public string ExtensionsPath { get; set; }


troydai · 2016-10-04T21:29:04Z

src/Microsoft.Extensions.ProjectModel.Sources/MsBuild/MsBuildProjectContextBuilder.cs

+        {
+            // don't to expensive things
+            return WithProperty("DesignTimeBuild", "true")
+                     .WithProperty("_ResolveReferenceDependencies", "true")


and you fix that! thank you!

NTaylorMullen

Get this in and we can rev on it as we need. 😄

dnfclas added the cla-already-signed label Oct 3, 2016

natemcmaster mentioned this pull request Oct 3, 2016

Create initial prototype of dotnet-user-secrets with MSBuild support #178

Merged

5 tasks

prafullbhosale reviewed Oct 3, 2016

View reviewed changes

troydai suggested changes Oct 3, 2016

View reviewed changes

NTaylorMullen suggested changes Oct 3, 2016

View reviewed changes

natemcmaster force-pushed the namc/project-model-abstractions branch from 6170551 to 2fdd27d Compare October 3, 2016 22:52

natemcmaster mentioned this pull request Oct 4, 2016

Rewrite of scaffolding tool to support MsBuild based projects dotnet/Scaffolding#302

Merged

troydai reviewed Oct 4, 2016

View reviewed changes

troydai approved these changes Oct 4, 2016

View reviewed changes

natemcmaster changed the base branch from feature/msbuild to dev October 4, 2016 21:45

NTaylorMullen approved these changes Oct 4, 2016

View reviewed changes

Create replacement API for Microsoft.DotNet.ProjectModel

40ff75d

natemcmaster force-pushed the namc/project-model-abstractions branch from 2fdd27d to 40ff75d Compare October 4, 2016 22:49

natemcmaster merged commit 40ff75d into dev Oct 4, 2016

natemcmaster added a commit to aspnet/Coherence that referenced this pull request Oct 4, 2016

React to aspnet/DotNetTools#180

5f3d6ed

natemcmaster deleted the namc/project-model-abstractions branch October 4, 2016 22:53

Create replacement API for Microsoft.DotNet.ProjectModel #180

Create replacement API for Microsoft.DotNet.ProjectModel #180

Conversation

natemcmaster commented Oct 3, 2016

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

prafullbhosale Oct 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prafullbhosale Oct 3, 2016 • 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

troydai Oct 3, 2016 • 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

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

NTaylorMullen 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

NTaylorMullen Oct 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NTaylorMullen Oct 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NTaylorMullen Oct 3, 2016 • 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

natemcmaster commented Oct 3, 2016

troydai 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

NTaylorMullen left a comment

Choose a reason for hiding this comment

prafullbhosale Oct 3, 2016 •

edited

Loading

prafullbhosale Oct 3, 2016 •

edited

Loading

troydai Oct 3, 2016 •

edited

Loading

NTaylorMullen Oct 3, 2016 •

edited

Loading

NTaylorMullen Oct 3, 2016 •

edited

Loading

NTaylorMullen Oct 3, 2016 •

edited

Loading